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: move to cibw on GHA #961

Merged
merged 9 commits into from
Jun 28, 2021
Merged

ci: move to cibw on GHA #961

merged 9 commits into from
Jun 28, 2021

Conversation

henryiii
Copy link
Member

@henryiii henryiii commented Jun 24, 2021

This PR moves the wheel building to cibuilldwheel, and performs it on GitHub Actions. One benefit of GitHub Actions is that we can keep this up-to-date with dependabot at some point; another is that the Azure 10 build queue and the GitHub Actions 10 build queue are not synced, so these can build in parallel with Azure ;). It's the same hardware and VMs, generally.

Features:

  • Fixed cross compile support for macOS.
  • Tests are now run on the wheels. This caught some bugs (tests: fix loading from any directory #953, fix(setup): sync with cmake_example #954, Appropriate FileNotFoundError for ak.from_json. #950, fix: manylinux1 couldn't take None/newaxis #955). Currently only testing with the [tests] extra.
  • SDists are built with pypa/build.
  • Wheels can be directly downloaded from the GitHub Actions interface.
  • Removed the manylinux1 32-bit wheels, which were segfaulting. Easy to re-add if the segfaults on running tests are fixed.
  • Removed the CPython 3.10 builds - Python 3.10 will not have a stable ABI until 3.10.0b4, so PyPy wheels are not supposed to be uploaded yet. Cibuildwheel 2.0.0 alphas have 3.10 support protected by a flag CIBW_PRERELEASE_PYTHONS, so that users don't upload 3.10 wheels accidentally during the early beta period.
  • Trimmed the overlapping manylinux wheels - it's now manylinux1 for 2.7, 3.5, and 3.6, and manylinux2010 for 3.6+. This is based on the Pip version analysis (performed using Hist ;) ) in Drop support for manylinux1 images January 1st, 2022 pypa/manylinux#994 (comment) Edit: now only removed the 3.9 wheel, should cover over 99% of users (in fact, NumPy does not ship a 3.9 manylinux1 wheel anymore either).
  • Added Universal2 builds for Python 3.8 and 3.9 (TODO: I still need to test these on my Apple Silicon machine)
  • Added Arm on Linux builds for 3.6+ (TODO: Should it be 3.7+? Are there old Arm users around? Due to emulation, these are slow. Travis can be used to get a faster native build, but not fond of messing with that. Building wheels should be pretty rare anyway.)
  • Added PyPy builds for Linux (tested) and macOS (untested due to missing macOS wheels for NumPy)
  • Builds manually triggered on GitHub, or on "Release" via GitHub (can be changed). TODO: Also on PR, so that this PR can build. Can be removed when the PR is ready. Or anytime, it just has to trigger once to make the "manual run" option available in the UI.
  • PyTest defaults to printing a summary.
  • Removed almost 200 lines of code.
  • The wheels also now only upload to PyPI if all of them pass, instead of partially uploading a set of wheels if one or more break (big plus!).

The CUDA build is currently untouched, still on Azure. We will need to add a token to the secrets before deployment, as well.

Summary of todo's:

  • Test wheel on Apple Silicon
  • Drop the build-on-PR
  • What should the range of ARM builds be? 3.6+? 3.7+?
  • Should we enable manylinux1 3.7 and 3.8? Looking back at that list, there's still 7% of users with an old pip version (that doesn't officially support Python 3.7) on Python 3.7. What are we aiming for here?

@henryiii henryiii marked this pull request as draft June 24, 2021 17:12
@jpivarski
Copy link
Member

I triggered the 1.4.0rc1 release (which is huge) and it's starting on Azure. I guess the switch from Azure to GitHub Actions is here, in this unmerged PR. That's okay—I just misunderstood.

