Skip to content

Bundle OpenMP (libomp) and libgfortran dylibs for macOS wheels, fix deployment target#4128

Merged
kratman merged 41 commits into
pybamm-team:developfrom
agriyakhetarpal:attempt-two-fix-openmp-macos-wheels
Jun 5, 2024
Merged

Bundle OpenMP (libomp) and libgfortran dylibs for macOS wheels, fix deployment target#4128
kratman merged 41 commits into
pybamm-team:developfrom
agriyakhetarpal:attempt-two-fix-openmp-macos-wheels

Conversation

@agriyakhetarpal

@agriyakhetarpal agriyakhetarpal commented Jun 3, 2024

Copy link
Copy Markdown
Member

Description

Important

Please refer to #4128 (comment) below for instructions on testing the wheels.

Fixes #4091; adds the LLVM implementation of OpenMP (used in scikit-image) and a custom gfortran distribution for the choice of a Fortran compiler where both of them have been provided against a carefully set MACOSX_DEPLOYMENT_TARGET (10.9 for Intel Macs and 11.0 for Silicon chips). We set these to 11.0 and 11.0 respectively because of CasADi and a few other dependencies, which do not set this setting when compiling using the Apple SDK. The earlier implementation was using the libomp package via Homebrew, and Homebrew binaries are not meant for linking and vendoring purposes of distribution due to a variety of reasons, such as being specific to the architecture and version of macOS being installed on. See #4092 for more context.

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@agriyakhetarpal agriyakhetarpal force-pushed the attempt-two-fix-openmp-macos-wheels branch 2 times, most recently from 97aaaf3 to 51ece0a Compare June 3, 2024 12:20
@agriyakhetarpal

Copy link
Copy Markdown
Member Author

For some reason, I can't get cibuildwheel to recognise the compilers that conda or mamba install from conda-forge (they come with OpenMP enabled). I'll look into another way to do this in a moment.

@agriyakhetarpal agriyakhetarpal force-pushed the attempt-two-fix-openmp-macos-wheels branch 5 times, most recently from de6ec10 to b406ece Compare June 3, 2024 14:50
@agriyakhetarpal

Copy link
Copy Markdown
Member Author

This is getting closer, I feel. I'll try to test out the wheels as well (just to confirm that we don't get any weird sigkill errors or memory leaks that we noted with #4092).

@agriyakhetarpal

Copy link
Copy Markdown
Member Author

I guess I used big numbers of CI runs for today, so I have to wait for my fork's CI and before I start getting allocated runners on request instead of queues.

@agriyakhetarpal agriyakhetarpal force-pushed the attempt-two-fix-openmp-macos-wheels branch from 6528bb0 to 8b6225c Compare June 3, 2024 16:38
@agriyakhetarpal

Copy link
Copy Markdown
Member Author

Almost there, rebasing to get the fix from #4125

@agriyakhetarpal

agriyakhetarpal commented Jun 3, 2024

Copy link
Copy Markdown
Member Author

There is some work needed to get at least the integration test suite running with cibuildwheel (with the IDAKLU solver tests not being skipped, that is – the rest of the tests proceed and pass right now), which I tried on a previous workflow run. However, the wheel builds are passing and the artifacts can be obtained from here: https://github.com/agriyakhetarpal/PyBaMM/actions/runs/9355079582/job/25749427627

I can mark this as ready for review. I still think we should test this better than what was implemented last time in #4092. I just tested locally with a fresh mamba environment and outside the PyBaMM directory, with a workflow roughly similar to this:

mamba create -n pybamm-dev python=3.12
mamba activate pybamm-dev
python -m pip install "path/to/your/downloaded/wheels/pybamm-24.1-cp312-cp312-macosx_11_0_arm64.whl[dev,all]"

# An initial test
python -c "import pybamm; print(pybamm.IDAKLUSolver())"  # should print an instantiated Python object

# Next, run the test suite
# Copy your PyBaMM's tests/ folder somewhere, say, in your home directory, and then
python -m pytest $HOME/pybamm_tests_copied/

I have all tests (both unit and integration) passing, and the IDAKLU solver tests are not being skipped (the ones being skipped or any of those failing due to missing CSV files are unrelated, of course – they can be fixed later).

@kratman and @BradyPlanden, it would be great if you both could try this out as well on your macOS machines with the downloaded wheel, thanks! In my case, I ran the tests with macOS Sonoma 14.5. Once this is resolved, we can get the PyBaMM and therefore the PyBOP releases rolling (cc: @Saransh-cpp).

@agriyakhetarpal agriyakhetarpal marked this pull request as ready for review June 3, 2024 18:46
@agriyakhetarpal agriyakhetarpal changed the title Bundle OpenMP and libgfortran dylibs for macOS wheels, fix deployment target Bundle OpenMP (libomp) and libgfortran dylibs for macOS wheels, fix deployment target Jun 3, 2024
@agriyakhetarpal agriyakhetarpal changed the title Bundle OpenMP (libomp) and libgfortran dylibs for macOS wheels, fix deployment target Bundle OpenMP (libomp) and libgfortran dylibs for macOS wheels, fix deployment target Jun 3, 2024
Comment thread .github/workflows/publish_pypi.yml Outdated
Comment thread .github/workflows/publish_pypi.yml Outdated
Comment thread .github/workflows/publish_pypi.yml Outdated
@agriyakhetarpal

