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

[WIP] fix issues with dependencies build with Macs with arm64 #3192

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

carmichaelong
Copy link
Member

@carmichaelong carmichaelong commented Apr 26, 2022

FYI @aymanhab @nickbianco

Related to #3168

Brief summary of changes

  • Update superbuild option for a Mac with arm64 architecture
  • Add mechanism for using a pre-installed IPOPT

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...

  • Is this more surgical approach OK, or should we switch to this for all Mac builds?
  • 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.
  • Not necessarily looking to merge right now (hence [WIP] tag), though this branch will be helpful to make sure I don't break other builds.

CHANGELOG.md (choose one)

TODO


This change is Reviewable

# 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")
Copy link
Member

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?

Copy link
Member Author

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:

  1. 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 about MACOS_RPATH.
  2. 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.
  3. 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.

Copy link
Member

@nickbianco nickbianco left a 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?

Copy link
Member Author

@carmichaelong carmichaelong left a 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.)

Copy link
Member

@nickbianco nickbianco left a 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 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.)

Thanks for the clarification!

@aymanhab
Copy link
Member

aymanhab commented Jul 7, 2023

@carmichaelong Just wanted to make sure I'm following your steps exactly:

  1. I merged your branch onto 4.4

  2. on dependencies build turn-off superbuild_ipopt, turn on use_ipopt_install based on the conversation above

  3. is there an assumed separate "brew install ipopt" ?
    I guess I'm confused because you're not superbuilding ipopt but applying a patch, does it mean ipopt has to be built manually after the patch or are this taken care of by the make files?

  4. Did you investigate using superbuild with a different hash(es) that support arm64 out of the box, or is that an overkill?

@carmichaelong
Copy link
Member Author

@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:

brew install cmake swig gcc pkgconfig autoconf libtool automake wget doxygen metis
git clone https://github.com/opensim-org/opensim-core.git
cd opensim-core
git switch dependencies-mac-arm64
cd ..
mkdir opensim_dependencies_build
cd opensim_dependencies_build
cmake ../opensim-core/dependencies \
      -DCMAKE_INSTALL_PREFIX="../opensim_dependencies_install" \
      -DCMAKE_BUILD_TYPE=RelWithDebInfo \
      -DSUPERBUILD_ezc3d=ON \
      -DOPENSIM_WITH_CASADI=ON \
      -DOPENSIM_WITH_TROPTER=ON
make -j4
cd ..
mkdir opensim_build
cd opensim_build
cmake ../opensim-core \
      -DCMAKE_INSTALL_PREFIX="../opensim_install" \
      -DCMAKE_BUILD_TYPE=RelWithDebInfo \
      -DBUILD_PYTHON_WRAPPING=OFF \
      -DBUILD_JAVA_WRAPPING=OFF \
      -DOPENSIM_DEPENDENCIES_DIR="../opensim_dependencies_install"
make -j8
ctest -j8

The use_ipopt_install was a flag I was using often when trying different IPOPT installs. If we keep it, it would stay as advanced, but I'm not sure if we should keep it or not.

@carmichaelong
Copy link
Member Author

Also some responses to questions above to be clearer:

is there an assumed separate "brew install ipopt" ?
I guess I'm confused because you're not superbuilding ipopt but applying a patch, does it mean ipopt has to be built manually after the patch or are this taken care of by the make files?

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.

Did you investigate using superbuild with a different hash(es) that support arm64 out of the box, or is that an overkill?

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants