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

Support numpy>=2 and remove numpy<2 pin #3040

Merged
merged 12 commits into from
Aug 29, 2024
Merged

Support numpy>=2 and remove numpy<2 pin #3040

merged 12 commits into from
Aug 29, 2024

Conversation

kbvw
Copy link
Contributor

@kbvw kbvw commented Aug 13, 2024

No description provided.

@bbpbuildbot

This comment has been minimized.

ci/win_test_installer.cmd Outdated Show resolved Hide resolved
packaging/python/test_wheels.sh Outdated Show resolved Hide resolved
Copy link

✔️ 8904530 -> Azure artifacts URL

@kbvw kbvw changed the title Support numpy>=2 and remove numpy<2 pin Support numpy>=2 and remove numpy<2 pin Aug 13, 2024
Copy link

✔️ 5ba690e -> Azure artifacts URL

@kbvw kbvw force-pushed the numpy2 branch 4 times, most recently from 4d2bfd8 to fb0b38c Compare August 13, 2024 15:25
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.26%. Comparing base (c918f6b) to head (9fb93ec).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3040      +/-   ##
==========================================
- Coverage   67.27%   67.26%   -0.01%     
==========================================
  Files         571      571              
  Lines      104882   104887       +5     
==========================================
- Hits        70555    70550       -5     
- Misses      34327    34337      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@kbvw kbvw force-pushed the numpy2 branch 2 times, most recently from ef44b28 to af837ba Compare August 14, 2024 17:52
@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ 24e493b -> Azure artifacts URL

Copy link

✔️ d30ad08 -> Azure artifacts URL

Copy link

✔️ 16d874e -> Azure artifacts URL

Copy link

✔️ 95b461c -> Azure artifacts URL

Copy link

✔️ 7b9e1be -> Azure artifacts URL

@kbvw
Copy link
Contributor Author

kbvw commented Aug 22, 2024

Wheels are now built with the latest Numpy and tested with the oldest Numpy that supports the corresponding Python version. A few notes:

  1. When testing with different older Numpy versions, Python 3.8 kept failing over many runs of the CI. Since Python 3.8 is reaching its EOL very soon (and Numpy 2 does not support Python 3.8), I decided not to bother. It is still supported and tested, just with the same Numpy as it was built with (1.24 if I'm not mistaken) instead of the oldest Numpy that still supports Python 3.8.
  2. For Windows, while debugging, it was straightforward to run these (quick) tests once with the Numpy it was built with, and then again with the oldest supported Numpy. I'm wondering if I should do the same for the Linux/Mac wheels, i.e. test before and after installing the older Numpy here. But the test_wheel function there does a lot of other tests that might take quite long and I'm not sure we want to do them twice for every wheel. What do you think? Alternatively, I can throw things around in that script to do neuron.test() and neuron.test_rxd() twice as in Windows, and do the other tests only once, either with latest or oldest Numpy.

@1uc
Copy link
Collaborator

1uc commented Aug 22, 2024

might take quite long and I'm not sure we want to do them twice for every wheel

The "might take" is measurable. How long do they take? How long is that compared to building NEURON?

@mgeplf
Copy link
Collaborator

mgeplf commented Aug 22, 2024

@kbvw > Python 3.8 kept failing over many runs of the CI.

Can you point me to a couple of those? I'm interested to see how it failed.

@bbpbuildbot

This comment has been minimized.

@matz-e matz-e requested a review from ramcdougal August 22, 2024 08:05
@kbvw
Copy link
Contributor Author

kbvw commented Aug 23, 2024

@kbvw > Python 3.8 kept failing over many runs of the CI.

Can you point me to a couple of those? I'm interested to see how it failed.

They're a bit hard to find back because I force-pushed to the branch so often while debugging, but here is one example. This particular time, it was built with Numpy 1.24.4 and the RxD test fails with 1.20.3, with this:

'from neuron import rxd' failed numpy.ndarray size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject

This was one in a range of runs where every time, it was built with 1.24.4 and then I tested with 1.17 (oldest Numpy that supports Python 3.8), 1.18, 1.19, etc. If I recall correctly it was still failing at 1.21, and as it was approaching the version it was built with, I decided I might as well stop. I did not investigate much, as Python 3.8 reaches EOL in October, and it's still built and tested, just with the same Numpy version it was built with. If it's important I can try to get to the bottom of it.

To be clear about what changed from before: we used to build the wheels with the oldest supported Numpy, and then test with the latest. As of Numpy 2, as you can read here, wheels built with 1.x will not run with 2.x, but wheels built with 2.x will run with an older 1.x. So in this PR, all the wheels are built with the latest Numpy and tested with the oldest supported Numpy. From Python 3.9, the latest Numpy is 2.x, and testing with the old 1.x versions seems to work fine. It is just Python 3.8 where we now have a reversed scenario from before. (It gets built with 1.24.4 instead of 1.17.3.)

@mgeplf
Copy link
Collaborator

mgeplf commented Aug 23, 2024

@kbvw > 'from neuron import rxd' failed numpy.ndarray size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject

Neat, thanks for the answer. That's what I was wondering. I remember running into similar problems in going from 1.19 to 1.20, since the size of ndarray changed.

I'm wondering if the build system is pickup various numpy headers instead of the ones that are specified?

As of Numpy 2, as you can read here, wheels built with 1.x will not run with 2.x, but wheels built with 2.x will run with an older 1.x.

That's very interesting. I wonder how they pulled that trick off, as even in the 1.xx days they couldn't guarantee backwards compat. The docs suggest it's for all the 1.xx series, but I wonder how they do that. Not important for this PR, just another thing I'd like to know.

Copy link

✔️ b624267 -> Azure artifacts URL

@kbvw
Copy link
Contributor Author

kbvw commented Aug 23, 2024

The "might take" is measurable. How long do they take? How long is that compared to building NEURON?

From the latest CI run, over the range of Linux and Mac Python wheels:

13m43s to 20m17s for the build script
3m49s to 4m44s for the test script
1m44s to 1m57s for the part that now repeats for two NumPy versions

So overal about a 9-11% increase.

Quickly testing with latest and oldest NumPy seems useful (especially instead of only oldest), but not sure if everything needs to be tested twice: MPI, nrnivmodl, etc. Maybe I'll re-arrange the script and just do neuron.test() and neuron.test_rxd() twice?

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ a665da1 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@kbvw
Copy link
Contributor Author

kbvw commented Aug 27, 2024

When running only neuron.test() and neuron.test_rxd() again with the oldest NumPy, this only takes about 4-6 seconds extra.

docs/install/install_instructions.md Outdated Show resolved Hide resolved
ci/win_test_installer.cmd Outdated Show resolved Hide resolved
packaging/python/test_requirements.txt Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Aug 28, 2024

Copy link

✔️ 9fb93ec -> Azure artifacts URL

Copy link
Collaborator

@1uc 1uc left a comment

Choose a reason for hiding this comment

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

Nice work.

@1uc 1uc merged commit 2996006 into master Aug 29, 2024
38 checks passed
@1uc 1uc deleted the numpy2 branch August 29, 2024 08:38
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.

6 participants