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

BLD: stop skipping musl wheel builds #26443

Merged
merged 4 commits into from
Aug 9, 2023

Conversation

tacaswell
Copy link
Member

closes #26438

I think that this is a generically bad idea (see https://pypackaging-native.github.io/meta-topics/user_expectations_wheels/ for longer comments on this), but given the currently low cost of enabling this there is no good reason to not do this.

I opened the PR so later when I say it was a bad idea at least I am to blame for it ;)

@tacaswell tacaswell added this to the v3.8.0 milestone Aug 3, 2023
@tacaswell tacaswell added the CI: Run cibuildwheel Run wheel building tests on a PR label Aug 3, 2023
@tacaswell
Copy link
Member Author

opened scipy/oldest-supported-numpy#78 upstream to oldest-supported-numpy

pyproject.toml Outdated
Comment on lines 5 to 6
"oldest-supported-numpy; platform_python_implementation!='PyPy'",
"numpy>=1.25; platform_python_implementation=='PyPy'",
Copy link
Member

Choose a reason for hiding this comment

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

My current understanding is that we should actually just be able to do numpy>=1.25 independent of platform since numpy/numpy#23528

Copy link
Member Author

Choose a reason for hiding this comment

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

Is our set of supported Pythons a subset of the versions that numpy supports in 1.25?

Copy link
Member Author

Choose a reason for hiding this comment

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

answering my own question: yes

I'll do that too.

Copy link
Member

Choose a reason for hiding this comment

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

It defaults to going to 1.17, which is perhaps a bit too far back... but also not really a harm in that, unless we actually know we need something newer? (At which point adding a #define can change the version, just not sure where the most sensible place for that define is)

Copy link
Member

Choose a reason for hiding this comment

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

Specifically, it allows you to put in the C code a line like

#define NPY_TARGET_VERSION NPY_1_22_API_VERSION

To control the python version. the default is NPY_1_17_API_VERSION

If we use features more recent (and the macro is not defined by e.g. pybind11/cibuildwheel already), we may wish to put the line specify 1.22 somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should do that in a follow-up PR.

If it defaults to an API version older than we actually support then we are sure it will work :)

As of numpy 1.25.0 we no longer need to pin to the oldest supported versions to
make wheels work reliably.
@ksunden
Copy link
Member

ksunden commented Aug 3, 2023

Got slightly worried that this change would e.g. install numpy 1.25 over the minimum supported version, but looks like we are good on that front (it is installed with --no-deps --no-build-isolation, I think the --no-deps saves it from upping numpy)

So LGTM

@oscargus
Copy link
Member

oscargus commented Aug 4, 2023

This should also be updated, right?
https://matplotlib.org/devdocs/devel/dependencies.html#dependencies-for-building-matplotlib

(Maybe a release note as well?)

Edit: I also note that pybind11 and setuptools are missing there... Added in #26448

@tacaswell
Copy link
Member Author

I don't think that we need to update any of our docs for this change. We will build with any version of numpy that we support, however because of the way that the pip/wheels ecosystem pushes the responsibility for building re-distributable wheels out to the projects and because wheels do not have enough meta-data to express their (set and build time) ABI dependency on numpy, we have to build the wheels with a version of numpy that will result in binaries that will work with any numpy that a user might have installed.

Before numpy 1.25 this was done via the oldest-supported-numpy meta-package that is an empty package that hard-pins to the right version of numpy for any given (python implementation, platform, python version). After numpy 1.25 numpy has made changes such that they are both backwards and forwards compatible so if we build everything with numpy 1.25 then the wheels will work with any version of numpy and as all of the Python's we support are supported by numpy 1.25 we can simplify our lives and pin that in the pyproject.toml build requirements.

I do non view that as an actual "build requirement" any more than what ever pins fedora / debian / conda / arch / ... set as part of their build systems. That we have to set this here is just an artifact the the pip/wheel ecosystem expects to be able to leak its internal details back into every single project.

@ksunden
Copy link
Member

ksunden commented Aug 4, 2023

Note that the minimum versions CI does build with our minimum supported numpy (the build dep pins are not checked with no build isolation) That was one of the things I was careful to check in this review.

So these pins only actually apply for build-isolation (which is the default) builds, which will be installing a new numpy anyway, so any effect is minimal if present.

@ksunden ksunden added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Aug 5, 2023
@ksunden
Copy link
Member

ksunden commented Aug 5, 2023

Marking as release critical because pypy builds are failing on main

@QuLogic
Copy link
Member

QuLogic commented Aug 5, 2023

I don't think that we need to update any of our docs for this change.

New wheels are generally documented in development API changes for a release.

@tacaswell
Copy link
Member Author

New wheels are generally documented in development API changes for a release.

I missed we did that 🐑 , pushed docs.

@ksunden
Copy link
Member

ksunden commented Aug 7, 2023

The ARM builds are taking extra long here... perhaps we should remove musl on arm, since that is a bottleneck?

Numpy does not supply MUSL arm builds, which explains the length extension on the cross platform builds.

@ksunden ksunden merged commit 412dced into matplotlib:main Aug 9, 2023
@tacaswell tacaswell deleted the bld/unskip_musl branch August 9, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Run cibuildwheel Run wheel building tests on a PR 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.

[ENH]: musllinux wheels for Alpine
4 participants