Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mumps in Homebrew #20429

Closed
Tracked by #17231
jwnimmer-tri opened this issue Oct 26, 2023 · 25 comments
Closed
Tracked by #17231

Mumps in Homebrew #20429

jwnimmer-tri opened this issue Oct 26, 2023 · 25 comments
Assignees

Comments

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Oct 26, 2023

Is your feature request related to a problem? Please describe.

The goal is to allow Drake to build Ipopt the way we want to (with different knobs + patches). To do that, I'd like to have Mumps available in Homebrew-core. (Ipopt depends on Mumps, but building Mumps from source with Bazel is difficult. Ubuntu already provides a Mumps we can use; the only blocker is on macOS with Homebrew.)

One way would be to take the ipopt.rb formula and split out the Mumps part of it into its own formula/library. Right now, only the Mumps shared library is available in brew, not the headers. If we had the Mumps headers in brew, then Drake could build our own Ipopt the way we want it configured, relying on the OS to provide Mumps.

Or possibly a simpler change would be to keep it as one formula, but just install the Mumps headers as alongside the Ipopt headers.

In the Ubuntu build, the header we need are these:

        "dmumps_c.h",
        "mumps_c_types.h",
        "mumps_compat.h",
        "mumps_int_def.h",
        "mumps_seq/elapse.h",
        "mumps_seq/mpi.h",
        "mumps_seq/mpif.h",

I expect the same would be true for the macOS build.

Describe the solution you'd like

There exists some homebrew-core formula we can install such that we have the Mumps shared library and Mumps headers available via homebrew.

Additional context

Towards #17231.

Edit: See Homebrew/homebrew-core#154418.

@jwnimmer-tri
Copy link
Collaborator Author

For avoidance of doubt -- using a custom tap would not be an acceptable solution. The only upside of Homebrew here is that users already have it available without any extra hassle, so we need this in homebrew-core directly.

@jwnimmer-tri
Copy link
Collaborator Author

Final thought -- my aim here is to do it in a way that makes the homebrew maintainers happiest, so they keep these formulae intact over the coming years. So long as Drake has the headers and library, I'm happy.

@johnwparent
Copy link
Contributor

This PR: Homebrew/homebrew-core#154418 vendors a NON HPC mumps because brew doesn't allow options in core. I've got another set of changes that introduce a MUMPS more useful for HPC purposes (i.e. MPI, parmetis, and scotch support) that I'll add if the first PR goes well.

@jwnimmer-tri
Copy link
Collaborator Author

I can't quite correlate the new mumps.rb with the old ipopt.rb build of mumps, but the only features Drake needs are the same baseline as Ipopt had. I think that means we would not use the formula that has MPI, parmetis, etc. enabled. (Parmetis, for example, does not have an open source license.)

@jwnimmer-tri
Copy link
Collaborator Author

... the only features Drake needs are the same baseline as Ipopt had.

I can actually clarify this further. The most features we need would be what Ipopt.rb's build of MUMPS provided. If it ends up that some of the features are a problem in the new formula, we can talk about dropping them in case Drake might not need them.

In no case do we need more features than what Ipopt.rb's build of MUMPS provided.

@johnwparent
Copy link
Contributor

johnwparent commented Nov 16, 2023

... the only features Drake needs are the same baseline as Ipopt had.

I can actually clarify this further. The most features we need would be what Ipopt.rb's build of MUMPS provided. If it ends up that some of the features are a problem in the new formula, we can talk about dropping them in case Drake might not need them.

In no case do we need more features than what Ipopt.rb's build of MUMPS provided.

AFAICT the minimal MUMPs I PR'd is exactly the same as the ipopt build of MUMPs. I also don't see that ipopt depends on MPI so likely that dep (and scotch/metis) were not used for the ipopt build. I think getting the PR I linked landed should be sufficient for your needs, but if you folks try it out and find something missing, I'll be happy to add to my PR (or make a new one as needed).

@jwnimmer-tri
Copy link
Collaborator Author

Great, thanks.

My plan is to let the Homebrew pull request land first (and get bottled, I suppose), before I start doing test builds in Drake.

@johnwparent
Copy link
Contributor

before I start doing test builds in Drake.

If you'd like I can try some minimal build just to verify the binaries produced have all the symbols you folks need? I'd hate to waste the time it might take getting that PR landed only to find out the port wasn't sufficient.

@jwnimmer-tri
Copy link
Collaborator Author

That might be easier said that done, but anyway here's the outline in case you want to try.

It probably makes sense to have @svenevs or @mwoehlke-kitware help test since they understand Drake's build system, but the steps would be something like this:

