Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

build: Install tensorflow-macos on Apple silicon Macs #2119

Merged
merged 3 commits into from
Feb 16, 2023

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Feb 16, 2023

Description

Use hatchling's environment markers system to use platform information to determine when the installing machine is an Apple silicon Mac and install tensorflow-macos instead of tensorflow in this situation. This works because of the following:

  • x86-64 Linux machine:
>>> import platform
>>> platform.machine()
'x86_64'
>>> platform.system()
'Linux'
  • x86-64 Apple machine:
>>> import platform
>>> platform.machine()
'x86_64'
>>> platform.system()
'Darwin'
  • AArch64 machine:
>>> import platform
>>> platform.machine()
'aarch64'
>>> platform.system()
'Linux'
  • Apple silicon machine:
>>> import platform
>>> platform.machine()
'arm64'
>>> platform.system()
'Darwin'

This is a nice workaround for tensorflow/tensorflow#57185.

The check for an Apple silicon Mac is purposefully over specified with both platform_machine and platform_system as I do not know for certain that arm64 will be reserved for Apple silicon machines forever.

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Use hatchling's environment markers system to use platform information
  to determine when the installing machine is an Apple silicon Mac
  and install tensorflow-macos instead of tensorflow in this situation.
   - c.f. https://hatch.pypa.io/latest/config/dependency/#environment-markers
* This works because of the following:
   - x86-64 Linux machine:
     >>> import platform
     >>> platform.machine()
     'x86_64'
     >>> platform.system()
     'Linux'
   - x86-64 Apple machine:
     >>> import platform
     >>> platform.machine()
     'x86_64'
     >>> platform.system()
     'Darwin'
   - AArch64 machine:
     >>> import platform
     >>> platform.machine()
     'aarch64'
     >>> platform.system()
     'Linux'
   - Apple silicon machine:
     >>> import platform
     >>> platform.machine()
     'arm64'
     >>> platform.system()
     'Darwin'

* Use hatchling's environment markers system to use platform information
  to determine when the installing machine is an Apple silicon Mac
  and install tensorflow-macos instead of tensorflow in this situation.
   - c.f. https://hatch.pypa.io/latest/config/dependency/#environment-markers
* This works because of the following:
   - x86 64 machine:
     >>> import platform
     >>> platform.machine()
     'x86_64'
     >>> platform.system()
     'Linux'
   - AArch64 machine:
     >>> import platform
     >>> platform.machine()
     'aarch64'
     >>> platform.system()
     'Linux'
   - Apple silicon machine:
     >>> import platform
     >>> platform.machine()
     'arm64'
     >>> platform.system()
     'Darwin'
@matthewfeickert matthewfeickert added fix A bug fix build Changes that affect the build system or external dependencies labels Feb 16, 2023
@matthewfeickert matthewfeickert self-assigned this Feb 16, 2023
pyproject.toml Outdated
@@ -66,7 +66,8 @@ Homepage = "https://github.com/scikit-hep/pyhf"
[project.optional-dependencies]
shellcomplete = ["click_completion"]
tensorflow = [
"tensorflow>=2.7.0", # c.f. PR #1962
"tensorflow>=2.7.0; platform_machine != 'arm64'", # c.f. PR #1962
"tensorflow-macos>=2.7.0; platform_machine == 'arm64' and platform_system == 'Darwin'", # c.f. PR #1962
Copy link
Member Author

Choose a reason for hiding this comment

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

Purposefully over specified with both platform_machine and platform_system as I do not know for certain that arm64 will be reserved for Apple silicon machines forever.

@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Base: 98.30% // Head: 98.30% // No change to project coverage 👍

Coverage data is based on head (b970ed4) compared to base (46ce316).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2119   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          69       69           
  Lines        4531     4531           
  Branches      645      645           
=======================================
  Hits         4454     4454           
  Misses         45       45           
  Partials       32       32           
Flag Coverage Δ
contrib 97.88% <ø> (ø)
doctest 61.15% <ø> (ø)
unittests-3.10 96.31% <ø> (ø)
unittests-3.8 96.33% <ø> (ø)
unittests-3.9 96.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Feb 16, 2023

@alexander-held @lukasheinrich as you both have Apple silicon machines can you check that this PR works? Just

$ python -m pip install --upgrade -e '.[backends]'

should be enough.

@matthewfeickert
Copy link
Member Author

I was able to get this tested on a M2 machine and it works there, so as @kratsg has signed off I'm going to merge this. 👍

@matthewfeickert matthewfeickert merged commit 74441ab into main Feb 16, 2023
@matthewfeickert matthewfeickert deleted the build/use-environment-markers branch February 16, 2023 18:50
matthewfeickert added a commit that referenced this pull request Mar 31, 2023
* Backport PR #2095 to the release/v0.7.x branch for easier comparisons.
* Also backport components of:
    - PR #2072
    - PR #2099
    - PR #2100
    - PR #2119
    - PR #2125
    - PR #2145
* In filterwarnings ignore all DeprecationWarning on patch release branches
  (e.g. release/v0.7.x).
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
build Changes that affect the build system or external dependencies fix A bug fix
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Cannot resolve dependencies in "all" setup extra
2 participants