Skip to content

Build aarch64 wheels #19402

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

Merged
merged 5 commits into from
Mar 10, 2021
Merged

Build aarch64 wheels #19402

merged 5 commits into from
Mar 10, 2021

Conversation

janaknat
Copy link
Contributor

From v1.8.0, cibuildwheel allows to build non-native architectures using CIBW_ARCH_LINUX. See https://cibuildwheel.readthedocs.io/en/stable/options/#archs for more details.

Also, use numpy v1.19. aarch64 wheels are available for it. Cuts down on time spent building aarch64 numpy wheels under emulation.

PR Summary

Use cibuildwheel option to build aarch64 wheels.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@tacaswell
Copy link
Member

Please do not bump the numpy version, we still support back to np1.16

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@janaknat
Copy link
Contributor Author

I've updated to use numpy==1.16 for x86 and numpy==1.19 for aarch64 wheels. Will that work?

@ksunden
Copy link
Member

ksunden commented Jan 30, 2021

Can't you just cache the numpy wheels? That way you compute it once but not for future ones, but still have compatibility for all supported numpy versions.

@dstansby dstansby added the Build label Jan 30, 2021
@janaknat
Copy link
Contributor Author

janaknat commented Feb 1, 2021

@ksunden I haven't used cache with Github Actions and cibuildwheel. Most examples I see are for when the dependency has its own step. With cibuildwheel, the dependency is with CIBW_BEFORE_BUILD. Not sure how to cache that. Any suggestions?

@QuLogic QuLogic added this to the v3.4.0 milestone Feb 1, 2021
@ksunden
Copy link
Member

ksunden commented Feb 3, 2021

@janaknat (First of all a disclaimer that I am no expert on cibw)

