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

Add support for publishing macOS M1 ARM64 wheels #2559

Merged
merged 2 commits into from
Sep 10, 2021

Conversation

lgeiger
Copy link
Contributor

@lgeiger lgeiger commented Sep 7, 2021

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:

  • I've properly formatted my code according to the guidelines
    • By running Black + Flake8
    • By running pre-commit hooks
  • This PR addresses an already submitted issue for TensorFlow Addons
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • This PR contains modifications to C++ custom-ops

How Has This Been Tested?

I manually verified the generated wheel on a ARM64 M1 Mac.

I am not sure what to do about the Run the cpu tests in a small python docker image CI failure. It seems unrelated to this PR. @seanpmorgan What do you think?

@boring-cyborg boring-cyborg bot added the github label Sep 7, 2021
@google-cla google-cla bot added the cla: yes label Sep 7, 2021
python configure.py

bazel build \
--cpu=darwin_arm64 \
Copy link
Contributor Author

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.

Comment on lines +12 to +13
--copt -mmacosx-version-min=11.0 \
--linkopt -mmacosx-version-min=11.0 \
Copy link
Contributor Author

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"
Copy link
Contributor Author

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.

@bhack bhack marked this pull request as ready for review September 7, 2021 23:34
Comment on lines +46 to +53
- 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'
Copy link
Contributor Author

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.

Copy link
Contributor

@bhack bhack Sep 8, 2021

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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.

Copy link
Contributor

@bhack bhack Sep 8, 2021

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

Copy link
Contributor Author

@lgeiger lgeiger Sep 8, 2021

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.

Copy link
Contributor

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.

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.

@mihaimaruseac and @penpornk

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 ?

Copy link
Contributor

@bhack bhack Sep 9, 2021

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.

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.

Copy link
Contributor

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?

@lgeiger
Copy link
Contributor Author

lgeiger commented Sep 8, 2021

We briefly tested the generated Python 3.9 wheel and it seems to work fine on a M1 ARM Mac with tensorflow-macos installed.

@lgeiger lgeiger requested a review from bhack September 8, 2021 14:45
@bhack
Copy link
Contributor

bhack commented Sep 8, 2021

/cc @tetsuyasu can you check this on your system?

@bhack
Copy link
Contributor

bhack commented Sep 8, 2021

/cc @mbui0327

@tetsuyasu
Copy link
Contributor

/cc @tetsuyasu can you check this on your system?

It works fine.

Copy link
Member

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

@bhack bhack merged commit 2bb46df into tensorflow:master Sep 10, 2021
@lgeiger lgeiger deleted the macos-arm64 branch September 10, 2021 10:42
@lgeiger
Copy link
Contributor Author

lgeiger commented Sep 10, 2021

Great! Thanks for the fast review and merge 🚀

How do you feel about backporting this to 0.13 and 0.14?

@bhack
Copy link
Contributor

bhack commented Sep 10, 2021

How do you feel about backporting this to 0.13 and 0.14?

I think it is ok.

lgeiger added a commit to lgeiger/addons that referenced this pull request Sep 10, 2021
lgeiger added a commit to lgeiger/addons that referenced this pull request Sep 10, 2021
* Add support for publishing macOS M1 ARM64 wheels

* Link against the tensorflow-macos when building for ARM Macs
bhack pushed a commit that referenced this pull request Sep 10, 2021
* Add support for publishing macOS M1 ARM64 wheels

* Link against the tensorflow-macos when building for ARM Macs
@bhack
Copy link
Contributor

bhack commented Sep 10, 2021

@lgeiger We had some problem in https://github.com/tensorflow/addons/actions/runs/1220898795

macos-11-3.9-tf2.5.0-arm64-wheel" was not found.

@lgeiger
Copy link
Contributor Author

lgeiger commented Sep 10, 2021

@lgeiger We had some problem in https://github.com/tensorflow/addons/actions/runs/1220898795

Very sorry about that, #2565 should fix this typo.

@bhack
Copy link
Contributor

bhack commented Sep 10, 2021

Thanks, when it is merged can you post an announce in the forum?

@seanpmorgan
Copy link
Member

@bhack @lgeiger Any thoughts on how we should handle the lagging tensorflow-macos releases? If it's going to lag by ~month I won't be in favor of waiting for it. IMO the obvious answer is for tensorflow core to publish an arm64 macos whl... @kulinseth are there any plans to submit that PR or is a separate whl the path forward?

Other options I see:

  • We could release a patch once tensorflow-macos catches up, but not really thrilled to absorb that burden and x.0 releases would be exempt from arm64 macos
  • We could only build macos arm for tfa-nightly. And then it's just a best effort thing

...open to other ideas but would like a good reason why tf core can't publish the build along with all the others.

@lgeiger
Copy link
Contributor Author

lgeiger commented Sep 14, 2021

IMO the obvious answer is for tensorflow core to publish an arm64 macos whl

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.

@bhack
Copy link
Contributor

bhack commented Sep 14, 2021

I think It is relatively easy to make a PR
in the MacOS directory I've mentioned.
But the problem is more related to the current CI script orchestration (Kokoro/Jenkins) that It isn't so clear for a contributor as Github actions cause it is not hosted in the repository so we don't have exactly the full overview of the CI.

The second point is:
Do we really need to wait for the tensorflow-metal release also on M1?
Can tensorflow-metal depend on the official TF wheel instead of tensorflow-macos or is there something different in this build other then the arm64 support?

@bhack
Copy link
Contributor

bhack commented Sep 27, 2021

@lgeiger Can you check the CI in the latest PRs?

@lgeiger
Copy link
Contributor Author

lgeiger commented Oct 5, 2021

Sorry for the late reply, I was away the last weeks.

Do we really need to wait for the tensorflow-metal release also on M1?

tensorflow-metal is just the device plugin for metal support for M1 Macs, so as far as I can tell tensorflow-macos can be used without tensorflow-metal just fine.

@lgeiger Can you check the CI in the latest PRs?

@bhack Could you clarify what you mean by that?

@bhack
Copy link
Contributor

bhack commented Oct 5, 2021

Could you clarify what you mean by that?

Nevermind, MacOS builds were failing but it is solved.

@bhack
Copy link
Contributor

bhack commented Nov 6, 2021

@seanpmorgan We could have some effect on the chain if we don't (or with @kulinseth) handle the M1 release.
See what happens in TF federated:
https://github.com/tensorflow/federated/issues/1254#issuecomment-803938932

@seanpmorgan
Copy link
Member

@seanpmorgan We could have some effect on the chain if we don't (or with @kulinseth) handle the M1 release. See what happens in TF federated: tensorflow/federated#1254 (comment)

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)

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

Successfully merging this pull request may close these issues.

tensorflow-addons doesn't work with tensorflow-macos
6 participants