RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.17k stars 1.24k forks source link

Mumps in Homebrew #20429

Open jwnimmer-tri opened 8 months ago

jwnimmer-tri commented 8 months ago

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 https://github.com/Homebrew/homebrew-core/pull/154418.

jwnimmer-tri commented 8 months ago

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 commented 8 months ago

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 commented 7 months ago

This PR: https://github.com/Homebrew/homebrew-core/pull/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 commented 7 months ago

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 commented 7 months ago

... 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 commented 7 months ago

... 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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

(1) Install https://github.com/Homebrew/homebrew-core/pull/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 commented 5 months ago

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

johnwparent commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

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

svenevs commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

@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 commented 4 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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 https://github.com/Homebrew/homebrew-core/pull/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 commented 1 month ago

Second PR: https://github.com/Homebrew/homebrew-core/pull/171266