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

Promote arm64 macOS to tier 1 #12102

Merged
merged 21 commits into from
Apr 30, 2024
Merged

Conversation

mtreinish
Copy link
Member

Summary

Github recently added a new macOS runner that is using the m1 CPU that is usable for open source projects. [1] Previously Qiskit had support for arm64 macOS at tier 3 because we were only able to cross compile for the platform and not test the binaries. Now that we can run CI jobs on the platform we're able to run both unit tests and test our binaries on release. This commit adds a new set of test jobs and wheel builds that use the macos-14 runner that mirrors the existing x86_64 macOS jobs we have. This brings arm64 macOS to the same support level as x86_64. THe only difference here is that azure pipelines doesn't have arm64 macOS support so the test job will run in github actions instead.

Details and comments

[1] https://github.blog/changelog/2024-01-30-github-actions-introducing-the-new-m1-macos-runner-available-to-open-source/

Github recently added a new macOS runner that is using the m1 CPU that
is usable for open source projects. [1] Previously Qiskit had support
for arm64 macOS at tier 3 because we were only able to cross compile
for the platform and not test the binaries. Now that we can run CI jobs
on the platform we're able to run both unit tests and test our binaries
on release. This commit adds a new set of test jobs and wheel builds
that use the macos-14 runner that mirrors the existing x86_64 macOS
jobs we have. This brings arm64 macOS to the same support level as
x86_64. THe only difference here is that azure pipelines doesn't have
arm64 macOS support so the test job will run in github actions instead.

[1] https://github.blog/changelog/2024-01-30-github-actions-introducing-the-new-m1-macos-runner-available-to-open-source/
@mtreinish mtreinish added type: qa Issues and PRs that relate to testing and code quality Changelog: New Feature Include in the "Added" section of the changelog labels Mar 29, 2024
@mtreinish mtreinish added this to the 1.1.0 milestone Mar 29, 2024
@mtreinish mtreinish requested a review from a team as a code owner March 29, 2024 16:14
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@mtreinish mtreinish added the on hold Can not fix yet label Mar 29, 2024
@mtreinish
Copy link
Member Author

I've marked this as on hold until we verify the job configurations work.

@coveralls
Copy link

coveralls commented Mar 29, 2024

Pull Request Test Coverage Report for Build 8902346045

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 19 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.02%) to 89.503%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 7 91.6%
crates/qasm2/src/parse.rs 12 97.15%
Totals Coverage Status
Change from base Build 8902271173: -0.02%
Covered Lines: 61363
Relevant Lines: 68560

💛 - Coveralls

