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 new cuda-python layout #117

Open
vyasr opened this issue Nov 6, 2024 · 11 comments
Open

Support new cuda-python layout #117

vyasr opened this issue Nov 6, 2024 · 11 comments

Comments

@vyasr
Copy link
Contributor

vyasr commented Nov 6, 2024

cuda-python 12.6.1 (11.8.4 on the CUDA 11 line) introduces a new layout and deprecates the old one. This is not considered a breaking change for them, but it is for much of RAPIDS CI where warnings are treated as errors. In the short term, the resolution is #116, which adds pinnings to avoid this problem (as well as others). Longer-term, we probably do not want to just change our imports because that would make us require only the latest versions of the package, which would be fairly constraining and could make our environments unsolveable if other libraries do not make similar updates. Instead, we will need to handle this importing conditionally to ensure that we can get what we need. Presumably this doesn't impact cimports since those would be Cython build warnings (and we are generally not blocked on those), so conditional imports should be sufficient to enable a broader range of usability.

@bdice
Copy link
Contributor

bdice commented Nov 18, 2024

I chatted with @vyasr about this offline. I would prefer for us to increase our lower bound of cuda-python to >=12.6.2 / >=11.8.5 to ensure we can use the new package layout (cuda.bindings), and refactor our imports accordingly. There is some risk in taking on a lower-bound that is so new, because it could constrain our ability to have consistent environments with other cuda-python consumers if other libraries have upper bounds. Hopefully by the 25.02 release, we would know if such issues exist in practice.

We can always go the route of adding backwards compatibility for older cuda-python versions (via conditional imports) later on. Using conditional imports introduces complexity and forces us into another decision (when do we drop support for "old style" cuda-python?) so I am not really eager to start with this approach. Increasing the lower bound to use the new layout also eliminates all deprecation warnings, keeping us from running into issues like rapidsai/rmm#1730.

We do occasionally use conditional imports for core dependencies like pandas and pyarrow, but I would like to avoid that in this case, especially given that RAPIDS works closely with cuda-python and can request patch releases for most regressions we find.

@vyasr
Copy link
Contributor Author

vyasr commented Nov 18, 2024

Thanks for writing this up Bradley! On further consideration I am fine with the approach of upgrading now and reenabling older versions later.

Hopefully by the 25.02 release, we would know if such issues exist in practice. We can always go the route of adding backwards compatibility for older cuda-python versions (via conditional imports) later on.

