Audit Wheel Dependencies - #3091
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe PR updates LibRAFT dependency declarations, scopes wheel build arguments to LibRAFT packaging updates
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This dependency audit is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
tarang-jain
added
improvement
tarang-jain
changed the title
libraft wheel dependencies
| - ${{ compiler("c") }} | ||
| host: | ||
| - ${{ pin_subpackage("libraft-headers", exact=True) }} | ||
| - ${{ pin_subpackage("libraft-headers-only", exact=True) }} |
There was a problem hiding this comment.
Why is this changed? What is the specific difference in host/run pinnings that you are aiming to produce?
There was a problem hiding this comment.
Summary of the impact from AI:
libraft-headersdepends onlibraft-headers-only, so selecting it also installs the latter'scuda-version,librmm, andrapids-loggerdependencies. Relative to that shared closure,libraft-headersadditionally bringscuda-profiler-api,libcublas-dev,libcurand-dev,libcusolver-dev,libcusparse-dev,nccl >=2.19, anducxx 0.52.\*.Consequently, replacing
libraft-headerswithlibraft-headers-onlyavoids the four CUDA development packages plusncclanducxx. The runtime CUDA libraries are not removed fromlibraftorlibraft-static: both packages continue to directly requirelibcublas,libcurand,libcusolver, andlibcusparse.cuda-profiler-apiis also directly removed from those two packages in PR 3091, rather than being removed solely through the header-package substitution.
So basically this means that depending on libraft is no longer enough to build other libraries against libraft, because you don't have CUDA math library dev packages. This is a regression.
There was a problem hiding this comment.
My understanding is that the libraft package only contains a few source files in the raft_runtime directory:
add_library(
raft_objs OBJECT
src/raft_runtime/random/rmat_rectangular_generator_int64_double.cu
src/raft_runtime/random/rmat_rectangular_generator_int64_float.cu
src/raft_runtime/random/rmat_rectangular_generator_int_double.cu
src/raft_runtime/random/rmat_rectangular_generator_int_float.cu
src/raft_runtime/solver/lanczos_solver_int64_double.cu
src/raft_runtime/solver/lanczos_solver_int64_float.cu
src/raft_runtime/solver/lanczos_solver_int_double.cu
src/raft_runtime/solver/lanczos_solver_int_float.cu
)
I think any downstream package should depend on libraft-headers and not libraft
There was a problem hiding this comment.
the important question is -- what do we need the libraft pkg to represent -- only the compiled source files or all the packages required to include any header in this repo?
There was a problem hiding this comment.
Looking at the description here:
https://pypi.org/project/libraft-cu12/#:~:text=RAFT%20is%20a,GPU%20with%20Dask.
I think this PR is in the right direction -- libraft's shared library should be C++ only. I'll wait for others / PICs to weigh in.
There was a problem hiding this comment.
@bdice yes, this is the right direction IMO. The main artifact of RAFT is libraft-headers split, not libraft
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
@tarang-jain asked for another review. I have nothing else to ask, so if @divyegala agrees with this direction I won’t block.
divyegala
left a comment
divyegala
left a comment
There was a problem hiding this comment.
@tarang-jain can you make sure to verify this doesn't break any downstream dependencies?
|
/merge |
3 participants
Drops
depends_on_ncclfrom thelibraftwheel's build and runtimerequirement groups (
py_rapids_build_libraft,py_run_libraft) independencies.yaml.More changes on 07/22/2026:
libraftandlibraft-staticdont need to depend on the complete libraft-headers (there are no multi-gpu targets etc). Further raft-dask seems like the only target that needs the cmake flag for ncclAddresses #2977.