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: add ARM builds #189

Merged
merged 2 commits into from
Jun 9, 2021
Merged

ci: add ARM builds #189

merged 2 commits into from
Jun 9, 2021

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Jun 8, 2021

Rough idea based on #188 (comment)

Matrix element names will change a bit.

Closes #182.

@codecov
Copy link

codecov bot commented Jun 8, 2021

Codecov Report

Merging #189 (4bc8c2f) into master (c3593ec) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #189   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          213       213           
=========================================
  Hits           213       213           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 597e6ef...4bc8c2f. Read the comment docs.

@henryiii
Copy link
Contributor Author

henryiii commented Jun 8, 2021

Oops, forgot QEMU.

@henryiii
Copy link
Contributor Author

henryiii commented Jun 8, 2021

Looks like we should probably work on scikit-build's Apple Silicon support (see pypa/cibuildwheel#582 ). scikit-build/cmake-python-distributions was adapted to AS, now waiting on upstream Kitware for scikit-build/ninja-python-distributions, though we probably could move forward with scikit-build/scikit-build while waiting on that. See scikit-build/scikit-build#530 .

The Aarch64 build should be fine. (But slow, of course)

@ajfriend
Copy link
Contributor

ajfriend commented Jun 8, 2021

Awesome. Thanks!

And thanks for the pointers, I'll make a ticket to check back in to follow up on AS.

Let's land this for now (should be good to go as is, yeah?), and follow up with @odidev and #183 to explore if it makes sense to split out the Python versions on aarch64 to speed up the CI run time.

@henryiii
Copy link
Contributor Author

henryiii commented Jun 8, 2021

Yes, it's fine to go if you are happy with it. A common approach is to only build wheels in a reduced number of situations - it's not really needed to build all wheels on every PR and master if you also have tests. You can add a workflow_dispatch to be able to trigger them manually, for example. Also, does building 3.5 for Aarch64 make sense? Not sure there really are many users left on 3.5, much less Aarch64 users? Yes, I'd recommend doing, say, 3.6 and 3.7 in one job and 3.8 and 3.9 in another. You could swap the driving variable to CIBW_BUILD instead of CIBW_ARCHS, and always have the CIBW_ARCHS_LINUX set.

Copy link
Collaborator

@dfellis dfellis left a comment

Choose a reason for hiding this comment

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

Looks fine. With all of these OS-level conditional checks and the desire to parallelize this, it may make sense to just split the make_wheels job into three, one per OS.

@henryiii
Copy link
Contributor Author

henryiii commented Jun 8, 2021

I've attempted a refactor to split the Arm job up more, along with nicer names. This also will avoid uploading any wheels at all if one of the jobs breaks (which didn't happen before). You can also download and test the wheels and sdist from a build (including a PR). This reduces the conditional checks a bit - also, if it's a single parallelized job, it exits quicker if there's a failure in one of the matrix elements.

You can see the exact changes in the second commit, the first commit is the previous work squashed. Reasoning for the job design at https://scikit-hep.org/developer/gha_wheels .

@henryiii henryiii force-pushed the patch-1 branch 3 times, most recently from 78ded17 to a8aacde Compare June 8, 2021 22:49
@henryiii
Copy link
Contributor Author

henryiii commented Jun 8, 2021

Much faster. :)

@ajfriend
Copy link
Contributor

ajfriend commented Jun 9, 2021

Awesome. This is great! Thanks a lot for putting this together @henryiii!

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

Successfully merging this pull request may close these issues.

Add support to release aarch64 wheels
4 participants