@leofang do you have a sense of how likely this case is? Basically this boils down to a question of how many packages you are aware of that are doing from cuda.cudart import or equivalent from other modules (but not cimport; I'm only concerned with runtime Python imports) in their code bases. I don't have a good sense of how concerned we should currently be about needing to create (conda or pip) environments where packages are still relying on the old layout (which I guess just means deprecation warnings, not runtime errors) or pinning to older versions (seems unlikely). Given that the latter seems unlikely I'm pretty OK with moving forward with Bradley's proposal.

@vyasr
Copy link
Contributor Author

vyasr commented Dec 3, 2024

@bdice can we get the cuda-python unpinning in now?

@leofang
Copy link
Member

leofang commented Dec 3, 2024

Sorry I missed this.

do you have a sense of how likely this case is? Basically this boils down to a question of how many packages you are aware of that are doing from cuda.cudart import or equivalent from other modules (but not cimport; I'm only concerned with runtime Python imports) in their code bases.

I think from cuda import {cuda,cudart,nvrtc} has been the canonical way of using the bindings, because Vlad did the right thing from day 1 and did not expose the sub packages to under the cuda namespace; that is, in REPL if one does import cuda; cuda.<tab> nothing would come out. Not sure if this answers what you asked, though.

@bdice
Copy link
Contributor

bdice commented Dec 4, 2024

PRs to lift the upper bounds have been opened:

Once those are merged, we can file follow-up PRs with the above proposal to update the bounds to >=12.6.2,<13.0a0 / >=11.8.5,<12.0a0 and migrate to the new binding names. We should try to complete this for the 25.02 release, to ensure that users don't see the deprecation warnings from the old cuda-python module names.

@jameslamb
Copy link
Member

A meta-comment about the PRs in #117 (comment) ... it's ok to change to e.g. cuda-python>=12.0,<13.0a0 without adding a !=12.6.1 because the 12.6.1 packages have been marked broken.

Image

ref: https://anaconda.org/conda-forge/cuda-python/files?version=12.6.1

And similar for 11.8.4: https://anaconda.org/conda-forge/cuda-python/files?version=11.8.4

Just wanted to get that in writing, I was wondering it while reviewing.

@bdice
Copy link
Contributor

bdice commented Dec 6, 2024

It seems like building packages using Cython like RMM/cuDF with the new-layout releases of cuda-python makes them incompatible with older cuda-python versions at runtime. Failures look like ModuleNotFoundError: No module named 'cuda.bindings'.

For now, I will set new lower bounds: cuda-python >=12.6.2,<13.0a0 and cuda-python >=11.8.5,<12.0a0 across RAPIDS. That will work around the problem, and we want these new layouts anyway for feature reasons. See rapidsai/rmm#1675.

However, this is likely an unintended breakage, so @vyasr said he will file an issue and we can cross-link it here.

@vyasr
Copy link
Contributor Author

vyasr commented Dec 6, 2024

Sorry I missed this.

do you have a sense of how likely this case is? Basically this boils down to a question of how many packages you are aware of that are doing from cuda.cudart import or equivalent from other modules (but not cimport; I'm only concerned with runtime Python imports) in their code bases.

I think from cuda import {cuda,cudart,nvrtc} has been the canonical way of using the bindings, because Vlad did the right thing from day 1 and did not expose the sub packages to under the cuda namespace; that is, in REPL if one does import cuda; cuda.<tab> nothing would come out. Not sure if this answers what you asked, though.

@leofang I think really our question is, how many projects outside RAPIDS are using cuda-python? If it's small (our hypothesis) then tightly constraining usable versions isn't a problem.

@vyasr
Copy link
Contributor Author

vyasr commented Dec 6, 2024

NVIDIA/cuda-python#274 details the most recent issue @bdice mentioned above.

rapids-bot bot pushed a commit to rapidsai/rmm that referenced this issue Dec 7, 2024
We require a newer cuda-python lower bound for new features and to use the new layout.
This will fix a number of errors observed when the runtime version of cuda-python is older than the version used to build packages using Cython features from cuda-python.

See rapidsai/build-planning#117 (comment) for details.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - James Lamb (https://github.com/jameslamb)

URL: #1751
rapids-bot bot pushed a commit to rapidsai/cuvs that referenced this issue Dec 7, 2024
We require a newer cuda-python lower bound for new features and to use the new layout.
This will fix a number of errors observed when the runtime version of cuda-python is older than the version used to build packages using Cython features from cuda-python.

See rapidsai/build-planning#117 (comment) for details.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - James Lamb (https://github.com/jameslamb)

URL: #524
rapids-bot bot pushed a commit to rapidsai/kvikio that referenced this issue Dec 7, 2024
We require a newer cuda-python lower bound for new features and to use the new layout.
This will fix a number of errors observed when the runtime version of cuda-python is older than the version used to build packages using Cython features from cuda-python.

See rapidsai/build-planning#117 (comment) for details.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - James Lamb (https://github.com/jameslamb)

URL: #573
rapids-bot bot pushed a commit to rapidsai/raft that referenced this issue Dec 7, 2024
We require a newer cuda-python lower bound for new features and to use the new layout.
This will fix a number of errors observed when the runtime version of cuda-python is older than the version used to build packages using Cython features from cuda-python.

See rapidsai/build-planning#117 (comment) for details.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - James Lamb (https://github.com/jameslamb)

URL: #2522
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this issue Dec 7, 2024
We require a newer cuda-python lower bound for new features and to use the new layout.
This will fix a number of errors observed when the runtime version of cuda-python is older than the version used to build packages using Cython features from cuda-python.

See rapidsai/build-planning#117 (comment) for details.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - James Lamb (https://github.com/jameslamb)

URL: #17547
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this issue Dec 9, 2024
We require a newer cuda-python lower bound for new features and to use the new layout.
This will fix a number of errors observed when the runtime version of cuda-python is older than the version used to build packages using Cython features from cuda-python.

See rapidsai/build-planning#117 (comment) for details.

Authors:
  - Bradley Dice (https://github.com/bdice)
  - Ralph Liu (https://github.com/nv-rliu)

Approvers:
  - James Lamb (https://github.com/jameslamb)

URL: #4813
@bdice
Copy link
Contributor

bdice commented Dec 10, 2024

I migrated rmm to the new cuda-python layout in rapidsai/rmm#1755. We’ll need to make similar changes across all other RAPIDS repos.

At the same time that we do this change, we should refactor all the places in RAPIDS where we are using numba to get the proper local CUDA runtime version, and use the new cuda-python API for getting the local runtime version. I still need to refactor rmm’s numba dependency now that we fixed rmm. It may let us make numba optional or not-required for rmm.

@bdice
Copy link
Contributor

bdice commented Dec 11, 2024

I filed a couple of related PRs in cuGraph:

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

No branches or pull requests

4 participants