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.

Sign up for GitHub

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

Jump to bottom

Use native aarch64 GitHub Action workers #1371

Merged
merged 4 commits into from
Feb 3, 2025
Merged

Use native aarch64 GitHub Action workers #1371

merged 4 commits into from
Feb 3, 2025

Conversation

IvanIsCoding
Copy link
Collaborator

Closes #1367

In hindsight we should have used this before the release as the QEMU jobs are really unstable. I might need to backport this to the 0.16 branch and trigger a workflow again.

This is a copy of Qiskit/qiskit#13682. I will test it in my branch and check it works.

@IvanIsCoding IvanIsCoding added this to the 0.17.0 milestone Jan 24, 2025
@IvanIsCoding
Copy link
Collaborator Author

IvanIsCoding commented Jan 24, 2025 •
edited
Loading

Test run: https://github.com/IvanIsCoding/rustworkx/actions/runs/12943314687

edit: there are still some warnings like

WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested

Most likely we should make the aarch64 steps now wait with the other steps to build and publish

@IvanIsCoding IvanIsCoding changed the title Use native aarch64 GitHub Action works Use native aarch64 GitHub Action workers Jan 24, 2025
@coveralls
Copy link

coveralls commented Jan 24, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 13103659085

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.819%
Totals Coverage Status
Change from base Build 13103624632: 0.0%
Covered Lines: 18381
Relevant Lines: 19183
💛 - Coveralls

@IvanIsCoding IvanIsCoding mentioned this pull request Jan 24, 2025
Closed
@mtreinish
Copy link
Member

WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested

Hmm, that makes me think we have a configuration issue that's explicitly setting the image for aarch64 to x86_64. But that's a bit odd, because I think we only set an image explicitly on x86_64.

@IvanIsCoding IvanIsCoding mentioned this pull request Jan 24, 2025
Closed
@IvanIsCoding
Copy link
Collaborator Author

WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested

Hmm, that makes me think we have a configuration issue that's explicitly setting the image for aarch64 to x86_64. But that's a bit odd, because I think we only set an image explicitly on x86_64.

This will go away when I refactor this PR.

Right now, the aarch64 build is faster than the default build because i686 spends a lot of time compiling NumPy. So there could be a window of time where there could be no source/pre-compiled wheels for new platforms which is chaos.

I will merge musl and manylinux for aarch64 and put them with the main infrastructure now.

@IvanIsCoding
Copy link
Collaborator Author

IvanIsCoding commented Jan 26, 2025 •
edited
Loading

Simplified test build is running: https://github.com/IvanIsCoding/rustworkx/actions/runs/12971477288

I think we'll be able to just add aarch64 to the OS matrix. That would simplify our setup quite a lot

edit: the wheels got built, this is very convenient!

mtreinish
mtreinish reviewed Feb 2, 2025
View reviewed changes
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This lgtm, but do you want to promote it to tier 1 and add a test job to our ci test matrix?

@IvanIsCoding
Copy link
Collaborator Author

IvanIsCoding commented Feb 2, 2025 •
edited
Loading

This lgtm, but do you want to promote it to tier 1 and add a test job to our ci test matrix?

Let's merge this first because the release is kind of broken right now.

I will defer the tiers for later, but I think we need to some cleanups:

  • win32 got removed from Qiskit and I think it should be removed from rustworkx
  • aarch64 manylinux could be promoted to tier 1
  • aarch64 musllinux could be promoted to tier 2 to include tests
  • i686 was the slowest step in the release last time, I am inclined not to downgrade the tier as NumPy does not release binaries for it anymore

edit: I am tracking the work in #1378

@IvanIsCoding IvanIsCoding mentioned this pull request Feb 2, 2025
Open
@mtreinish mtreinish enabled auto-merge February 2, 2025 23:58
@mtreinish
Copy link
Member

mtreinish commented Feb 3, 2025 •
edited
Loading

Fwiw in qiskit 2.0 we're dropping all 32 bit platform support. I'm not sure we need to necessarily follow what qiskit does here. We can check the download statistics for different platforms and make the call based on that.

@IvanIsCoding
Copy link
Collaborator Author

I think that is fair. Let’s check download statistics.

My main concern is 32-bit builds breaking the build because of NumPy not compiling. Maybe we could downgrade i686/win32 to Tier 4

@mtreinish mtreinish added this pull request to the merge queue Feb 3, 2025
Merged via the queue into Qiskit:main with commit f2f4542 Feb 3, 2025
31 of 32 checks passed
@IvanIsCoding
Copy link
Collaborator Author

@Mergifyio backport stable/0.16

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 3, 2025 •
edited
Loading

backport stable/0.16

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Feb 3, 2025
@IvanIsCoding @mergify
Use native aarch64 GitHub Action workers (#1371)
28ddb0d 
* Use native aarch64 GitHub Action works

* Simplify release structure

(cherry picked from commit f2f4542)
@mergify mergify bot mentioned this pull request Feb 3, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtreinish mtreinish mtreinish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.17.0
Development

Successfully merging this pull request may close these issues.

Use Linux arm64 runners for the release process
3 participants
@IvanIsCoding @coveralls @mtreinish