(1) Apply this patch to Drake:

--- a/tools/workspace/ipopt/repository.bzl
+++ b/tools/workspace/ipopt/repository.bzl
@@ -6,8 +6,8 @@ def ipopt_repository(name):
         name = name,
         mapping = {
             "macOS default": [
-                "ipopt=@ipopt_internal_pkgconfig//:ipopt_internal_pkgconfig",
-                "install=@ipopt_internal_pkgconfig//:install",
+                "ipopt=@ipopt_internal_fromsource//:ipopt",
+                "install=@ipopt_internal_fromsource//:install",
             ],
             "Ubuntu default": [
                 "ipopt=@ipopt_internal_fromsource//:ipopt",

(2) Hack on the two files in drake/tools/workspace/mumps_internal to change the hard-coded include path(s), linker path, and linked library name to match your local homebrew binaries.

(3) Run bazel test //solvers:ipopt_solver_test.

It's likely that the @ipopt_internal_fromsource rules don't work on macOS yet anyway, so it might not even be possible to make much progress until I can work on that.

@jwnimmer-tri
Copy link
Collaborator Author

jwnimmer-tri commented Nov 16, 2023

Actually, let me take that back -- there's an easier way.

(1) Install Homebrew/homebrew-core#154418 locally (i.e., use that pull request to build and install a new MUMPS as well as a new IPOPT).

(2) Run bazel test //solvers:ipopt_solver_test in Drake.

There's no need to switch to Drake's "from source" build of IPOPT. If the revised Homebrew IPOPT works for Drake, then the MUMPS part is fine on its own.

@jwnimmer-tri
Copy link
Collaborator Author

Any updates on the schedule of this work? The homebrew PR seems to have stalled out on our end.

@johnwparent
Copy link
Contributor

Pushed some updates today. Mumps is building fine, just working out some unexpected linker behavior that differs from Linux to MacOS. @svenevs get me setup on the Mac machines so I can test properly now.
I expect to have the CI green by the end of today.

@jwnimmer-tri
Copy link
Collaborator Author

jwnimmer-tri commented Jan 18, 2024

@svenevs Could you please locally install the Homebrew PR, and then try running a Drake build & test. At least on one arch. If you can do both arches, that would be slightly better. The only question is whether we broke IPOPT insofar as Drake's tests can determine. (We don't need to check if the MUMPS on its own is good enough for our future work.) IOW, just check for regressions.

@johnwparent
Copy link
Contributor

@svenevs FWIW the machine you setup for me has the PR Mumps + ipopt installed already

@svenevs
Copy link
Contributor

svenevs commented Jan 22, 2024

Tests are looking good

$ bazel test --keep_going //...
INFO: Analyzed 12056 targets (0 packages loaded, 0 targets configured).
INFO: Found 4385 targets and 7671 test targets...
FAIL: //geometry/optimization:iris_test (see /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/execroot/drake/bazel-out/darwin_arm64-opt/testlogs/geometry/optimization/iris_test/test.log)
INFO: From Testing //geometry/optimization:iris_test:
==================== Test output for //geometry/optimization:iris_test:
Using drake_cc_googletest_main.cc

[==========] Running 18 tests from 3 test suites.
[----------] Global test environment set-up.
[----------] 10 tests from IrisTest
[ RUN      ] IrisTest.NoObstacles
[       OK ] IrisTest.NoObstacles (29 ms)
[ RUN      ] IrisTest.NoObstacles2
[       OK ] IrisTest.NoObstacles2 (2 ms)
[ RUN      ] IrisTest.SmallBox
[       OK ] IrisTest.SmallBox (6 ms)
[ RUN      ] IrisTest.BallInBox
[       OK ] IrisTest.BallInBox (0 ms)
[ RUN      ] IrisTest.MultipleBoxes
[       OK ] IrisTest.MultipleBoxes (2 ms)
[ RUN      ] IrisTest.StartingEllipse
[       OK ] IrisTest.StartingEllipse (0 ms)
[ RUN      ] IrisTest.BoundingRegion
[       OK ] IrisTest.BoundingRegion (3 ms)
[ RUN      ] IrisTest.TerminationConditions
[       OK ] IrisTest.TerminationConditions (2 ms)
[ RUN      ] IrisTest.BallInBoxNDims
[       OK ] IrisTest.BallInBoxNDims (34 ms)
[ RUN      ] IrisTest.ClosestPointFailure
[       OK ] IrisTest.ClosestPointFailure (15 ms)
[----------] 10 tests from IrisTest (100 ms total)

[----------] 1 test from IrisOptionsTest
[ RUN      ] IrisOptionsTest.Serialize
[       OK ] IrisOptionsTest.Serialize (0 ms)
[----------] 1 test from IrisOptionsTest (0 ms total)

[----------] 7 tests from SceneGraphTester
[ RUN      ] SceneGraphTester.MakeSphere
[       OK ] SceneGraphTester.MakeSphere (9 ms)
[ RUN      ] SceneGraphTester.MakeCylinder
[       OK ] SceneGraphTester.MakeCylinder (0 ms)
[ RUN      ] SceneGraphTester.MakeHalfSpace
[       OK ] SceneGraphTester.MakeHalfSpace (0 ms)
[ RUN      ] SceneGraphTester.MakeBox
[       OK ] SceneGraphTester.MakeBox (0 ms)
[ RUN      ] SceneGraphTester.MakeCapsule
[       OK ] SceneGraphTester.MakeCapsule (0 ms)
[ RUN      ] SceneGraphTester.MakeEllipsoid
[       OK ] SceneGraphTester.MakeEllipsoid (0 ms)
[ RUN      ] SceneGraphTester.MultipleBoxes
thread '<unnamed>' panicked at external/crate__clarabel-0.6.0/src/solver/core/cones/psdtrianglecone.rs:292:9:
not implemented: Mixed PSD and Exponential/Power cones are not yet supported
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
================================================================================
FAIL: //examples/pendulum:trajectory_optimization_simulation_test (see /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/execroot/drake/bazel-out/darwin_arm64-opt/testlogs/examples/pendulum/trajectory_optimization_simulation_test/test_attempts/attempt_1.log)
FAIL: //examples/pendulum:trajectory_optimization_simulation_test (see /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/execroot/drake/bazel-out/darwin_arm64-opt/testlogs/examples/pendulum/trajectory_optimization_simulation_test/test_attempts/attempt_2.log)
FAIL: //examples/pendulum:trajectory_optimization_simulation_test (see /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/execroot/drake/bazel-out/darwin_arm64-opt/testlogs/examples/pendulum/trajectory_optimization_simulation_test/test.log)

FAILED: //examples/pendulum:trajectory_optimization_simulation_test (Summary)
      /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/execroot/drake/bazel-out/darwin_arm64-opt/testlogs/examples/pendulum/trajectory_optimization_simulation_test/test.log
      /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/execroot/drake/bazel-out/darwin_arm64-opt/testlogs/examples/pendulum/trajectory_optimization_simulation_test/test_attempts/attempt_1.log
      /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/execroot/drake/bazel-out/darwin_arm64-opt/testlogs/examples/pendulum/trajectory_optimization_simulation_test/test_attempts/attempt_2.log
INFO: From Testing //examples/pendulum:trajectory_optimization_simulation_test:
==================== Test output for //examples/pendulum:trajectory_optimization_simulation_test:
Failed to solve optimization for the swing-up trajectory
================================================================================
==================== Test output for //examples/pendulum:trajectory_optimization_simulation_test:
Failed to solve optimization for the swing-up trajectory
================================================================================
==================== Test output for //examples/pendulum:trajectory_optimization_simulation_test:
Failed to solve optimization for the swing-up trajectory
================================================================================
INFO: Elapsed time: 6.078s, Critical Path: 0.82s
INFO: 4 processes: 8 darwin-sandbox, 1 local.
INFO: Build completed, 2 tests FAILED, 4 total actions
//geometry/optimization:iris_test                                        FAILED in 0.3s
  /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/execroot/drake/bazel-out/darwin_arm64-opt/testlogs/geometry/optimization/iris_test/test.log
//examples/pendulum:trajectory_optimization_simulation_test              FAILED in 3 out of 3 in 0.4s
  Stats over 3 runs: max = 0.4s, min = 0.1s, avg = 0.2s, dev = 0.1s
  /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/execroot/drake/bazel-out/darwin_arm64-opt/testlogs/examples/pendulum/trajectory_optimization_simulation_test/test.log
  /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/execroot/drake/bazel-out/darwin_arm64-opt/testlogs/examples/pendulum/trajectory_optimization_simulation_test/test_attempts/attempt_1.log
  /private/var/tmp/_bazel_stephen.mcdowell/c82709baf62040b4ad09f16721f3b518/execroot/drake/bazel-out/darwin_arm64-opt/testlogs/examples/pendulum/trajectory_optimization_simulation_test/test_attempts/attempt_2.log

Executed 3 out of 7671 tests: 7669 tests pass and 2 fail locally.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.

There really isn't any straightforward way to use openmp 0.3.25, which are the two failures being shown here (see: #20799).

@jwnimmer-tri
Copy link
Collaborator Author

Sounds fine, thanks. That's good enough to let the Homebrew PR merge and then I'll circle back later to start trying out some Ipopt changes in Drake.

@jwnimmer-tri
Copy link
Collaborator Author

@johnwparent a reminder -- if I'm reading the brew pull request correctly, you still need to push the "depends gcc build only" and "cd testpath" fixups to the brew pull request? Maybe if you can do that soon, they'll just merge it as-is.

@johnwparent
Copy link
Contributor

@jwnimmer-tri Returned from being away today.
MUMPS got back to me, they are very amicable toward upstreaming changes that get them into brew and improve support for MacOS. I'm working with them on a best path forward, but that still leaves us with the issue of brew tracking the upstream.
I've made the requested edits on the brew side, and informed them of the changes and the MUMPS response signaling willingness to accept some of the requested changes.
To address the issue of tracking the upstream, I suggested that we add a docstring instructing the next dev to bump the MUMPS recipe version in brew to verify if package builds without the "customizations" I had to add, and if so, remove the customizations and the docstring. I'm hoping this is enough that the brew folks are OK moving forward.

@johnwparent
Copy link
Contributor

johnwparent commented Mar 14, 2024

@jwnimmer-tri Weekly update: I think it's likely you're caught up w.r.t. this issue as you were the last person to comment on the brew PR, but long story short, AFAICT, we're just waiting on a brew dev to hit merge. I've made all requested changes, and it seems like we've reached a consensus on not requiring a MUMPs upstream issue to track, which were the only remaining blockers prior to last week, so we should be good to move forward. My reviewer +1'd my comment summarizing this and suggesting LGTM but did not actually move the PR toward merging.

My current plan is to wait until Monday (~1 week after your last ping) and ping them again, although I'm interested to hear your thoughts on our approach.

@jwnimmer-tri
Copy link
Collaborator Author

One way would be to take the ipopt.rb formula and split out the Mumps part of it into its own formula/library. Right now, only the Mumps shared library is available in brew, not the headers. If we had the Mumps headers in brew, then Drake could build our own Ipopt the way we want it configured, relying on the OS to provide Mumps.

This approach is having trouble receiving approval from the homebrew maintainers. I think it's still worth following up and trying to answer the questions / add docs in case we can get it merged. However ...

Or possibly a simpler change would be to keep it as one formula, but just install the Mumps headers as alongside the Ipopt headers.

Let's also try this approach in parallel. Could you please open a different homebrew PR with this approach, and see if it fares better?

@johnwparent
Copy link
Contributor

answer the questions / add docs in case we can get it merged. However ...

Will answer the questions as best I can, however the docs request leaves me concerned as there are no public facing docs for the build (only the API). The comment the reviewer refers to is pulled from the README (or maybe the makefiles directly, I forget), and they generally seem unwilling to move forward unless we can get those docs/references based on this:

willing to change my mind here if there's enough documentation added and linked to

I have a couple ideas for alternate approaches I would be happy to discuss.

Let's also try this approach in parallel. Could you please open a different homebrew PR with this approach, and see if it fares better?

Absolutely will do early next week, the brew PL seems to prefer this anyway.

@jwnimmer-tri
Copy link
Collaborator Author

Let's also try this approach in parallel. Could you please open a different homebrew PR with this approach, and see if it fares better?

Absolutely will do early next week, the brew PL seems to prefer this anyway.

Gentle ping -- remember this work is still pending (at least, I haven't seen a new PR yet).


Also of note -- the original PR Homebrew/homebrew-core#154418 got some recent feedback from what looks like a community member(?). If you want to try to keep editing that PR to make people happy and get it merged, I am OK with that. Up to your discretion.

I'm also OK if you want to focus on the upcoming second PR exclusively, and only if it's rejected to we circle back to the first one.

@BetsyMcPhail
Copy link
Contributor

Second PR: Homebrew/homebrew-core#171266

@jwnimmer-tri
Copy link
Collaborator Author

jwnimmer-tri commented Jul 11, 2024

Hi @johnwparent could you please ping Homebrew/homebrew-core#171266 one more time and see if we can convince upstream to look at it?

(I am satisfied leave Homebrew/homebrew-core#154418 closed and give up on that approach.)

@jwnimmer-tri
Copy link
Collaborator Author

Homebrew gave us the cold shoulder. Nothing more we can do.

Next step: burn it with fire: #21714.

@jwnimmer-tri jwnimmer-tri closed this as not planned Won't fix, can't repro, duplicate, stale Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants