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.

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

@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.

Copy link

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
nrnhines added a commit that referenced this pull request Jan 23, 2025
nrnhines added a commit that referenced this pull request Jan 23, 2025
JCGoran added a commit that referenced this pull request Jan 24, 2025
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