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

RMM: Build CUDA 12 packages #1223

Merged
merged 71 commits into from
Jun 21, 2023
Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Feb 23, 2023

Description

This PR builds librmm and rmm conda packages using CUDA 12. Resolves #1207.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@github-actions github-actions bot added the conda label Feb 23, 2023
@bdice bdice added non-breaking Non-breaking change 5 - DO NOT MERGE Hold off on merging; see PR for details improvement Improvement / enhancement to an existing function labels Feb 23, 2023
@bdice bdice self-assigned this Feb 24, 2023
@bdice
Copy link
Contributor Author

bdice commented Feb 28, 2023

I'm looking at the CI output from the CUDA 12 C++ builds. We'll want to fix the ClobberWarnings like this one, perhaps by adopting rapids-core-dependencies which places the RAPIDS versions of CCCL libraries in a subdirectory instead of include/ or lib/ directly.

ClobberWarning: This transaction has incompatible packages due to a shared path.
  packages: nvidia/linux-64::cuda-cccl-12.1.55-0, file:///tmp/conda-bld-output/linux-64::librmm-23.04.00a-cuda12_230228_gfdad1308_47
  path: 'lib/cmake/cub/cub-config-version.cmake'

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.

Following up on the cuda-python run_exports discussion, do we want this?

Edit: Ok punting on these if preferred

conda/recipes/rmm/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/rmm/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/rmm/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/rmm/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/rmm/meta.yaml Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <jakirkham@gmail.com>
@bdice bdice removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Jun 21, 2023
@bdice
Copy link
Contributor Author

bdice commented Jun 21, 2023

@jakirkham Should librmm have a run dependency on cuda-cudart-dev? I see the overlinking issues in librmm-tests, too (example).

WARNING (librmm-tests,bin/gtests/librmm/CUDA_ASYNC_MR_TEST): $RPATH/libcudart.so.12 not found in packages, sysroot(s) nor the missing_dso_whitelist.
.. is this binary repackaging?

We solved that issue in rmm in 865f707 by adding a host dep on cuda-cudart-dev (which used run_exports to get cuda-cudart into the run of rmm). However, I am not sure which is better:

  1. Add cuda-cudart to librmm-tests run section (fix overlinking in librmm-tests only)
  2. Add cuda-cudart-dev to librmm run section (all packages built with librmm will have a dependency on cuda-cudart-dev -- this is fine because librmm is header only and thus should only ever be in host unless used in another header-only package)

edit: I recall there may be an issue with run exports when used in this way... if I remember correctly, putting librmm in host won't allow its dependency cuda-cudart-dev to apply its run exports and put cuda-cudart into the package's run section?

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.

Would suggest adding cuda-cudart-dev to host for librmm-test. Something like the snippet below may do the trick

conda/recipes/librmm/meta.yaml Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <jakirkham@gmail.com>
@vyasr vyasr requested a review from jakirkham June 21, 2023 14:38
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Hopefully the last review! I noticed a couple of things I missed before.

conda/recipes/librmm/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/rmm/meta.yaml Outdated Show resolved Hide resolved
cuda: "12.0"
packages:
- &cuda_python12 cuda-python>=12.0,<13.0
- matrix: # All CUDA 11 versions
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, let's come back to this after we resolve the dfg work.

@bdice bdice requested a review from vyasr June 21, 2023 16:09
conda/recipes/rmm/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/rmm/meta.yaml Outdated Show resolved Hide resolved
@bdice bdice requested a review from a team as a code owner June 21, 2023 18:58
@github-actions github-actions bot added the Python Related to RMM Python API label Jun 21, 2023
@bdice bdice requested a review from vyasr June 21, 2023 18:58
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Assuming CI passes, we look good to go! Awesome work @bdice!

@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit d6cc9b8 into rapidsai:branch-23.08 Jun 21, 2023
43 checks passed
@jakirkham
Copy link
Member

Thanks all! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conda improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to RMM Python API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RMM: CUDA 12 Conda Packages
5 participants