mtreinish added a commit to mtreinish/qiskit-documentation that referenced this pull request Mar 29, 2024
In Qiskit/qiskit#12102 we're promoting the macOS arm64 environment to
tier 1 now that github actions exposes native arm64 macOS workers. This
commit updates the documentation accordingly. This should not be merged
until Qiskit 1.1.0 is released (or 1.0.3 if we decided to backport
Qiskit/qiskit#12102).
@mtreinish
Copy link
Member Author

So there are 2 big issues so far with this. The first is 6 to 12 tests are failing because of numpy precision issues that probably come down to SIMD differences or issues with accelerate on arm64. This also causes the PGO test runs to fail in the wheel builds.

The second issue is that there are no Python 3.8 official Python binaries available on macOS arm64. We were previously publishing arm64 py3.8 wheels with cross compilation on x86_64, but now that we're doing native builds for arm64 the builds fail because it tries to run with x86_64 python. I think the only path to workaround that is to add back the cross-build job and have it only run for python 3.8. This precludes doing PGO, but I think the number of arm64 macOS Python 3.8 users will be very low if there aren't official Python packages for it so that's an ok tradeoff.

@mtreinish
Copy link
Member Author

I've pushed up #12197 to remove linear algebra with numpy from the equation in the isometry code path to fix the issues we're having here. This should be good to go once #12197 merges and we update the PR branch

I've tested that tests work here: https://github.com/mtreinish/qiskit-core/actions/runs/8730127221 and I'm debugging the wheel builds here: https://github.com/mtreinish/qiskit-core/actions/runs/8730127218/job/23953358067 once that's working I'll remove the test commits from this branch to get this in a mergable state again.

The only other change I think we should make here is tweaking one of the test jobs to run with MSRV instead of the latest stable. But that's trivial to do.

@mtreinish
Copy link
Member Author

I've tested that unittest jobs: https://github.com/mtreinish/qiskit-core/actions/runs/8736818119

and wheel builds: https://github.com/mtreinish/qiskit-core/actions/runs/8736818125

with the current state of this PR and #12197 applied. So I'm going to revert 97eaa6f and prepare this final merging now. The test CI jobs will still fail until we merge #12197 but otherwise I think this will be ready.

@mtreinish mtreinish removed the on hold Can not fix yet label Apr 18, 2024
github-merge-queue bot pushed a commit to qiskit-community/qiskit-experiments that referenced this pull request Apr 25, 2024
Reverts #1443 since dulwich made a new release with @wshanks's fix. Also
updates the mac test runner to `macos-13` since `macOS-latest` is now
arm-based and no longer supports python3.8. We should eventually follow
Qiskit/qiskit#12102 and add arm support.
Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

Looks mostly good.

It's really cool to see us finally able to run CI for macOS ARM variants :D.

.github/workflows/wheels.yml Outdated Show resolved Hide resolved
@mtreinish mtreinish requested a review from kevinhartman April 30, 2024 20:40
@kevinhartman kevinhartman added this pull request to the merge queue Apr 30, 2024
@kevinhartman kevinhartman removed this pull request from the merge queue due to a manual request Apr 30, 2024
@mtreinish mtreinish enabled auto-merge April 30, 2024 22:20
@mtreinish mtreinish added this pull request to the merge queue Apr 30, 2024
Merged via the queue into Qiskit:main with commit f2b874b Apr 30, 2024
15 checks passed
@mtreinish mtreinish deleted the macos-arm64-tier1 branch May 1, 2024 00:11
github-merge-queue bot pushed a commit to Qiskit/documentation that referenced this pull request May 16, 2024
In Qiskit/qiskit#12102 we're promoting the macOS arm64 environment to
tier 1 now that github actions exposes native arm64 macOS workers. This
commit updates the documentation accordingly. This should not be merged
until Qiskit 1.1.0 is released (~or 1.0.3 if we decided to backport
Qiskit/qiskit#12102~ this isn't an option because there are issues with
arm64 mac in numpy that need pretty extensive changes to workaround
which will only be in 1.1.0).

---------

Co-authored-by: Rebecca Dimock <[email protected]>
ElePT pushed a commit to ElePT/qiskit that referenced this pull request May 31, 2024
* Promote arm64 macOS to tier 1

Github recently added a new macOS runner that is using the m1 CPU that
is usable for open source projects. [1] Previously Qiskit had support
for arm64 macOS at tier 3 because we were only able to cross compile
for the platform and not test the binaries. Now that we can run CI jobs
on the platform we're able to run both unit tests and test our binaries
on release. This commit adds a new set of test jobs and wheel builds
that use the macos-14 runner that mirrors the existing x86_64 macOS
jobs we have. This brings arm64 macOS to the same support level as
x86_64. THe only difference here is that azure pipelines doesn't have
arm64 macOS support so the test job will run in github actions instead.

[1] https://github.blog/changelog/2024-01-30-github-actions-introducing-the-new-m1-macos-runner-available-to-open-source/

* Update test job

* Fix syntax error

* Use a string for python version in test job

* Disable fail-fast on test job

* DNM: Test wheel builds

* Skip universal builds

* Force system python to arm64 on macOS arm64 bwheel builds

* Correctly skip universal builds

* Skip python 3.8 arm64

* Add back cross build job just for python 3.8

* Add numpy env details to arm64 test job

* Use MSRV for one of the test jobs

* Fix build_pgo.sh script when running on arm64 mac

* Revert "DNM: Test wheel builds"

This reverts commit 97eaa6f.

* Rename macos arm py38 cross-build job
frankharkins pushed a commit to frankharkins/documentation that referenced this pull request Jul 22, 2024
In Qiskit/qiskit#12102 we're promoting the macOS arm64 environment to
tier 1 now that github actions exposes native arm64 macOS workers. This
commit updates the documentation accordingly. This should not be merged
until Qiskit 1.1.0 is released (~or 1.0.3 if we decided to backport
Qiskit/qiskit#12102~ this isn't an option because there are issues with
arm64 mac in numpy that need pretty extensive changes to workaround
which will only be in 1.1.0).

---------

Co-authored-by: Rebecca Dimock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog type: qa Issues and PRs that relate to testing and code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants