-
Notifications
You must be signed in to change notification settings - Fork 60
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
Remove NumPy <2 pin #762
Remove NumPy <2 pin #762
Conversation
Now that CuPy 13.3.0 is out. Marking this ready for review Merged in |
Looks like the wheels had two test failures in CI: FAILED python/cucim/src/cucim/skimage/color/tests/test_colorconv.py::test_gray2rgba_alpha_fail_cast[-1-uint8] - OverflowError: Python integer -1 out of bounds for uint8
FAILED python/cucim/src/cucim/skimage/color/tests/test_colorconv.py::test_gray2rgba_alpha_fail_cast[300-int8] - OverflowError: Python integer 300 out of bounds for int8 Conda doesn't have the same issue, but am seeing NumPy downgraded to there. So we are likely missing something there too |
So Conda downgrades NumPy because
Addressing either of these would fix the issue |
So this failure is new with NumPy 2.1, it seems pretty harmless, I pushed a potential fix, but would be good if @grlee77 can have a quick look if that seems good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Packaging changes look ok to me. I poked around and don't see any other uses of numpy
that were missed.
Just one question... have we double-checked that the assumptions here hold for both the NumPy 1.x and 2.x ABI?
cucim/cpp/include/cucim/memory/dlpack.h
Line 33 in de6bfbb
inline const char* to_numpy_dtype(const DLDataType& dtype) { |
Are there new-in-NumPy-2.x datatypes that need to be added there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @seberg. The compatibility change looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks all! 🙏
The changes look good here
We do need to update scikit-image, which is being done in PR: #769
So marking as requests changes and adding "DO NOT MERGE" until that PR is in
Once that PR is in will merge in the latest upstream branch here to rerun CI and confirm that we have fixed the Conda NumPy 1 downgrade issue mentioned above. Then validate the CI logs to make sure we are getting NumPy 2
Assuming all of that goes well. We can merge after
PR ( #769 ) was just merged Have resolved conflicts (as CI is now running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok can now see NumPy 2 in CI testing of conda packages and it remains through later updates
numpy 2.1.0 py311hed25524_0 conda-forge
Approving now that this issue is cleared up
/merge |
Thanks all! 🙏 |
This PR removes the NumPy<2 pin which is expected to work for
RAPIDS projects once CuPy 13.3.0 is released (CuPy 13.2.0 had
some issues preventing the use with NumPy 2).