-
Notifications
You must be signed in to change notification settings - Fork 68
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, remove panel ceiling #622
Conversation
Note that Line 200 in ca63e9d
So this depends on PR: rapidsai/cuspatial#1441 |
cc @jameslamb (for awareness) |
Yeah, sorry, I unmarked draft too early, but I think once things are in place and NumPy is picked up we can make it no-draft again and it should be good. |
All good. Was more looping in James as we had discussed the cuSpatial NumPy 2 PR earlier |
I've taken over rapidsai/cuspatial#1441 and so am testing here too. Got I pushed commits here to use the packages built from that PR's CI. Let's see if it'll be enough to loosen the pin here! |
Putting an explicit
That's evidence that Now that rapidsai/cuspatial#1441 is merged, I just pushed commits removing all the testing-only changes here and moving that pin back down to Hopefully we'll see everything pass here and use NumPy 2.x. |
@@ -260,9 +260,6 @@ dependencies: | |||
- pytest | |||
- pytest-cov | |||
- pytest-xdist | |||
- output_types: conda | |||
packages: | |||
- panel>=1.0,<=1.3.4 |
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.
Interested to see how this goes
If we need to ignore a specific version, maybe we can do !=
Though if this is just the case of working around a past solver issue or limiting to a set of tested releases, then simply dropping seems reasonable
Maybe we can follow up with the cuxfilter team on which makes more sense
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.
Yep for sure!
Note this pin is only for the testing environment.... it's already unpinned (>=1.0
) at runtime for users.
- context for why it was pinned (something about compatibility with parallel
pytest
tests): refactor CUDA versions in dependencies.yaml #562 (comment) - no ceiling on
run
dependency:- panel >=1.0
@AjayThorve could the ceiling on panel
be removed?
I do see evidence that it helps conda find a solve with cuxftiler=24.10.*
and numpy>=2.0
.
+ bokeh 3.5.2 pyhd8ed1ab_0 conda-forge 5MB
+ numpy 2.0.2 py312h58c1407_0 conda-forge 8MB
+ panel 1.5.0 pyhd8ed1ab_0 conda-forge 21MB
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.
Yeah, AFAIK, there are no issues at runtime, so as long as the tests pass (seems like they do), this should be fine
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.
Great, thank you!
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.
LGTM!
@@ -260,9 +260,6 @@ dependencies: | |||
- pytest | |||
- pytest-cov | |||
- pytest-xdist | |||
- output_types: conda | |||
packages: | |||
- panel>=1.0,<=1.3.4 |
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.
Yeah, AFAIK, there are no issues at runtime, so as long as the tests pass (seems like they do), this should be fine
/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).
EDIT: Sorry, converted back to draft again, NumPy 2 is not yet picked up.