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

CI: Build Linux AArch64 wheels natively, and add Arm runners for testing #788

Merged
merged 6 commits into from
Jan 29, 2025

Conversation

agriyakhetarpal
Copy link
Collaborator

Description

This PR switches the Linux arm64/aarch64 wheels to build natively on ubuntu-22.04-arm instead of using emulation.

(Hopefully?) fixes #787

Additional context

@agriyakhetarpal
Copy link
Collaborator Author

For some reason, I can't trigger https://github.com/PyWavelets/pywt/actions/workflows/wheel_tests_and_release.yml. I do remember that those with triage permissions were able to do that at some point in the past, perhaps GitHub changed that... could you please trigger the workflow, @rgommers?

@agriyakhetarpal agriyakhetarpal added maintenance CI Continuous integration labels Jan 28, 2025
@agriyakhetarpal agriyakhetarpal added this to the v1.9.0 milestone Jan 28, 2025
@agriyakhetarpal
Copy link
Collaborator Author

In the meantime, now that the runners are available, I can also expand the changes to tests.yml.

@agriyakhetarpal
Copy link
Collaborator Author

Perhaps I spoke too soon 😄 as the Linux job has vanished despite having perfectly fine syntax (I tried both ubuntu-22.04-arm both with and without the double quotes):

This screenshot shows the Tests workflow for PyWavelets failing because of an error while evaluating the runs-on parameter for the test_pywavelets_linux job. It complains of an unexpected value '' in Line 31, column 14 for .github/workflows/tests.yml

@agriyakhetarpal agriyakhetarpal marked this pull request as draft January 28, 2025 21:50
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks! Switching to native runners looks like a good idea. The change in the wheels job looks right. The changes in tests.yml don't quite look right (aside from the possible syntax issue & disappearing job), because we don't want to double the number of jobs. Instead, can you please change 1 job over and leave the rest unchanged?

@agriyakhetarpal
Copy link
Collaborator Author

Thanks, @rgommers! I fixed the disappearing jobs; the error message was a bit cryptic.

because we don't want to double the number of jobs. Instead, can you please change 1 job over and leave the rest unchanged?

With 3a1ffa8, we're running only the "default" set of tests for Linux arm64, while Linux amd64 will run the "with-scipy", "minimum-req", "refguide-check", and "pre-releases" jobs in addition to the "default" set. Is this what you had in mind?

@agriyakhetarpal agriyakhetarpal changed the title CI: Build Linux AArch64 wheels natively CI: Build Linux AArch64 wheels natively, and add Arm runners for testing Jan 29, 2025
@agriyakhetarpal agriyakhetarpal marked this pull request as ready for review January 29, 2025 13:26
@rgommers
Copy link
Member

Is this what you had in mind?

Closer, but not quite. On this PR I now see these 6 jobs:

image

That's pretty wasteful, there is no need for iterating over all Python versions already, let alone twice. I'd be fine with either 2 or 3 jobs there: we need to cover minimum/maximum Python version, and amd64/aarch64. So two jobs in total would cover that.

While you're at it, perhaps bump the maximum from 3.12 to 3.13?

@agriyakhetarpal
Copy link
Collaborator Author

Done; we now have two jobs with Python 3.10 and Python 3.13 for Linux amd64 (plus the extra jobs with options), and one job with Python 3.13 for Linux arm64.

@agriyakhetarpal
Copy link
Collaborator Author

0832609 does the same for macOS, bumping to Python 3.13. I notice we're not testing Intel macOS runners. Should we add them, or leave them out?

@agriyakhetarpal
Copy link
Collaborator Author

The segfault is unrelated and tracked via #782 already.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM now, let's give this a go (wheels get built on merge to main). Thanks Agriya!

@rgommers rgommers merged commit 93babb4 into PyWavelets:main Jan 29, 2025
17 checks passed
@agriyakhetarpal agriyakhetarpal deleted the try/linux-arm-builds branch January 29, 2025 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some Linux aarch64 builds are failing on main
2 participants