Copy link
Copy Markdown
Member Author

In the coming months, we should bump SuiteSparse to v7 so that we can avoid the use of Fortran altogether (by setting SUITESPARSE_USE_FORTRAN to OFF). I don't know if Fortran is optional for SUNDIALS routines as well. OpenMP will have to stay until we get wheels for it on PyPI or another place that makes installing the dynamic libraries easier.

@kratman

kratman commented Jun 3, 2024

Copy link
Copy Markdown
Contributor

@agriyakhetarpal I should have time to test this on Wednesday afternoon

@BradyPlanden

Copy link
Copy Markdown
Member

Hi @agriyakhetarpal, I can confirm the IDAKLU tests complete successfully from the above wheels on my M3 / 14.5 laptop. Nice one!

@agriyakhetarpal

agriyakhetarpal commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

Thank you, @BradyPlanden! That makes two M-series devices. @kratman, is it okay if we can merge and get the release ready or would you prefer we wait until you test on Wednesday?

We should get someone to test on an Intel Mac as well, but I'm not sure who has them. Let me write a small workflow to do the above in CI by sharing the artifacts across jobs. If the tests pass there, I feel we can go ahead and merge.

@BradyPlanden

Copy link
Copy Markdown
Member

We should get someone to test on an Intel Mac as well, but I'm not sure who has them.

If you struggle to find someone, I have access to one I can test on.

@agriyakhetarpal

Copy link
Copy Markdown
Member Author

If you struggle to find someone, I have access to one I can test on.

That will be helpful, thanks! In addition, I just triggered https://github.com/agriyakhetarpal/PyBaMM/actions/runs/9364191057 to test the Intel wheels in a separate job, I hope that works.

@agriyakhetarpal

Copy link
Copy Markdown
Member Author

The workflow above has passed (all the relevant tests at least, we can ignore the failing tests and the Python 3.12 job safely). This confirms that wheels for both amd64 and arm64 architectures are working without issues. We are now ready for PyBaMM v24.5rc0 and further release candidates, and for PyBOP v24.5 soon after.

@Saransh-cpp Saransh-cpp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, @agriyakhetarpal! I could not go through it in detail but the wheels work for me locally.

@agriyakhetarpal

agriyakhetarpal commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

That makes three M-series machines (locally) and one Intel machine (GitHub Actions). It would be great if you could test on your Intel one as well, @BradyPlanden. It might be a bit extra but all I want is to not break anything :)

On arm64, another way the amd64 wheels can be tested is by installing the amd64 Python from the official Python.org binaries, which will then run through Rosetta.

@BradyPlanden

BradyPlanden commented Jun 4, 2024

Copy link
Copy Markdown
Member

Tested on intel x86 macOS (13.6) and can confirm the idaklu tests pass 🎉

@kratman

kratman commented Jun 5, 2024

Copy link
Copy Markdown
Contributor

I am about to start testing this one. I will merge it after I am done since it is already approved

@kratman kratman merged commit d77d12c into pybamm-team:develop Jun 5, 2024
@agriyakhetarpal agriyakhetarpal deleted the attempt-two-fix-openmp-macos-wheels branch June 5, 2024 19:17
js1tr3 pushed a commit to js1tr3/PyBaMM that referenced this pull request Aug 12, 2024
…ix deployment target (pybamm-team#4128)

* Setup minimal conda/mamba environment file

* Don't set Homebrew-OpenMP  for PyPI wheels

* Use `setup-miniconda` to access compilers

* Set deployment target for amd64, arm64, separately

* Rename macOS wheel job

* Add `miniconda-version: "latest"` input to action

* Fix deployment target env var

* Fix Linux syntax error

* Add a fortran compiler as well

* Set FC explicitly – can't be found? Not sure

* Activate pybamm-dev environment in CIBW_BEFORE_ALL

* Set CC, CXX, and FCC, don't use XCode compilers

* Run `conda init` before `conda activate`

* Create environment via file inside `CIBW_BEFORE_ALL`

* Use micromamba instead of conda

* Use Homebrew to install miniforge

* Set FC and PATH for mamba to find compiler

* Don't use cibuildwheel action, use PyPI package

* Activate mamba environment

* Try to activate Miniforge-mamba environment

* Delete minimal environment file

* `scikit-image`'s solution, conda for LLVM-OpenMP

* Set correct path to Miniforge envs

* Include tests in sdist and wheel

* Install gfortran from scipy's solution

* Fix CI environment variable

* Copy libomp dylib for repair

* Improve wheel test command

* Export SuiteSparse paths to search libomp dylib for

* Copy libomp dylib to local directory again

* Target 11.0, repair against 11.1, rename wheel

* Run tests from {project} for now, not {package}

* Fix conditional binary operator

* Run integration tests, add note for `libgcc_s.1.dylib`

* Remove incorrect tests, import IDAKLU for now

* Use `set -e -x` to verify PyBaMM import

* Apply suggestions from code review

* Update publish_pypi.yml
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.

Fortnightly build for wheels failed / PyBaMM import kills process on macOS M-series

4 participants