Use CCCL random for device PRNG draws - #8243
Conversation
There was a problem hiding this comment.
Pull request overview
This PR begins migrating cuML device-side PRNG usage from RAFT/curand-based implementations to CCCL (cuda::std::random), simplifying kernels and moving toward removing curand dependencies (per #8242).
Changes:
- Replaced RAFT Philox-based integer draws in UMAP optimization kernels with
cuda::std::philox4x64+cuda::std::uniform_int_distribution. - Replaced curand usage in KernelSHAP sampling with CCCL random utilities.
- Replaced RAFT RNG utilities in decision tree sampling kernels and quantile sampling with CCCL random utilities; removed a direct
<curand.h>include from UMAP algo header.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cpp/src/umap/simpl_set_embed/optimize_inverse_kernel.cuh | Switch negative sampling RNG to cuda::std::random. |
| cpp/src/umap/simpl_set_embed/optimize_batch_kernel.cuh | Switch multiple negative-sampling codepaths to cuda::std::random. |
| cpp/src/umap/simpl_set_embed/algo.cuh | Remove unused curand include. |
| cpp/src/genetic/genetic.cu | Switch tournament selection RNG draws to cuda::std::random. |
| cpp/src/explainer/kernel_shap.cu | Replace curand-based sampling with cuda::std::random. |
| cpp/src/decisiontree/batched-levelalgo/quantiles.cuh | Replace RAFT RNG-based row sampling with cuda::std::random. |
| cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuh | Replace RAFT RNG utilities with cuda::std::random for feature sampling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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 (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughDevice-side RNGs in decision tree, explainer, genetic, and UMAP CUDA kernels were migrated from RAFT/CURAND to CUDA C++ ChangesRNG Implementation Modernization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuh`:
- Around line 303-304: The uniform_real_distribution uniform01 currently allows
0.0f which can make fp_uniform_val==0 and produce inf/NaN in
algo_L_sample_kernel when computing raft::log(fp_uniform_val) and subsequent
W/log(1-W) math; fix by ensuring fp_uniform_val is strictly positive before any
raft::log call (either initialize uniform01 with a positive lower bound like
std::numeric_limits<float>::min() or clamp fp_uniform_val = max(fp_uniform_val,
eps) immediately before using it), and apply the same guard before computing W
and any raft::log(1 - W) to avoid divide-by-zero/NaN propagation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 834f5f1a-f7c1-4df6-9312-500966d51eb9
📒 Files selected for processing (7)
cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuhcpp/src/decisiontree/batched-levelalgo/quantiles.cuhcpp/src/explainer/kernel_shap.cucpp/src/genetic/genetic.cucpp/src/umap/simpl_set_embed/algo.cuhcpp/src/umap/simpl_set_embed/optimize_batch_kernel.cuhcpp/src/umap/simpl_set_embed/optimize_inverse_kernel.cuh
💤 Files with no reviewable changes (1)
- cpp/src/umap/simpl_set_embed/algo.cuh
98a18ff to
b7703c2
Compare
| static_cast<int>(T(epoch - _epoch_of_next_negative_sample) / epochs_per_negative_sample); | ||
|
|
||
| raft::random::detail::PhiloxGenerator gen(seed, static_cast<nnz_t>(e), 0); | ||
| cuda::std::philox4x64 rng(static_cast<cuda::std::philox4x64::result_type>(seed)); |
There was a problem hiding this comment.
Please wrap this in raft insted of invoking CCCL durectly here.
Start on #8242