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

Update Numpy and Cython for Python 3.10 and 3.11 builds #152

Conversation

OCopping
Copy link
Contributor

This PR sets the wheel builds for Python 3.10 and 3.11 to use Numpy 2.0.1

@OCopping OCopping requested review from AlexanderWells-diamond and mdavidsaver and removed request for AlexanderWells-diamond August 14, 2024 14:48
@mdavidsaver
Copy link
Member

With reference to the discussion starting here #145 (comment)

I think some changes and additions will be necessary.

  1. In setup.py, conditional computing of install_requires to use the existing "assume ABI forward compatibility ..." computed range when building against numpy 1.x, and numpy >= 1.7 when building against 2.x.
  2. In src/p4p.h, define NPY_NO_DEPRECATED_API unconditionally, and add a definition of NPY_TARGET_VERSION to 1.7.
  3. In pyproject.toml, I think the numpy dependency would simply be numpy >= 1.7. The specific versions in .github/workflows/build.yml should take precedence for pypi.org builds, and would let users continue to build from source against numpy 1.x .

This would clearly state that numpy 1.7 is the minimum support by P4P.

eg. when building against numpy 1.10.0, the install_requires would end up as: ["numpy >= 1.10.0", "numpy < 2"].

eg. when building against numpy 2.0.0, the install_requires would end up as: ["numpy >= 1.7", "numpy < 3"].

As mentioned at the bottom at some point it would be good to test the backwards compatibility at the end of a 2.x build by rolling back numpy and re-running the unit tests. I don't see this as a blocker for this PR.

@OCopping OCopping force-pushed the bump-numpy-versions-for-python-builds branch from 47ef0b1 to 1b0e5e8 Compare August 15, 2024 07:37
@OCopping
Copy link
Contributor Author

  1. In src/p4p.h, define NPY_NO_DEPRECATED_API unconditionally, and add a definition of NPY_TARGET_VERSION to 1.7.

It seems that removing the condition for this breaks the builds...
#if CYTHON_HEX_VERSION>=0x03000000

@OCopping OCopping force-pushed the bump-numpy-versions-for-python-builds branch from 1b0e5e8 to d99a8df Compare August 15, 2024 08:22
@OCopping OCopping force-pushed the bump-numpy-versions-for-python-builds branch from d99a8df to 47e04e5 Compare August 15, 2024 09:17
@OCopping
Copy link
Contributor Author

OCopping commented Aug 15, 2024

Also NumpyVersion was only added to numpy in version 1.9, so we would either have to try/except around the call, or set the floor to 1.9 if we were to stick with the approach

@mdavidsaver
Copy link
Member

It seems that removing the condition for this breaks the builds...
#if CYTHON_HEX_VERSION>=0x03000000

Replacing NPY_CARRAY_RO with NPY_ARRAY_CARRAY_RO resolves this for me.

@mdavidsaver
Copy link
Member

mdavidsaver commented Aug 16, 2024

Also NumpyVersion was only added to numpy in version 1.9, ...

A negative hasattr() test for NumpyVersion seems like a solid < 2.0 indicator.

Even with these changes, I am still seeing a couple of build failures.

One appears to be a bug in numpy 2.0.1 building with MSVC. fatal error C1021: invalid preprocessor command 'warning' in numpyconfig.h. This is legit as # warning ... is a GNU extension. Interesting that their CI didn't catch this...

https://github.com/numpy/numpy/blob/282c79ebc9b61ebc5f0755736ec953b8d954922c/numpy/_core/include/numpy/numpyconfig.h#L133

The Linux py3.9 manylinux2010_x86_64 job does not find a numpy 2.0.1. Indeed there is no manylinux wheel for 3.9

ERROR: Could not find a version that satisfies the requirement numpy==2.0.1 (from versions: 1.19.3, 1.19.4, 1.19.5, 1.20.0, 1.20.1, 1.20.2, 1.20.3, 1.21.0, 1.21.1, 1.21.2, 1.21.3, 1.21.4, 1.21.5, 1.21.6)

The OSX 3.7 build fails with what I hope is a caching glitch.

Downloading ply-3.11-py2.py3-none-any.whl (49 kB)
        Expected sha256 80a41edf64a3626e729a62df7dd278474fc1726836552b67a8c6396fd7e86760
             Got        8f1d6e470697953dcaad90e3922e9b9b543e1754524552b01ad6b394b6e134e6

@mdavidsaver
Copy link
Member

appears to be a bug in numpy 2.0.1 building with MSVC

numpy/numpy#27224

@OCopping
Copy link
Contributor Author

The Linux py3.9 manylinux2010_x86_64 job does not find a numpy 2.0.1.

I have a fork with a branch looking at updating supported versions and images (including arguing for the removal of Python 2.7 and 3.5 support), and using manylinux_2_28_x86_64 has Numpy 2.0 for Python 3.9

OCopping#3

@OCopping
Copy link
Contributor Author

@mdavidsaver are you happy to merge this PR in so we can make another beta release to allow people to test their builds with Python 3.10 and 3.11? We can then merge in the Numpy fix for Windows builds in another PR?

@mdavidsaver mdavidsaver force-pushed the bump-numpy-versions-for-python-builds branch from 6a255fa to 03a1308 Compare August 22, 2024 04:30
@mdavidsaver
Copy link
Member

I made another iteration to pin the windows builds to numpy 1.x until the #warning issue can be addressed. Well, all but the py3.12 windows job. Apparently that #warning was added earlier than I expected, effecting all numpy wheels uploaded for py3.12. So I think that specific combination will have to wait for a fix to numpy.

@AlexanderWells-diamond
Copy link
Contributor

@mdavidsaver Just to confirm, do you want to wait for a Numpy release that fixes that particular issue, or are you happy if we try and work around it?

@mdavidsaver
Copy link
Member

At this point the numpy fix for MSVC is merged, but not released. I have re-triggered all jobs to see if they still pass. If so, then I think I am ok with disabling/ignoring the one failing job until the next numpy release.

@mdavidsaver
Copy link
Member

Merged as of b1052f4 .

@mdavidsaver
Copy link
Member

We can then merge in the Numpy fix for Windows builds in another PR?

fyi. It looks like this fix appears in numpy >= 2.1.3.

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.

3 participants