[DO NOT MERGE] Test RAFT 3052 - #8261
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:
📝 WalkthroughWalkthroughThis PR adds two CI helper scripts for PR-derived conda channels and wheels, then sources them from build and test workflows before environment setup, artifact download, and wheel installation steps. ChangesPR artifact sourcing in CI workflows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci/use_conda_packages_from_prs.sh (1)
1-33:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftTest-only script with hardcoded PR numbers must not be merged.
The PR title "[DO NOT MERGE] Test RAFT 3052" confirms this, but the script contains hardcoded PR numbers (raft 3052, cuvs 2227) that would break if merged to main. Before this can be merged, the script needs to be parameterized or removed entirely.
🤖 Prompt for 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. In `@ci/use_conda_packages_from_prs.sh` around lines 1 - 33, This script hardcodes PR numbers into LIBRAFT_CHANNEL, RAFT_CHANNEL and LIBCUVS_CHANNEL which must be parameterized before merging; update the script to accept PR identifiers via environment variables or positional arguments (e.g., RAFT_PR, CUVS_PR or $1/$2) and use those when calling rapids-get-pr-artifact, add sensible defaults or a guard that exits unless a TEST_PR flag is set to prevent accidental runs on main, and keep the rest of the logic that builds RAPIDS_PREPENDED_CONDA_CHANNELS and iterates to conda config unchanged so the channel list is derived from the provided params rather than fixed values.
🤖 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 `@ci/use_conda_packages_from_prs.sh`:
- Around line 29-32: The loop over RAPIDS_PREPENDED_CONDA_CHANNELS may call
conda config with an empty string; update the loop that iterates _channel to
skip empty values by testing _channel is non-empty (e.g., using a shell check
like [[ -n "$_channel" ]] or if [ -z ... ] continue) before running conda config
--system --add channels "${_channel}" so only valid channel strings are added.
- Around line 6-8: Enable strict shell failure at the top of
ci/use_conda_packages_from_prs.sh (set -euo pipefail) and validate the results
of the rapids-get-pr-artifact calls by checking that LIBRAFT_CHANNEL,
RAFT_CHANNEL, and LIBCUVS_CHANNEL are non-empty after assignment; if any is
empty, print a clear error mentioning the variable name and the
rapids-get-pr-artifact invocation that failed, and exit with a non-zero status
so an empty string is never passed as a conda channel.
---
Outside diff comments:
In `@ci/use_conda_packages_from_prs.sh`:
- Around line 1-33: This script hardcodes PR numbers into LIBRAFT_CHANNEL,
RAFT_CHANNEL and LIBCUVS_CHANNEL which must be parameterized before merging;
update the script to accept PR identifiers via environment variables or
positional arguments (e.g., RAFT_PR, CUVS_PR or $1/$2) and use those when
calling rapids-get-pr-artifact, add sensible defaults or a guard that exits
unless a TEST_PR flag is set to prevent accidental runs on main, and keep the
rest of the logic that builds RAPIDS_PREPENDED_CONDA_CHANNELS and iterates to
conda config unchanged so the channel list is derived from the provided params
rather than fixed values.
🪄 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: 44356763-1983-45a1-93cf-9c6baa5f94e2
📒 Files selected for processing (12)
ci/build_cpp.shci/build_docs.shci/build_python.shci/build_wheel_cuml.shci/build_wheel_libcuml.shci/test_cpp.shci/test_notebooks.shci/test_python_common.shci/test_wheel.shci/test_wheel_dask.shci/use_conda_packages_from_prs.shci/use_wheels_from_prs.sh
| LIBRAFT_CHANNEL=$(rapids-get-pr-artifact raft 3052 cpp conda) | ||
| RAFT_CHANNEL=$(rapids-get-pr-artifact raft 3052 python conda) | ||
| LIBCUVS_CHANNEL=$(rapids-get-pr-artifact cuvs 2227 cpp conda) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if rapids-get-pr-artifact exists and how it handles invalid inputs
command -v rapids-get-pr-artifact
# Search for error handling patterns in similar CI helper scripts
rg -n -A 3 -B 1 'rapids-get-pr-artifact' --type sh -g 'ci/**/*.sh'
# Look for validation patterns after command substitution in CI scripts
rg -n -A 2 '\$\(rapids-get-pr-artifact.*\)' --type sh -g 'ci/**/*.sh'Repository: rapidsai/cuml
Length of output: 2069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target script
echo "---- ci/use_conda_packages_from_prs.sh (top 120 lines) ----"
nl -ba ci/use_conda_packages_from_prs.sh | head -n 120
# Inspect the related wheels script (for consistency/precedent)
echo "---- ci/use_wheels_from_prs.sh (top 140 lines) ----"
nl -ba ci/use_wheels_from_prs.sh | head -n 140
# Find all uses of rapids-get-pr-artifact and whether any validation exists nearby
echo "---- occurrences + nearby lines (validation/error handling) ----"
rg -n -A 5 -B 2 'rapids-get-pr-artifact' --type sh 'ci/**/*.sh' || true
# Specifically look for empty-string validation after command substitution assignments
echo "---- empty-string / non-empty validation after rapids-get-pr-artifact ----"
rg -n '\[\[\s*-n\s+"\$\{?(LIB|RAFT|LIBCUVS)[A-Z0-9_]*\}?"\s*\]\]' ci/**/*.sh || true
rg -n '\[\[\s*-n\s+"\$\([^)]*rapids-get-pr-artifact' --type sh ci/**/*.sh || true
rg -n 'rapids-get-pr-artifact.*\n.*\[\[\s*-n' --type sh ci/**/*.sh || true
# Look for existing checks on variables populated from rapids-get-pr-artifact
echo "---- checks of variables populated from rapids-get-pr-artifact ----"
rg -n 'LIBRAFT_CHANNEL|RAFT_CHANNEL|LIBCUVS_CHANNEL' ci/use_conda_packages_from_prs.sh || trueRepository: rapidsai/cuml
Length of output: 178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- ci/use_conda_packages_from_prs.sh (first 120 lines) ----"
sed -n '1,120p' ci/use_conda_packages_from_prs.sh
echo "---- ci/use_wheels_from_prs.sh (first 140 lines) ----"
sed -n '1,140p' ci/use_wheels_from_prs.sh
echo "---- occurrences + nearby lines (validation/error handling) ----"
rg -n -A 6 -B 2 'rapids-get-pr-artifact' --type sh 'ci/**/*.sh' || true
echo "---- checks of variables populated from rapids-get-pr-artifact ----"
rg -n 'LIBRAFT_CHANNEL|RAFT_CHANNEL|LIBCUVS_CHANNEL' ci/use_conda_packages_from_prs.sh || true
echo "---- empty-string / non-empty validation patterns in CI scripts ----"
rg -n '\[\[\s*-n\s+"\$\{?(LIB|RAFT|LIBCUVS)[A-Z0-9_]*\}?"\s*\]\]' --type sh 'ci/**/*.sh' || true
rg -n '\[\[\s*-n\s+"\$\([^)]*rapids-get-pr-artifact' --type sh 'ci/**/*.sh' || trueRepository: rapidsai/cuml
Length of output: 2901
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- locate rapids-get-pr-artifact in repo ----"
fd -a 'rapids-get-pr-artifact' . || true
rg -n --files-with-matches 'rapids-get-pr-artifact' . || true
echo "---- find likely script locations on PATH (from repo tooling) ----"
command -v rapids-get-pr-artifact || true
type rapids-get-pr-artifact || true
echo "---- search all CI shell scripts for rapids-get-pr-artifact usage + nearby validation ----"
rg -n -A 6 -B 2 'rapids-get-pr-artifact' --type sh ci || true
echo "---- search for non-empty checks tied to rapids-get-pr-artifact outputs ----"
rg -n '\[\[\s*-n\s+"\$\{?[^}]*\}?\]|\[\[\s*-n\s+"\$\([^)]*\)\]' --type sh ci || true
echo "---- inspect rapids-get-pr-artifact script if it exists in PATH and is readable ----"
PTH="$(command -v rapids-get-pr-artifact 2>/dev/null || true)"
if [[ -n "$PTH" && -r "$PTH" ]]; then
echo "---- contents: $PTH (first 160 lines) ----"
sed -n '1,160p' "$PTH"
else
echo "---- cannot read rapids-get-pr-artifact from PATH (not found or not readable) ----"
fiRepository: rapidsai/cuml
Length of output: 2637
Add validation for non-empty conda channels after rapids-get-pr-artifact
ci/use_conda_packages_from_prs.sh assigns LIBRAFT_CHANNEL, RAFT_CHANNEL, and LIBCUVS_CHANNEL from command substitution with no checks, and the script doesn’t enable set -euo pipefail, so empty strings can be added as conda channels.
🛡️ Proposed validation to add
# download CI artifacts
LIBRAFT_CHANNEL=$(rapids-get-pr-artifact raft 3052 cpp conda)
+[[ -n "${LIBRAFT_CHANNEL}" ]] || { echo "ERROR: Failed to download libraft channel"; exit 1; }
RAFT_CHANNEL=$(rapids-get-pr-artifact raft 3052 python conda)
+[[ -n "${RAFT_CHANNEL}" ]] || { echo "ERROR: Failed to download raft channel"; exit 1; }
LIBCUVS_CHANNEL=$(rapids-get-pr-artifact cuvs 2227 cpp conda)
+[[ -n "${LIBCUVS_CHANNEL}" ]] || { echo "ERROR: Failed to download libcuvs channel"; exit 1; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LIBRAFT_CHANNEL=$(rapids-get-pr-artifact raft 3052 cpp conda) | |
| RAFT_CHANNEL=$(rapids-get-pr-artifact raft 3052 python conda) | |
| LIBCUVS_CHANNEL=$(rapids-get-pr-artifact cuvs 2227 cpp conda) | |
| LIBRAFT_CHANNEL=$(rapids-get-pr-artifact raft 3052 cpp conda) | |
| [[ -n "${LIBRAFT_CHANNEL}" ]] || { echo "ERROR: Failed to download libraft channel"; exit 1; } | |
| RAFT_CHANNEL=$(rapids-get-pr-artifact raft 3052 python conda) | |
| [[ -n "${RAFT_CHANNEL}" ]] || { echo "ERROR: Failed to download raft channel"; exit 1; } | |
| LIBCUVS_CHANNEL=$(rapids-get-pr-artifact cuvs 2227 cpp conda) | |
| [[ -n "${LIBCUVS_CHANNEL}" ]] || { echo "ERROR: Failed to download libcuvs channel"; exit 1; } |
🤖 Prompt for 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.
In `@ci/use_conda_packages_from_prs.sh` around lines 6 - 8, Enable strict shell
failure at the top of ci/use_conda_packages_from_prs.sh (set -euo pipefail) and
validate the results of the rapids-get-pr-artifact calls by checking that
LIBRAFT_CHANNEL, RAFT_CHANNEL, and LIBCUVS_CHANNEL are non-empty after
assignment; if any is empty, print a clear error mentioning the variable name
and the rapids-get-pr-artifact invocation that failed, and exit with a non-zero
status so an empty string is never passed as a conda channel.
| for _channel in "${RAPIDS_PREPENDED_CONDA_CHANNELS[@]}" | ||
| do | ||
| conda config --system --add channels "${_channel}" | ||
| done |
There was a problem hiding this comment.
Guard against adding empty channels to conda config.
If any channel variable is empty (e.g., due to artifact download failure), conda config --system --add channels "" may fail or behave unexpectedly. Add a check to skip empty values.
🛡️ Proposed fix to skip empty channels
for _channel in "${RAPIDS_PREPENDED_CONDA_CHANNELS[@]}"
do
+ [[ -n "${_channel}" ]] || continue
conda config --system --add channels "${_channel}"
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _channel in "${RAPIDS_PREPENDED_CONDA_CHANNELS[@]}" | |
| do | |
| conda config --system --add channels "${_channel}" | |
| done | |
| for _channel in "${RAPIDS_PREPENDED_CONDA_CHANNELS[@]}" | |
| do | |
| [[ -n "${_channel}" ]] || continue | |
| conda config --system --add channels "${_channel}" | |
| done |
🤖 Prompt for 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.
In `@ci/use_conda_packages_from_prs.sh` around lines 29 - 32, The loop over
RAPIDS_PREPENDED_CONDA_CHANNELS may call conda config with an empty string;
update the loop that iterates _channel to skip empty values by testing _channel
is non-empty (e.g., using a shell check like [[ -n "$_channel" ]] or if [ -z ...
] continue) before running conda config --system --add channels "${_channel}" so
only valid channel strings are added.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ci/test_notebooks.sh (1)
17-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame redundant double-sourcing as
test_cpp.sh.Per the change details, the helper is already sourced earlier (Lines 12-15) before the artifact download, so the variables consumed by
--prepend-channelare in scope without re-sourcing at Line 17. The secondsourceonly re-adds the channels to the system conda config. Consider dropping it.🤖 Prompt for 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. In `@ci/test_notebooks.sh` around lines 17 - 24, The notebook test script is sourcing the conda helper twice, which redundantly re-applies channel setup. Remove the second source in test_notebooks.sh and rely on the earlier sourcing so the LIBRAFT_CHANNEL, RAFT_CHANNEL, and LIBCUVS_CHANNEL variables remain available for rapids-dependency-file-generator without mutating the system conda config again.ci/test_cpp.sh (1)
16-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant second sourcing of the helper.
use_conda_packages_from_prs.shis sourced at Line 16 and again at Line 20. Because it issourced (not run in a subshell),LIBRAFT_CHANNEL/RAFT_CHANNEL/LIBCUVS_CHANNELfrom the first source are still in scope at Line 21, so the second source is unnecessary. It also re-runs the helper'sconda config --system --add channelsloop, prepending the same channels a second time. The secondsourcecan be dropped.♻️ Proposed change
rapids-logger "Generate C++ testing dependencies" -source ./ci/use_conda_packages_from_prs.sh rapids-dependency-file-generator \🤖 Prompt for 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. In `@ci/test_cpp.sh` around lines 16 - 20, The C++ test setup is sourcing use_conda_packages_from_prs.sh twice, which is redundant because the first source already leaves LIBRAFT_CHANNEL, RAFT_CHANNEL, and LIBCUVS_CHANNEL in scope for the rest of ci/test_cpp.sh. Remove the second source near the rapids-logger step so the helper’s conda config channel-add loop only runs once, and keep the existing CPP_CHANNEL download flow unchanged.
🤖 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 `@ci/test_python_common.sh`:
- Line 18: The conda PR helper is being sourced twice in the test script, which
can re-apply its channel mutations and create duplicate state. Update the logic
in the test setup around source ./ci/use_conda_packages_from_prs.sh so it is
only sourced once, and reuse the variables it sets for the rest of the script
instead of re-invoking it.
---
Nitpick comments:
In `@ci/test_cpp.sh`:
- Around line 16-20: The C++ test setup is sourcing
use_conda_packages_from_prs.sh twice, which is redundant because the first
source already leaves LIBRAFT_CHANNEL, RAFT_CHANNEL, and LIBCUVS_CHANNEL in
scope for the rest of ci/test_cpp.sh. Remove the second source near the
rapids-logger step so the helper’s conda config channel-add loop only runs once,
and keep the existing CPP_CHANNEL download flow unchanged.
In `@ci/test_notebooks.sh`:
- Around line 17-24: The notebook test script is sourcing the conda helper
twice, which redundantly re-applies channel setup. Remove the second source in
test_notebooks.sh and rely on the earlier sourcing so the LIBRAFT_CHANNEL,
RAFT_CHANNEL, and LIBCUVS_CHANNEL variables remain available for
rapids-dependency-file-generator without mutating the system conda config again.
🪄 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: de68febf-1425-4549-8f0b-5cf2b425f308
📒 Files selected for processing (3)
ci/test_cpp.shci/test_notebooks.shci/test_python_common.sh
| PYTHON_CHANNEL=$(rapids-download-from-github "$(rapids-artifact-name conda_python cuml cuml --stable --cuda "$RAPIDS_CUDA_VERSION")") | ||
|
|
||
| rapids-logger "Generate Python testing dependencies" | ||
| source ./ci/use_conda_packages_from_prs.sh |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove duplicate sourcing of PR conda helper.
Line 18 re-sources ./ci/use_conda_packages_from_prs.sh after it was already sourced at Line 13. Since this helper mutates conda channel state, running it twice can duplicate prepended channels and cause unnecessary artifact/config churn. Source it once and reuse the resulting variables.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 18-18: Not following: ./ci/use_conda_packages_from_prs.sh was not specified as input (see shellcheck -x).
(SC1091)
🤖 Prompt for 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.
In `@ci/test_python_common.sh` at line 18, The conda PR helper is being sourced
twice in the test script, which can re-apply its channel mutations and create
duplicate state. Update the logic in the test setup around source
./ci/use_conda_packages_from_prs.sh so it is only sourced once, and reuse the
variables it sets for the rest of the script instead of re-invoking it.
|
Closing since 3052 has been merged. |
4 participants
No description provided.