You can have a build numpy wheel stage that runs first and just populates a cache using pip wheel numpy==1.16.0 (if invalid, otherwise just finish immediately)
(See https://github.com/actions/cache, https://docs.github.com/en/actions/guides/caching-dependencies-to-speed-up-workflows#using-the-cache-action)

The install step for numpy is a CIBW_BEFORE_SCRIPT that just calls into pip. Instead of going out to pypi, it can just install the wheel directly from the cache.

There may need to be some additional variables to build up the path for the wheel, but nothing too crazy.

Alternatively it may "just work" to just add ~/.cache/pip to the github actions cache, no need to introduce additional steps or explicitly cache the wheel separately... I'd have to play with that a bit, but it should work.

@QuLogic
Copy link
Member

QuLogic commented Feb 4, 2021

This doesn't really need to go in the rc, but would be nice to have by the time 3.4 is finished.

@QuLogic QuLogic added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 4, 2021
@janaknat
Copy link
Contributor Author

janaknat commented Feb 5, 2021

@ksunden I tried with caching ~/.cache/pip.

Build log: https://github.com/janaknat/matplotlib/runs/1834849484?check_suite_focus=true
Caching: https://github.com/janaknat/matplotlib/blob/7a04819ff0d81d2ef8b9a509f21f781be387c563/.github/workflows/cibuildwheel.yml#L40

Numpy is still getting built. These dependencies are being built inside the manylinux2014-aarch64 container under QEMU as part of cibuildwheel's operation. Not sure if numpy can be cached outside the container with a separate step since that'll pull in the x86 numpy wheel.

@ksunden
Copy link
Member

ksunden commented Feb 5, 2021

For a more finegrained level of control however, the root of the host file system is mounted as /host, allowing for example to access shared files, caches, etc. on the host file system.

from https://cibuildwheel.readthedocs.io/en/stable/faq/

So you can just copy the wheel file onto the host in a cached folder, then copy it back in the before script, if it exists/

@ksunden
Copy link
Member

ksunden commented Feb 5, 2021

Another potential option: making a docker image with numpy preinstalled, I see that cibuildwheel has env vars to specify the docker image (on an architecture specific basis) this would require an outside maintenance step though, so not sure its the best solution long term.

@henryiii
Copy link

henryiii commented Feb 8, 2021

Wouldn't it be simpler/easier to just use this in the pyproject.toml (or as a manual pip install in CIBW_BEFORE_BUILD/ALL, if you really have to): https://github.com/scipy/oldest-supported-numpy ?

I see there's no pyproject.toml. :'( Okay, before build it is.

@henryiii
Copy link

henryiii commented Feb 8, 2021

Also, NumPy dropped manylinux1 wheels for 3.9. With an older pinned NumPy, it should be fine, but something to keep in mind.

@janaknat
Copy link
Contributor Author

janaknat commented Feb 9, 2021

@henryiii I've made the changes. oldest-supported-numpy defaults to 1.14.3 for python 3.7 (x86/i386) and 1.17.3 for python 3.8 (x86/i386). aarch64 goes with 1.19.2.

cibuildwheel allows to build non-native architectures using CIBW_ARCHS.
@janaknat
Copy link
Contributor Author

@ksunden I've added a step to build and stash numpy=1.16 aarch64 wheels.

wget https://files.pythonhosted.org/packages/04/b6/d7faa70a3e3eac39f943cc6a6a64ce378259677de516bd899dd9eb8f9b32/numpy-1.16.0.zip
unzip numpy-1.16.0.zip
cd numpy-1.16.0
python -m cibuildwheel --output-dir ../numpy-aarch64-cache
Copy link
Member

Choose a reason for hiding this comment

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

can we make this a hidden directory? I assume that the folder needs to exist in the repo for the caching to work correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I prefer to have the wheels stored in a separate directory so as to not confuse pip and cibuildwheel when searching for wheels to install.

Copy link
Member

Choose a reason for hiding this comment

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

That seems reasonable, but I do not want to clutter the top level of the repo with empty folders if we do not have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. How about under the 'requirements' directory, seeing as numpy is a requirement for matplotlib?. I can make a hidden directory there.

Copy link
Member

Choose a reason for hiding this comment

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

This is made in the build only, not checked in? It seems like build/ or dist/ would be a better location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran a few tests with cibuildwheel. There doesn't need to be any separate directory for the numpy wheels. cibuildwheel create a directory as part of its run. That should be sufficient.

@jarrodmillman
Copy link
Contributor

Please do not bump the numpy version, we still support back to np1.16

@tacaswell I am confused about what NEP 29 means (assuming Matplotlib is following NEP 29). On the lead up to NumPy 1.20, there was a debate about whether or not to support Python 3.6. During that discussion, I asked for clarification about what the purpose of NEP 29 was. I said that I thought it was a mechanism to get projects to drop support for old versions of Python (and NumPy); but that in practice it looked more like it was actually intended to keep projects from dropping versions of Python (and NumPy) too soon. In that discussion, Ralf said he thought it was intended to get projects to drop old version and not to keep them from dropping version too soon.

If it is the case the NEP 29 was intended to provide projects with a schedule for dropping old versions, shouldn't Matplotlib 3.4 drop support for NumPy 1.16 since 3.4 will be released after Jan 13, 2021?

@tacaswell
Copy link
Member

Consensus on the call is that we are going to leave the setup.py requirement, runtime checks and the tests on the 3.4.x branch at 1.16, but build the wheels with 1.17 (assuming that there is enough meta-data available in wheels for that to be possible).

@henryiii
Copy link

Won’t that end up with broken wheels if a user has NumPy 1.16, rather than upgrading NumPy or showing a proper error? Lying about requirements is dangerous?

@henryiii
Copy link

The other way round would make more sense, wouldn’t it?

@tacaswell
Copy link
Member

I agree we should not lie about versions!

Are wheels / pip smart enough to fall back to compiling from source if numpy1.16 is installed? On a bit more consideration I suspect not.

This is an annoying edge case. Because we planned to release before the deadline the code support np1.16 and is tested as such. We could bump the min version to 1.17 which would match the letter-of-law for NEP29 but there would be no other benefit (in terms of being able to have made use of new numpy features) and a cost of making it uninstallable for people on numpy 1.16 despite the code actually being able to run.

@tacaswell
Copy link
Member

Would bumping to 1.17 make getting arm wheels to build any easier?

@janaknat
Copy link
Contributor Author

Would bumping to 1.17 make getting arm wheels to build any easier?

Arm wheel support started with numpy==1.19. So, no change in build ease with numpy==1.17.

@stefanv
Copy link
Contributor

stefanv commented Feb 25, 2021

@tacaswell NEP 29 is currently self-contradicting: it tries to specify both minimum support required and minimum support allowed. The latter is what we should specify so that everyone can get in sync.

@henryiii
Copy link

henryiii commented Feb 26, 2021

My thought would be to leave this at 1.16, then bump master to 1.17 and look for things that can be simplified, etc. I think this appears to be too far along to make any changes that would actually take advantage of 1.17, so I think it should be declared this release was targeting an older date, which is why it's 1.16. Though it could be bumped to 1.17, it’s just sad that it can’t be taken advantage of. L

Quoting from https://github.com/scipy/oldest-supported-numpy#can-i-use-this-if-my-package-requires-a-recent-version-of-numpy:

In many cases, even though your package may require a version of Numpy that is more recent than the pinned versions here, this is often a runtime requirement, i.e. for running (rather than building) your package. In many cases, unless you use recent features of the Numpy C API, you will still be able to build your package with an older version of Numpy and therefore you will still be able to use oldest-supported-numpy. You can still impose a more recent Numpy requirement in install_requires.

I would still strongly recommend using oldest-supported-numpy, as they cover a lot of cases that would have to be manually listed and maintained here: https://github.com/scipy/oldest-supported-numpy/blob/master/setup.cfg

If you want to ensure a build time requirement that is higher, you can always override just a few lines of that with conditionals. Or ask for a NEP 29 version of oldest-supported-numpy. :)

The latter is what we should specify so that everyone can get in sync.

I'm not sure what this means. There is no benefit to matching minimum versions of NumPy, and all the NEP 29 libraries are not released in sync anyway, so there always will be mismatches. As long as it's not more than a version it shouldn't be a problem, I’d think.

@stefanv
Copy link
Contributor

stefanv commented Mar 2, 2021

The latter is what we should specify so that everyone can get in sync.

I'm not sure what this means. There is no benefit to matching minimum versions of NumPy, and all the NEP 29 libraries are not released in sync anyway, so there always will be mismatches. As long as it's not more than a version it shouldn't be a problem, I’d think.

The problem is that the community are hesitant to move on, so they keep supporting older versions (AKA burning developer and CI cycles). It would be good for the community to agree at a pace that all the projects will move forward at.

@henryiii
Copy link

henryiii commented Mar 2, 2021

I would repeat: As long as it's not more than a version it shouldn't be a problem, I’d think.

I think the libraries that agreed on NEP 29 have been dropping support for older versions. The only problem here is that 3.4 was supposed to come out before NumPy 1.16 was to be dropped but got delayed. I think it's quite fine just to keep 1.16 for the release, and drop 1.16 for master, where actual improvements (like __array_function__ or something) possibly could be taken advantage of. Having to change in the middle of release cycle just because it passes an arbitrary date is not reasonable.

@stefanv
Copy link
Contributor

stefanv commented Mar 2, 2021

Yes, sorry if it wasn't clear: I was arguing for improving the NEP wording to state its intent more clearly, not for changing the version of NumPy being used here.

@tacaswell
Copy link
Member

On a bit more consideration, getting things set up so that we can build the wheels with np117, but get it to fallback to the tarball when the user has np116 installed seems like it will be someplace on the a-lot-of-work to impossible scale.

Contrary to my previous comment, I think we should just keep going with np116 for mpl34. I think it is still in the spirit of NEP29 (we meant to release before the cut off, we started the final release work before the cut off, etc).

I agree with @stefanv that we should make the NEP29 wording clearer on the intent. I think the thing we should make clearer is that you projects should look at when they plan to release, look at the calendar and then make the supported version decisions very early in the development cycle (now is the right time to make those decisions for mpl35) and then stick with those versions through the release. The longer time horizon of the planning is how you get the smoothing effect on the support windows so you can avoid exactly this conversation.

QuLogic added 2 commits March 9, 2021 02:47
It must be unique across jobs, or one of the others (which are
undoubtedly faster, since they aren't building NumPy) will finish first
and fill the cache with an empty directory.
@QuLogic
Copy link
Member

QuLogic commented Mar 9, 2021

I pushed two fixes here, to move the version into a variable used more often, as I said, and to fix the cache key, as it was previously doing nothing.

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

While I have changes here, CI is somewhat, push and check if it works, so I'd like to merge this soon to test before making releases.

@tacaswell tacaswell merged commit 6fb2f7b into matplotlib:master Mar 10, 2021
@tacaswell
Copy link
Member

Thank you for all of your work and persistence on this @janaknat !

Hopefully the next release we will have aarch weels up :)

@janaknat
Copy link
Contributor Author

@tacaswell Thanks a bunch.

@janaknat
Copy link
Contributor Author

@tacaswell Any ideas on when the next release will be?

@henryiii
Copy link

Before Jan 13, 2021 last I heard. :-P

@QuLogic
Copy link
Member

QuLogic commented Mar 11, 2021

@meeseeksdev backport to v3.4.x

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 11, 2021
QuLogic added a commit that referenced this pull request Mar 11, 2021
…402-on-v3.4.x

Backport PR #19402 on branch v3.4.x (Build aarch64 wheels)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants