-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
CI Build macOS arm wheels [cd build gh] #21827
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
Conversation
build_tools/github/build_wheels.sh
Outdated
# https://packages.macports.org/libomp/. | ||
if [[ "$CIBW_BUILD" == *-macosx_arm64 ]]; then | ||
# arm64 builds must cross compile because CI is on x64 | ||
export PYTHON_CROSSENV=1 | ||
# SciPy requires 12.0 on arm to prevent kernel panics | ||
# https://github.com/scipy/scipy/issues/14688 | ||
# We use the same deployment target to match SciPy. | ||
export MACOSX_DEPLOYMENT_TARGET=12.0 | ||
wget https://packages.macports.org/libomp/libomp-11.0.1_0.darwin_20.arm64.tbz2 -O libomp.tbz2 | ||
else | ||
# Currently, the oldest supported macos version is: High Sierra / 10.13. | ||
# When upgrading this, be sure to update the MACOSX_DEPLOYMENT_TARGET | ||
# environment variable in wheels.yml accordingly. Note that Darwin_17 == | ||
# High Sierra / 10.13. | ||
export MACOSX_DEPLOYMENT_TARGET=10.13 | ||
wget https://packages.macports.org/libomp/libomp-11.0.1_0+universal.darwin_17.i386-x86_64.tbz2 -O libomp.tbz2 |
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.
Just was working on this for scikit-image as well and noticed there were no universal2
libomp wheels on macports. It was suggested in the napari zulip that we could potentially use lipo
to make a universal2 libomp
from the separate x86_64
and arm64
ones, but perhaps it is not worth the effort at this point?
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.
I do not think it's worth the effort for now. I plan to revisit if an issue opens up and there is demand for a universal2
wheel.
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.
also, I'm not sure about needing to bump to 12.0 for the deployment target. I had left this at 11.0 on scikit-image and the preliminary wheels we built pass the test suite when they were tested (although the user testing was on MacOS 12). Did you encounter failures for scikit-learn with 11.0?
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.
I guess given that we have SciPy as a required dependency and it only provides 12.0 wheels we should perhaps do the same.
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.
I guess given that we have SciPy as a required dependency and it only provides 12.0 wheels we should perhaps do the same.
Yes, I don't see how it would be possible to install scikit-learn or scikit-image wheels on macos 11 if scipy only provides wheels for macos 12+ anyways. Better be consistent in the stack.
I tested the wheels from this PR on Python 3.8, 3.9 and 3.10 with a macOS arm64 M1 machine and the test suite passes. |
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.
Let's wait for the current build to complete, test again locally to check that the conflict resolution with main
did not introduce any unintended side effects (unlikely) and then merge.
Scratch that, 310 is already configured here. There is no dependency between the PRs. |
The unrelated Azure Pipelines failures on Windows has already been reported here: #21798. |
Running the tests again locally from the latest artifacts (only Python 3.10). Will merge if green. |
All tests pass. I have the feeling that when running with But this can be improved in the future. All tests are green, merging. |
I needed to set |
So how do we install scikit-learn on M1 chips exactly? |
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>
Reference Issues/PRs
Fixes #19137
What does this implement/fix? Explain your changes.
This PR:
macosx_x86_64
,macosx_arm64
] the build matrix looked a little too complicated usingexclude
+ a product of configurations. I think listing out all the configurations is easier to reason about.Any other comments?
I tested the Python 3.9 build on my local m1 machine and the test pass. I'll check again with the wheels built in this PR.