-
Notifications
You must be signed in to change notification settings - Fork 325
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
[WIP] fix issues with dependencies build with Macs with arm64 #3192
base: main
Are you sure you want to change the base?
Conversation
…se pre-installed ipopt
# Use METIS from Homebrew, which is currently 5.1.0 (IPOPT 3.12.8 | ||
# automatically pulls 4.10.0, which had some segfault issues) | ||
set(HOMEBREW_DIR "/opt/homebrew") | ||
set(METIS_INSTALL_DIR "${HOMEBREW_DIR}/opt/metis") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is homebrew the only way to get metis and are these directories hardcoded/unchangeable by users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is homebrew the only way to get metis
There are a few ways to get metis, but there seemed to be issues with using anything but the homebrew version:
- Building from source from here: This built properly, however, the library that comes out does not seem to have the correct path stored in the .dylib when using
otool -L libmetis.dylib
to check dependencies (e.g., it seems relative rather than absolute), so there are linking problems downstream. I suspect there is some issue with the provided configure/cmake step and RPATH, as there is a warning aboutMACOS_RPATH
. - Building from coin-or's repo
ThirdParty-Metis
(link). The latest version here, however, is only 4.10.0, which had segfault issues downstream at runtime. - homebrew: automatically installs 5.1.0, and using this resolved segfaults.
are these directories hardcoded/unchangeable by users
With homebrew, the directory structure is set by homebrew. For arm64
installation, the base directory is /opt/homebrew
. For x86_64
, these are kept separately in /usr/local
. From there, the relative directory of opt/metis
should also be the same. These directories are set by homebrew when installing without any input from users. Whether users could change these afterwards, I'm not sure, but my understanding is that that would not be a typical use of homebrew.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this more surgical approach OK, or should we switch to this for all Mac builds?
I think there's an argument for keeping the build instructions the same for all Mac (and possibly) Windows builds. Especially if we know that the homebrew approach is more reliable than retrieving the libraries from coin-or. Homebrew is simpler than downloading from coin-or, at least in my opinion. If we wanted to do something similar for Windows, hopefully Chocolately can provide Metis/Mumps in the same way (but I'm getting a little off-topic now).
Is adding the option to use a pre-installed IPOPT overkill? I mainly used it for quicker testing locally when trying to isolate dependencies build issues.
I could see this as an advanced option, but not sure how many people would use it. If it's useful as a development tool going forward I would be fine keeping it too.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aymanhab and @carmichaelong)
dependencies/ipopt.patch
line 7 at r1 (raw file):
// In newer ThirdParty/Mumps, mpi.h is renamed to mumps_mpi.h. // We get informed about this by having COIN_USE_MUMPS_MPI_H defined. +#include "mumps_compat.h"
Why is this new include needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Homebrew is simpler than downloading from coin-or, at least in my opinion. If we wanted to do something similar for Windows, hopefully Chocolately can provide Metis/Mumps in the same way (but I'm getting a little off-topic now).
This would be great, but just to highlight a slight difference right now: The current flow is to only use homebrew for metis. We still rely on coin-or for mumps. However, instead of relying IPOPT 3.12.8 to grab mumps automatically, we grab a different branch (more info on this on your comment at a specific line about the include line). For 4.4, I think it'll be good to keep this surgical as it should only affect a few users (i.e., new M1 owners building Moco from source), but we should consider bumping to newer versions down the road, which may help unify IPOPT versions across different platforms and simplify the dependencies build.
I could see this as an advanced option, but not sure how many people would use it. If it's useful as a development tool going forward I would be fine keeping it too.
Sounds good, will mark the options advanced.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aymanhab and @nickbianco)
dependencies/ipopt.patch
line 7 at r1 (raw file):
Previously, nickbianco (Nicholas Bianco) wrote…
Why is this new include needed?
IPOPT 3.12.8 automatically pulls coin-or-tools/ThirdParty-Mumps
branch stable/1.6
which uses MUMPS 4.10.0. This, however, does not work with the homebrew version of METIS due to some linker errors (likely because homebrew uses METIS 5.1.0, but coin-or-tools/ThirdParty-Metis
uses METIS 4.10.0).
So instead, this modified superbuild pulls coin-or-tools/ThirdParty-Mumps
branch stable/2.1
. This does not have linker errors and still uses MUMPS 4.10.0. My guess is that IPOPT 3.12.8 expected COIN_USE_MUMPS_MPI_H
to be defined in a different place for stable/1.6
and for stable/2.1
. stable/2.1 conveniently defines many of these variables in this
mumps_compat.h` file.
(As an aside, IPOPT 3.13+ uses stable/2.0+
branches of ThirdParty-Mumps
. IPOPT 3.13+ made a breaking change for how dependencies worked, so this gives further support that there was major build changes for these two ThirdParty-Mumps
branches too.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For 4.4, I think it'll be good to keep this surgical
Totally agree. Thanks for the clarification on the include comment below and on how all these dependencies work (both here and in person).
LGTM.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aymanhab)
dependencies/ipopt.patch
line 7 at r1 (raw file):
Previously, carmichaelong wrote…
IPOPT 3.12.8 automatically pulls
coin-or-tools/ThirdParty-Mumps
branchstable/1.6
which uses MUMPS 4.10.0. This, however, does not work with the homebrew version of METIS due to some linker errors (likely because homebrew uses METIS 5.1.0, butcoin-or-tools/ThirdParty-Metis
uses METIS 4.10.0).So instead, this modified superbuild pulls
coin-or-tools/ThirdParty-Mumps
branchstable/2.1
. This does not have linker errors and still uses MUMPS 4.10.0. My guess is that IPOPT 3.12.8 expectedCOIN_USE_MUMPS_MPI_H
to be defined in a different place forstable/1.6
and forstable/2.1
.stable/2.1 conveniently defines many of these variables in this
mumps_compat.h` file.(As an aside, IPOPT 3.13+ uses
stable/2.0+
branches ofThirdParty-Mumps
. IPOPT 3.13+ made a breaking change for how dependencies worked, so this gives further support that there was major build changes for these twoThirdParty-Mumps
branches too.)
Thanks for the clarification!
…is libraries for apple during install
@carmichaelong Just wanted to make sure I'm following your steps exactly:
|
@aymanhab sorry, it was probably pretty unclear which instructions to use because it was a long discussion. I did make it work with the superbuild option, after trying a bunch of different ways to get metis, mumps, and ipopt. See the instructions below:
The |
Also some responses to questions above to be clearer:
The last version I had adjusted the superbuild to work by pulling in and building a specific version of IPOPT, and then doing a small patch, and the superbuild should do this all for you.
Buried in the issue discussion linked has some discussion on the different combinations of mumps, metis, and ipopt versions I've tried. Some of them build, some of them build but fail tests, and some of them build and pass tests (and one of these combinations should be the current superbuild settings here). I'm not sure maybe about what hash(es) you are referring to, so please point out what that might mean and maybe I can remember some info that might be useful there! |
FYI @aymanhab @nickbianco
Related to #3168
Brief summary of changes
Testing I've completed
Works locally, though will fail often when using many cores (e.g.,
make -j8
) and sporadically with fewer (e.g.,make -j4
). There seems to be a race somewhere for making directories during superbuild. Runners don't seem to be available for GitHub actions yet.This version passes all OpenSim tests.
Looking for feedback on...
CHANGELOG.md (choose one)
TODO
This change is