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

raft: Build CUDA 12 packages #1388

Merged
merged 76 commits into from
Jul 5, 2023

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Mar 30, 2023

First attempt at building libraft with CUDA 12

Closes #1278

@vyasr vyasr self-assigned this Mar 30, 2023
@github-actions github-actions bot added the ci label Mar 30, 2023
@vyasr vyasr added 3 - Ready for Review feature request New feature or request non-breaking Non-breaking change and removed ci labels Mar 30, 2023
@github-actions github-actions bot added the ci label Mar 30, 2023
conda/recipes/libraft/meta.yaml Show resolved Hide resolved
conda/recipes/libraft/meta.yaml Outdated Show resolved Hide resolved
@bdice bdice mentioned this pull request Mar 31, 2023
2 tasks
@vyasr vyasr changed the title Build libraft with CUDA 12 Build CUDA 12 packages of libraft Mar 31, 2023
@vyasr vyasr added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Mar 31, 2023
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@jakirkham jakirkham requested a review from bdice June 29, 2023 21:11
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Thanks @dantegd! This looks fine to me.

conda/recipes/libraft/meta.yaml Outdated Show resolved Hide resolved
with:
build_type: ${{ inputs.build_type || 'branch' }}
branch: ${{ inputs.branch }}
date: ${{ inputs.date }}
sha: ${{ inputs.sha }}
skip_upload_pkgs: libraft-template
docs-build:
if: github.ref_type == 'branch'
if: github.ref_type == 'branch' && github.event_name == 'push'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Member

Choose a reason for hiding this comment

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

that change was before I started working on the PR, I wonder if @ajschmidt8 might be able to help us here

cpp/bench/ann/CMakeLists.txt Show resolved Hide resolved
Comment on lines +181 to +186
- cuda-cudart-dev
- cuda-profiler-api
- libcublas-dev
- libcurand-dev
- libcusolver-dev
- libcusparse-dev
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can see of where the cudatoolkit list is used, most of the places are runtime rather than build envs. Should we be including the non -dev packages instead?

And if we do need the dev packages, do we need cuda-nvcc @bdice?

Copy link
Contributor

@bdice bdice Jun 30, 2023

Choose a reason for hiding this comment

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

This is intentional. RAFT is a special case within RAPIDS because it is header-only (like RMM) and has nontrivial library dev dependencies (unlike RMM). We must have the dev packages to build RAFT, but we also list these dev packages as run dependencies for libraft-headers (but not libraft-headers-only) because they are required to use the headers. We can discuss the pros and cons of whether header-only libraries should list their dev dependencies in run (this is a choice that can go either way), but I would prefer to be as consistent as possible with past decisions for CUDA 11 and the current conda recipes for this PR, and most importantly avoid holding back merging for packaging discussions if we can defer them (to prevent blocking cuml/cugraph from starting their CUDA 12 work).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh of course wasn't thinking about the header-only bit. OK yeah that makes sense. But then practically speaking we are always going to need cuda-nvcc as well. I'm fine not listing it and leaving that to the user since it's the compiler if that's your preference.

@vyasr
Copy link
Contributor Author

vyasr commented Jun 29, 2023

I can't approve or request changes on my own PR, so just leaving comments.

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 all! 🙏

Had a few comments/questions below

conda/recipes/raft-dask/meta.yaml Show resolved Hide resolved
conda/recipes/raft-dask/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libraft/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libraft/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libraft/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libraft/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libraft/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/pylibraft/meta.yaml Show resolved Hide resolved
conda/recipes/libraft/meta.yaml Show resolved Hide resolved
conda/recipes/libraft/meta.yaml Show resolved Hide resolved
Co-authored-by: jakirkham <jakirkham@gmail.com>
@jakirkham jakirkham requested a review from cjnolet June 30, 2023 18:32
jakirkham

This comment was marked as resolved.

@dantegd dantegd removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Jun 30, 2023
@jakirkham

This comment was marked as resolved.

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.

Think the couple CI failures are occurring because we are missing this version constraint

conda/recipes/raft-dask/meta.yaml Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <jakirkham@gmail.com>
- {{ compiler('cuda') }} {{ cuda_version }}
{% if cuda_major == "11" %}
- {{ compiler('cuda11') }} ={{ cuda_version }}

Copy link
Member

@jakirkham jakirkham Jul 3, 2023

Choose a reason for hiding this comment

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

Suggesting dropping this blank line (missed it in the previous suggestion 🤦)

Suggested change

Copy link
Member

Choose a reason for hiding this comment

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

Submitted as PR ( #1637 )

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks everyone who helped make this possible!

@dantegd
Copy link
Member

dantegd commented Jul 5, 2023

/merge

@rapids-bot rapids-bot bot merged commit 32b3dba into rapidsai:branch-23.08 Jul 5, 2023
@jakirkham
Copy link
Member

Thanks everyone! 🙏

rapids-bot bot pushed a commit that referenced this pull request Jul 5, 2023
Follow up on this review comment ( #1388 (comment) )

Authors:
  - https://github.com/jakirkham

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - Ray Douglass (https://github.com/raydouglass)

URL: #1637
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

RAFT: CUDA 12 Conda Packages
8 participants