-
Notifications
You must be signed in to change notification settings - Fork 39
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
Fix numpy 2.0 compatibility #319
Conversation
@jswhit I've signed the CLA. If you could enable CI then I can know that CI fails with numpy 2.0 and no other changes. I'll then work on fixing the issues. |
The CI tests did not fail - so I guess we don't have a numpy 2.0 compatibility problem after all? |
No actually it looks like only your "tests_conda.yml" is properly configured to run on pull requests. cibuildwheel, tests_latest (what I modified), and publish are all configured to seemingly run on pull requests but didn't run. Any ideas? |
Ah this says the workflow was disabled manually: https://github.com/Unidata/cftime/actions/workflows/tests_latest.yml If the latest python (3.13 from the YAML) never passes for cftime can I request that I/you/we turn it into a unstable tests workflow? Or actually I could just add an unstable dependency install to the |
I updated the tests_conda workflow to do what I mentoned. Another thing I could do is have wheel building use numpy 2 so the wheels that are built are compatible with numpy 1 and 2. |
Ok somehow a dumb hyphen got added to the YAML. Hopefully it will work this time. |
Ok, great. CI is failing now. Let's see if I can fix it (should be simple).
|
Would you be against me adding a github dependabot config in |
I've just realized I was wrong about the available number nightly versions. The advice here (https://numpy.org/devdocs/dev/depending_on_numpy.html#adding-a-dependency-on-numpy) is to wait until numpy 2 rc1 is out if you have a Cython/C extension. At that point you'll know that the C ABI is not changing. That rc1 is taking longer than expected to get released: I've already released packages with the newest numpy nightly so I know they're compatible with numpy 1, but I would understand if you wanted to wait to build with numpy 2 for this project. Regardless, testing is looking good against numpy 2, but a lot of the github actions are old and need to be updated. I can help with that if a cftime maintainer gives me the 👍. |
Ah in my own fork I see why the "latest" was turned off. Numpy isn't building wheels for unreleased versions of python: https://github.com/djhoese/cftime/actions/runs/7933180535/job/21661356472 So maybe that workflow should be removed? |
Thoughts on the updates here? It'd be great to get a new version out with these changes/fixes. |
I rebased with master. If you could approve the CI @jswhit then I can get back to updating the builds for numpy 2.0rc1 which was released a couple weeks ago. |
how about using the approach in https://github.com/Unidata/netcdf4-python/pull/1317/files? (still use oldest-supported-numpy for python < 3.9) |
@jswhit Do you have a need for 3.8 support still? According to https://scientific-python.org/specs/spec-0000/ 3.9 is already dropped by many scientific Python packages. Not a huge deal for keeping 3.8 but I wanted to make sure. |
Yes, I would like to keep 3.8 support. |
just one workflow failing now - the 3.8 wheel build on ubuntu. |
It looks like something called nh3 is looking for the rust compiler. I have no idea what that is. I can do some research later maybe but if you have an idea, let me know. |
I have no idea why the wheel building fails for python 3.8 - could be unrelated to this PR. Googling it turns up many suggestions (upgrade pip, switch from 32 bit to 64 bit python,...) but nothing definitive. I don't think we really need a rust compiler. Maybe skip musllinux wheels with
?? |
@jswhit I could do that, but I wanted to run something by you. I had never heard of
That sounds unnecessary for building the |
Oops I didn't realize setup.cfg had pytest-cov options specified. I re-added that dependency in the wheel test phase. |
Looks good @djhoese, thanks. Merging now... |
Closes #318