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 #2414

Merged
merged 4 commits into from
Aug 23, 2024
Merged

Remove NumPy <2 pin #2414

merged 4 commits into from
Aug 23, 2024

Conversation

seberg
Copy link
Contributor

@seberg seberg commented Aug 19, 2024

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

@seberg seberg added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Aug 19, 2024
@jakirkham jakirkham marked this pull request as ready for review August 22, 2024 18:57
@jakirkham jakirkham requested a review from a team as a code owner August 22, 2024 18:58
@jakirkham jakirkham closed this Aug 22, 2024
@jakirkham jakirkham reopened this Aug 22, 2024
@jakirkham
Copy link
Member

Now that CuPy 13.3.0 is out, restarting CI and marking this ready for review

@jameslamb jameslamb requested review from jameslamb and removed request for KyleFromNVIDIA August 22, 2024 20:48
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Changes looked good to me.

I double-checked that this project doesn't have a build dependency on numpy, so no need to add any pins for build time.

Looked around at all the other uses of numpy and I think this got all of them. Assuming CI passes, I think this is good to go.

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Sebastian and James! 🙏

The RAFT-Dask package doesn't actually `import numpy` anywhere inside.
This may have been different in the past. However today RAFT-Dask does
not use NumPy.

Further pylibraft, which does use NumPy and has a dependency on it, is
already a dependency of RAFT-Dask. As a result, NumPy is pulled into the
environment anyways.

The conda package for raft-dask already doesn't have NumPy and has been
working ok for a while. So line up the wheels to match this behavior.
"numpy>=1.23,<2.0a0",
"pylibraft==24.10.*,>=0.0.0a0",
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT numpy is not used in raft-dask

$ git grep numpy -- python/raft-dask/ | wc -l
       0

Also the Conda package doesn't include it

run:
{% if cuda_major == "11" %}
- cudatoolkit
{% else %}
- cuda-cudart
{% endif %}
- {{ pin_compatible('cuda-version', max_pin='x', min_pin='x') }}
- dask-cuda ={{ minor_version }}
- rapids-dask-dependency ={{ minor_version }}
- joblib >=0.11
- nccl >=2.9.9
- pylibraft {{ version }}
- python x.x
- rmm ={{ minor_version }}
- ucx-py {{ ucx_py_version }}
- distributed-ucxx {{ ucxx_version }}

The origin of this numpy line in raft-dask's wheels traces back to the addition of wheels in PR ( #1013 ). Though not seeing any discussion about why numpy was added there. Even at the time conda raft-dask packages did not depend on numpy. Am guessing this was just copied into the different RAFT wheel packages. So it is likely just an oversight that this hasn't already been cleaned up


Though pylibraft, used by raft-dask, does use numpy

$ git grep numpy -- python/pylibraft/ | wc -l
      52

Also pylibraft already includes numpy as a dependency

- numpy >=1.23,<3.0a0

"numpy>=1.23,<3.0a0",

So we can be confident numpy will be in the environment raft-dask is installed into even though it appears raft-dask itself doesn't need it


Given all of this, took this opportunity to clean this up and align our packages on the handling of this dependency

@jakirkham
Copy link
Member

jakirkham commented Aug 23, 2024

There was a network error in this CI job. Will wait for running CI jobs to complete and then restart failed jobs

Edit: Looks like that fixed it

@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 2dafe79 into rapidsai:branch-24.10 Aug 23, 2024
58 of 60 checks passed
@jakirkham
Copy link
Member

Thanks Sebastian and James! 🙏

@seberg seberg deleted the my_new_branch branch August 23, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants