-
Notifications
You must be signed in to change notification settings - Fork 613
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
Add support for publishing macOS M1 ARM64 wheels #2559
Conversation
python configure.py | ||
|
||
bazel build \ | ||
--cpu=darwin_arm64 \ |
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.
This CPU setting makes sure that we cross compile for ARM64.
--copt -mmacosx-version-min=11.0 \ | ||
--linkopt -mmacosx-version-min=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.
We don't need to support 10.13 since the only the latest version macOS version currently supports ARM64. But setting a minimum version here makes sure that the script will keep working even if the GitHub actions VM are updated to 11.1 at some point in the future.
--test_output=errors \ | ||
build_pip_pkg | ||
|
||
bazel-bin/build_pip_pkg artifacts "--plat-name macosx_11_0_arm64 $NIGHTLY_FLAG" |
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.
We need to manually set the correct wheel name, since we are cross compiling on an x86 machine.
- os: 'macos-11' | ||
cpu: 'arm64' | ||
tf-version: '2.5.0' | ||
py-version: '3.8' | ||
- os: 'macos-11' | ||
cpu: 'arm64' | ||
tf-version: '2.5.0' | ||
py-version: '3.9' |
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.
tensorflow-macos
currently only supports TF 2.5.0 and Python 3.8 and 3.9.
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.
And tensorflow-metal?
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.
As far as I can tell tensorflow-metal
is an optional dependency to enable metal support via the new pluggable device mechanism so it is not really necessary here since as far as I know TFA doesn't directly link against the metal plugin.
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.
Yes I meant if It Is constrained to 2.5.0 now I suppose that all the macos stuffs are aligned on 2.5.0 including GPU acell
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.
Yes, tensorflow-metal
is also only built against 2.5.0 currently.
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.
Yes, as I told you it is not about conflicts, It is about that if tensorflow-macos
is imminent to land on 2.6.0 as tensorflow-deps
is ready for TF 2.6.0 I will just wait to edit this PR directly to have a 2.6.0 wheel published for TF addons.
If not probably we will merge as is.
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.
Yes, as I told you it is not about conflicts, It is about that if
tensorflow-macos
is imminent to land on 2.6.0 astensorflow-deps
is ready for TF 2.6.0 I will just wait to edit this PR directly to have a 2.6.0 wheel published for TF addons.
If not probably we will merge as is.
Thanks @bhack for the heads-up. Is this PR for enabling ARM64 builds for TF through official build channels which will be hosted on pypi ?
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 PR for enabling ARM64 builds for TF through official build channels which will be hosted on pypi ?
No this is limited for Tensorflow Addons ARM64 builds in the official Tesnroflow Addons pypi so we need to know if we could enable 2.6.0 for tensorflow-macos
2.6.0.
ARM64 builds for TF through official build channels which will be hosted on pypi
For this you really need a brand new PR in the TF own repository over:
(Stable)
https://github.com/tensorflow/tensorflow/tree/master/tensorflow/tools/ci_build/rel/macos
(Eventually nightly)
https://github.com/tensorflow/tensorflow/tree/master/tensorflow/tools/ci_build/nightly_release/macos
But it is another topic.
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.
@bhack , thanks for the update. We are working on the v2.6 release, I will update here regarding that. I see this PR has merged we can adopt to tensorflow-macos
after we make the release in a separate PR.
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.
@kulinseth Are you interested to manage the TF addons wheels release on your pypi on a new tensorflow-macos/metal
release?
We briefly tested the generated Python 3.9 wheel and it seems to work fine on a M1 ARM Mac with |
/cc @tetsuyasu can you check this on your system? |
/cc @mbui0327 |
It works fine. |
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.
LGTM from a build standpoint. Thanks @lgeiger we've had a number of requests for this.
Great! Thanks for the fast review and merge 🚀 How do you feel about backporting this to 0.13 and 0.14? |
I think it is ok. |
* Add support for publishing macOS M1 ARM64 wheels * Link against the tensorflow-macos when building for ARM Macs
@lgeiger We had some problem in https://github.com/tensorflow/addons/actions/runs/1220898795
|
Very sorry about that, #2565 should fix this typo. |
Thanks, when it is merged can you post an announce in the forum? |
@bhack @lgeiger Any thoughts on how we should handle the lagging Other options I see:
...open to other ideas but would like a good reason why tf core can't publish the build along with all the others. |
I fully agree. To me, it seems like there shouldn't be any technical blockers for a proper arm64 macos wheel as it can be cross compiled on the existing macos CI infra. It is probably just a matter of priorities for the TF/Apple teams. |
I think It is relatively easy to make a PR The second point is: |
@lgeiger Can you check the CI in the latest PRs? |
Sorry for the late reply, I was away the last weeks.
@bhack Could you clarify what you mean by that? |
Nevermind, MacOS builds were failing but it is solved. |
@seanpmorgan We could have some effect on the chain if we don't (or with @kulinseth) handle the M1 release. |
Given discussion at the last SIG meeting, we won't support TFA releases on M1 ARM until an acceptable SLA can be given from Apple on their release of tensorflow-macos. I'm okay with continuing the TFA-Nightly support though while we wait to see what happens. Only issue there is tfa-nightly arm may be compiled against an older TF version (would only affect custom ops) |
Description
This PR adds a release script and updated the GitHub actions workflow to publish cross compiled ARM64 macOS wheels. Recent XCode version support cross compilation, so we can generate ARM64 wheels on the x86 GitHub actions VMs.
Fixes #2503
Checklist:
How Has This Been Tested?
I manually verified the generated wheel on a ARM64 M1 Mac.
I am not sure what to do about theRun the cpu tests in a small python docker image
CI failure. It seems unrelated to this PR. @seanpmorgan What do you think?