-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[ci] [python-package] add arm64 macOS wheels #6391
Open
jameslamb
wants to merge
20
commits into
master
Choose a base branch
from
python/arm64-wheels
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
+71
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jameslamb
changed the title
WIP: [ci] [python-package] add arm64 macOS wheels
[ci] [python-package] add arm64 macOS wheels
Jun 12, 2024
tagging some others who I think might be interested in this approach / topic: |
guolinke
approved these changes
Jun 12, 2024
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.
Thank you, LGTM!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contributes to #5328
Contributes to #4229
Might help with #5764
arm64
macOS wheels for the Python package (for the M1/M2/M3 Macs)lib_lightgbm.dylib
so that apip
-installedlightgbm
won't segfault when used in a conda environmentNotes for Reviewers
Proposed approach for building
arm64
wheelsIt seems that Azure DevOps does not support
arm64
macOS runners... only GitHub Actions: actions/runner-images#8971.So I'm proposing using GitHub Actions new
macos-14
runners, which are arm64 (#5328 (comment)).Proposed approach for fixing loading errors
The problem "using a
pip
-installedlightgbm
inside a conda environment segfaults" is not specific to arm64. It's been around inlightgbm
's macOS support for a few years. I decided to try to fix it here to be able to test the new arm64 wheels.I'm proposing the following approach:
libomp
lib_lightgbm.dylib
to be more forgiving about wherelibomp.dylib
is foundIn short:
/opt/homebrew/opt/libomp/lib/libomp.dylib
currently stored inlib_lightgb.dylib
forces the macOS loader to load a library from that exact path, even if there is already alibomp.dylib
loaded from some other source (like thellvm-openmp
shipped y conda)@rpath/libomp.dylib
, which means that at runtime oflightgbm
, the loader will do the following (in order)libomp.dylib
already loaded, just use that/opt/homebrew/opt/libomp/lib/
/opt/homebrew/opt/libomp/lib/
)/usr/local/lib
and/usr/lib
See "References" section for the MANY resources I consulted to get to this point. Particularly:
How I tested this
downloaded the wheel built in CI (click me)
Got a run ID from CI, looking at the URL.
e.g. for this build: https://github.com/microsoft/LightGBM/actions/runs/9476495757/job/26109485388?pr=6391
the run id is
9476495757
.Then, following https://docs.github.com/en/actions/managing-workflow-runs/downloading-workflow-artifacts?tool=cli, used the GitHub CLI to download the wheels. A fine-grained access token with just read-only permission on
Actions
should be enough.From the root of the repo, on my M2 Mac laptop:
rm -rf ./whl-tmp mkdir -p ./whl-tmp RUN_ID=9476495757 gh run download \ --repo microsoft/LightGBM \ "${RUN_ID}" \ -n macosx-arm64-wheel \ --dir ./whl-tmp
created a new conda environment (click me)
Created a conda environment.
conda create \ --name delete-me-lgb-dev \ --yes \ --file ./.ci/conda-envs/ci-core.txt \ python=3.10 \ pydistcheck source activate delete-me-lgb-dev
Inspected the wheel with `pydistcheck` (click me)
Installed from the wheel (on my M2 mac, using macOS 14.4.1).
Ran the tests
Those all passed! Removed the brew-installed
libomp
on my system and ran that again.They passed again 🎉
Can
lightgbm
really safely link to anylibomp.dylib
?This project's macOS wheels have already been doing that for years, by shipping with an absolute path that evaluates to "whatever
brew install libomp
installs", and no limits comparing the version linked against at build time to the one used at runtime.The changes in this PR are actually safer than the existing behavior of
lightgbm
's macOS wheels because they avoid a well-known source of runtime issue (loading alibomp.dylib
that conflicts with the one loaded byscikit-learn
).Why not just vendor
libomp.dylib
like other libraries do?Other projects like
lightgbm
bundle a copy of LLVM OpenMP (libomp.dylib
) in their wheels.numpy
andscipy
: https://pypackaging-native.github.io/key-issues/native-dependencies/blas_openmp/xgboost
: [CI] Build Python wheels for MacOS (x86_64 and arm64) dmlc/xgboost#7621scikit-learn
: https://github.com/scikit-learn/scikit-learn/blob/7e8ad632ff7aec09e99f0c4b8e493e13e19aef14/build_tools/wheels/build_wheels.sh#L20-L22That comes with a significantly reduced risk of "cannot find
libomp.dylib
" types of runtime issues, but in exchange for:omp_set_num_threads()
does not affect all the loaded OpenMP libraries, and so projects' parallelism can fight each other and lead to oversubscription)Since
lightgbm
has already been using dynamic linking + not vendoring for several years and since "you have tobrew install libomp
" has generally not been a problem for macOS users, I'm proposing continuing those practices in this release.Will these CMake changes break the
lightgbm
conda package?They shouldn't.
conda-build
rewrites paths in binaries it produces to correctly link between its packages. See "Making packages relocatable" (conda docs)References