Anyway, if the support for a different set of platforms is in main (and therefore the 1.4.0rc1 that's getting built and deployed now), then it should be 1.4.0 regardless of whether it was built on GitHub Actions or Azure. The important thing for the version number is to have a clear dividing line when a different set of things is supported. if this PR is just changing where it gets built, not which systems it's deploying wheels for, then this PR does not need to have a transitional version number: it can be 1.4.1 or 1.5.0rc1.

(On the side, I wish I knew what to do about bug-fix releases, such as 1.4.1, that come after next-version prereleases, such as 1.5.0rc1. In the past, I've been interleaving them, but I think it's terrible: 1.4.0, 1.5.0rc1, 1.4.1, 1.5.0rc2, 1.4.2, ... It seems very wrong to me that the chronological and incremental increases are not monotonic with version number. At the moment, though, it doesn't look like that's what's going to happen here, but I'd like to have a plan in place, in case you have any recommendations.)

@henryiii
Copy link
Member Author

This could go into r2 / 1.4.0 final, still? This contains PyPy & Universal2 binaries that would be good to ship. Changing the set of binaries in an RC shouldn't be an issue - pip only looks at final releases in the normal solve.

Do you have answers for the final two points? (Manylinux1 for 3.7 and 3.8, where should we start with ARM). I need to test this on Apple Silicon, drop the build-on-all-PRs setting, and add a token to GitHub's Secrets.

@henryiii
Copy link
Member Author

Interleaving versions is fine - if you sort chronologically, that's just what's going to happen. PyPI sorts by version number, so it looks fine there. GitHub Releases sorts chronologically - it has to, there's no rules about versions there. You could call your releases "bill" and "bob" and it would be just as happy.

@henryiii
Copy link
Member Author

( I should point out there are no code changes at all in this PR, those have already been made. So it seems like a fine change to go into a new RC, IMO)

@henryiii
Copy link
Member Author

Looks like I need to adapt the CMake build to target Apple Silicon, the Universal2 wheel just has x86 arch in it. Working on it.

@jpivarski
Copy link
Member

This contains PyPy & Universal2 binaries that would be good to ship.

In that case, let's try to get this PR into 1.4.0rc2. I'm not at all worried about changing the set of supported releases within the release candidates, only with the final 1.4.0. If it goes in two batches (worst case), then it would be 1.5.0. Just so that there's a clear line for figuring out dependencies.

I'll be out of touch for the next day, starting right now (camping with family), so we'll get back to this on Monday.

As for your two questions,

  • What should the range of ARM builds be? 3.6+? 3.7+?

From a programming perspective, the steep threshold is between 3.5 and 3.6, largely because of the unstable dict order. From 3.6 onward, it's all the same as far as Awkward Array compatibility is concerned. If 3.6 has a lot fewer users on ARM than 3.7, that would be a reason to not bother with it (reasons from outside the codebase, rather than reasons from inside the codebase). But I can't provide guidance on that; I don't have usage statistics.

  • Should we enable manylinux1 3.7 and 3.8? Looking back at that list, there's still 7% of users with an old pip version (that doesn't officially support Python 3.7) on Python 3.7. What are we aiming for here?

"7% of users" sounds like a large fraction to me. Personally, I would cut a threshold somewhere between 0.1% and 1%. The 1-in-a thousand or 1-in-a hundred user doesn't have to be catered to (should still be possible to install, but we don't need to make special wheels for them), but 7% of users is a lot. If we have to draw a hard line, make it 1%.

@henryiii
Copy link
Member Author

Guess I'll work on this slowly, GH seems to be having networking issues. "Internal Server Error" trying to checkout awkward from GitHub. 🤣

@henryiii
Copy link
Member Author

NumPy on PyPy Linux is failing to load - maybe the NumPy PyPy wheel is not actually manylinux2010 compatible? Going to guess it's their problem, not ours, and disable testing for now. We should probably check the PyPy Linux (or even macOS) wheels manually (I'm on an AS machine currently, so no local PyPy or docker)

@henryiii
Copy link
Member Author

henryiii commented Jun 26, 2021

Tested the wheels locally on Apple Silicon, they work fine. Should we do Universal + x86, or Arm + x86? Eventually, we should get multitagging and then we can have all three tags on a single universal wheel, if we want to. But that's on my todo list.

Currently, the set of all the wheels (which can be downloaded from the PR) is about 350 MB.

@jpivarski
Copy link
Member

  • Drop the build-on-PR

Let me know when you think it's ready. (I'm not really sure what is meant by this checkbox.) I'd like to do another release candidate with this PR included to test-run the new deployment. We have all week to go through a few cycles of tests, but it should be stable by the end of the week with a non-RC release so that it's ready for new users from PyHEP. Thanks!

@henryiii
Copy link
Member Author

I'm not really sure what is meant by this checkbox

Currently this builds on all PRs, which is overkill for building wheels. Maybe building a small subset of wheels would be a good idea, though, on all PRs? I do that in boost-histogram.

I'll drop this and then it is ready. If you have a PR that changes the build and might affect wheels, you can manually trigger this from the GHA web interface on local branches.

@henryiii henryiii marked this pull request as ready for review June 28, 2021 15:46
@henryiii
Copy link
Member Author

FYI, PyPy is untested, due to NumPy not working in the "classic" pypy manylinux image, so I'd advertise it as experimental support currently. It should be easy to test locally. cibuildwheel 2.0 will support pypy in any manylinux image 2010+, so we could likely enable testing then if a newer image fixes it.

@jpivarski
Copy link
Member

jpivarski commented Jun 28, 2021

Error: Unable to resolve action actions/checkout@v2cp36-manylinux_x86_64, unable to find version v2cp36-manylinux_x86_64

I haven't been able to find any references to this string anywhere: "v2cp36-manylinux_x86_64". Is it a GitHub Action module, a Docker image, or what?

Edit: But it looks like you fixed it.

@henryiii
Copy link
Member Author

The syntax is uses: <github_repo>@<tag>, and I pasted the manylinux repo name onto it by accident. :)

@jpivarski jpivarski merged commit 45d59ef into main Jun 28, 2021
@jpivarski jpivarski deleted the henryiii/ci/cibw branch June 28, 2021 18:44
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.

2 participants