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

Remove NumPy <2 pin #208

Merged
merged 2 commits into from
Aug 23, 2024
Merged

Remove NumPy <2 pin #208

merged 2 commits into from
Aug 23, 2024

Conversation

seberg
Copy link
Contributor

@seberg seberg commented Aug 19, 2024

This PR removes the NumPy<2 pin. wholegraph does not appear to be a heavy user of NumPy or CuPy, so it should be fine to simply remove the pin.
For other RAPIDS projects with heavier dependency, CuPy 13.3.0 was required (just released) to have sufficient good CuPy/NumPy interoperability.

@seberg seberg added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels Aug 19, 2024
@seberg seberg marked this pull request as ready for review August 23, 2024 08:55
@seberg seberg requested a review from a team as a code owner August 23, 2024 08:55
@seberg
Copy link
Contributor Author

seberg commented Aug 23, 2024

This project has a very light NumPy and no CuPy dependency, so pinning was briefly useful for the transition when NumPy 2 came out, but unnecessary as soon as there were no obvious Python incompatibilities.
(The dependency is so light, there really should be no issue at all.)

@seberg
Copy link
Contributor Author

seberg commented Aug 23, 2024

@jakirkham I am a bit confused by the result here. The tests seem to pick up NumPy 2.1 (good), but are also forced to use pytorch 2.0.0. However, pytorch==2.0.0 should not be compatible with NumPy 2?

Considering how simple the dependency on NumPy is, I don't have a concern at all here (that is a torch/NumPy problem), but it does confuse me right now.
However, if pytorch==2.0.0 is required here, it may even make sense to keep the NumPy pin in light of the torch limitation.

EDIT: To clarify this confusion. pytorch seems installed in a pretty aggressive way, bypassing the incompatible versions. But torch is also somewhat forgiving about not requiring numpy for everything, so it seems that things just work out for the light usage here.

@seberg
Copy link
Contributor Author

seberg commented Aug 23, 2024

Realized that wholegraph is needed for cugraph. Since usage here is increadibly light, I would suggest putting it in soon, to see how the cugraph CI itself does.

@seberg seberg force-pushed the my_new_branch branch 2 times, most recently from 5507030 to 2a33517 Compare August 23, 2024 19:58
@jakirkham
Copy link
Member

jakirkham commented Aug 23, 2024

Sebastian and I discussed the PyTorch question offline


We observed a few things about PyTorch usage here:

  1. PyTorch appears to be a dependency, but is not listed as one
  2. Old versions of PyTorch are being installed in CI

These are documented in issue: #210


This led to a separate question. PyTorch only started supporting NumPy 2 in PyTorch 2.3.0 ( pytorch/pytorch#107302 (comment) ). However the versions here are older than that. We observed:

  1. Older PyTorch installed with NumPy 2 seems to be working here on CI
  2. Testing an older PyTorch locally import torch works with NumPy 2
  3. Testing tensor creation in PyTorch locally only issues a warning (no error)

While it doesn't appear necessary to change PyTorch here, we still think it is a good idea to add the PyTorch dependency and update the versions used. However we don't know enough about how PyTorch is used here to develop a solution without more discussion. To facilitate discussion and reaching an acceptable solution, have raised issue: #210

@jakirkham
Copy link
Member

One job had trouble finding the libraft package. While it doesn't explicitly show a network error, it is quit likely this happened. The other jobs in that group found libraft without issues

Restarted that job and it appears to have cleared up

So just noting this for posterity

@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 44e8a4c into rapidsai:branch-24.10 Aug 23, 2024
82 of 100 checks passed
@jakirkham
Copy link
Member

Thanks Sebastian! 🙏

@seberg seberg deleted the my_new_branch branch August 24, 2024 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants