[DO NOT MERGE] Test RAFT 3052#2227
Open
divyegala wants to merge 6 commits into
Open
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR integrates two new CI helper scripts that fetch PR-built conda channels and wheels, then sources them across multiple build and test workflows to use the downloaded artifacts during environment initialization and dependency resolution. ChangesPR artifact sourcing for CI workflows
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 8
🧹 Nitpick comments (4)
ci/use_wheels_from_prs.sh (1)
1-4: ⚡ Quick winAdd bash strict mode for safer error handling.
The script lacks bash strict mode flags (
set -e,set -u,set -o pipefail), which would catch unhandled errors, undefined variables, and pipeline failures automatically. This is a best practice for CI scripts to prevent silent failures.♻️ Proposed addition of strict mode
#!/bin/bash # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION. # SPDX-License-Identifier: Apache-2.0 +set -euo pipefail + # initialize PIP_CONSTRAINT🤖 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_wheels_from_prs.sh` around lines 1 - 4, Add bash "strict mode" to the top of the script: after the existing shebang in ci/use_wheels_from_prs.sh enable safe failure handling by adding set -euo pipefail (or equivalent flags) so the script exits on errors, treats unset variables as errors, and fails pipelines on first failed command; place this immediately after the #! /bin/bash line so functions and commands like those in this script run under strict mode.ci/use_conda_packages_from_prs.sh (1)
1-4: ⚡ Quick winAdd bash strict mode for safer error handling.
The script lacks bash strict mode flags (
set -e,set -u,set -o pipefail), which would catch unhandled errors, undefined variables, and pipeline failures automatically. This is a best practice for CI scripts to prevent silent failures.♻️ Proposed addition of strict mode
#!/bin/bash # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION. # SPDX-License-Identifier: Apache-2.0 +set -euo pipefail + # download CI artifacts🤖 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 - 4, The script currently lacks bash strict mode — add strict-mode flags immediately after the shebang in ci/use_conda_packages_from_prs.sh (i.e., enable errexit, nounset, and pipefail) to stop on errors, catch undefined variables, and fail on pipeline errors; update any affected functions or code paths (e.g., any logic in the script that assumes unset variables or non-failing pipelines) to handle those cases explicitly so the new strict mode (set -euo pipefail) doesn’t break expected behavior.ci/test_wheel_cuvs.sh (2)
32-32: ⚡ Quick winAdd GPU availability check before running tests.
The script runs pytest without verifying GPU availability. As per coding guidelines, CI test scripts should check GPU availability before executing tests to provide meaningful error messages and avoid unclear failures.
Consider adding a GPU check before line 32:
# Verify GPU availability before tests if ! nvidia-smi > /dev/null 2>&1; then echo "Error: No GPU detected. Tests require GPU availability." exit 1 fi🤖 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_wheel_cuvs.sh` at line 32, Add a GPU availability check before the pytest invocation in ci/test_wheel_cuvs.sh so tests don't run when no GPU is present: call nvidia-smi (or similar GPU probe) and if it fails, print a clear error like "Error: No GPU detected. Tests require GPU availability." and exit with non‑zero status; place this check immediately before the pytest line that runs "python -m pytest ./python/cuvs/cuvs/tests" so the script aborts early when GPU is unavailable.Source: Coding guidelines
12-18:${PIP_CONSTRAINT}won’t lose PR wheel constraints fromrapids-generate-pip-constraints[minor note]
ci/use_wheels_from_prs.shcreates PR artifact constraints viacat > "${PIP_CONSTRAINT}", andrapids-generate-pip-constraintsappends to the file (usestee -a), so the call atci/test_wheel_cuvs.sh:18won’t overwrite the PR constraints.
Optional:ci/use_wheels_from_prs.shstill clobbers any pre-existing${PIP_CONSTRAINT}content (fromrapids-init-pip)—use>>instead ofcat >if preserving base constraints is desired.🤖 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_wheel_cuvs.sh` around lines 12 - 18, The PR artifact constraints written in ci/use_wheels_from_prs.sh use a destructive redirect (cat > "${PIP_CONSTRAINT}") which clobbers any existing base constraints, and since rapids-generate-pip-constraints (called in ci/test_wheel_cuvs.sh) appends with tee -a the current sequence will not overwrite PR constraints; update ci/use_wheels_from_prs.sh to append (use >> "${PIP_CONSTRAINT}" instead of >) when writing PR wheel constraints so pre-existing constraints from rapids-init-pip are preserved, or alternatively change the write to be conditional/merge-aware if overwriting is truly required.
🤖 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/build_cpp.sh`:
- Around line 26-27: The RATTLER_CHANNELS computation happens before the helper
script is sourced, so PR-prepended channels from
ci/use_conda_packages_from_prs.sh are not applied; move the line sourcing that
helper (source ./ci/use_conda_packages_from_prs.sh) to occur before the rattler
channel computation (before the code that sets/uses RATTLER_CHANNELS and before
source rapids-rattler-channel-string) so the helper can modify environment
variables used by the rattler-build call.
In `@ci/build_python.sh`:
- Around line 23-24: Move the sourcing of the PR-channel helper so it runs
before sourcing rapids-rattler-channel-string: source
./ci/use_conda_packages_from_prs.sh must be executed prior to source
rapids-rattler-channel-string so that RATTLER_CHANNELS (and any arrays it relies
on) are populated before the script builds the channel string used by the
rattler-build commands; update the order in the script to source the helper
first, leaving the existing variable names and rattler-build invocations
unchanged.
In `@ci/use_conda_packages_from_prs.sh`:
- Around line 27-30: The loop that runs "conda config --system --add channels"
over _channel lacks error handling; ensure conda exists (check command -v conda
or similar) before the loop and after each "conda config --system --add channels
\"${_channel}\"" invocation test its exit status, and if it fails print a clear
error message that includes the failing _channel and the command output, then
exit with a non‑zero status; update the loop around the _channel variable and
the conda config command to perform these checks and fail fast on error.
- Around line 6-7: The assignments to LIBRAFT_CHANNEL and RAFT_CHANNEL using
rapids-get-pr-artifact lack error checking; after calling rapids-get-pr-artifact
(the commands that set LIBRAFT_CHANNEL and RAFT_CHANNEL) check the command exit
status and that the variables are non-empty/valid, and if either failed print a
clear CI error message including which artifact failed and the attempted command
(or returned value) and exit with a non-zero status so the job fails fast
instead of passing empty/invalid channel values into the conda channel list.
In `@ci/use_wheels_from_prs.sh`:
- Line 6: The script currently runs "source rapids-init-pip" with no checks so
failures or a missing script silently break later uses of PIP_CONSTRAINT; modify
the CI script to first verify the file exists and is readable (or test
sourcing), then attempt to source "rapids-init-pip" and immediately check that
PIP_CONSTRAINT is set (non-empty); if the file is missing or sourcing fails or
PIP_CONSTRAINT is empty, print a clear error message mentioning
"rapids-init-pip" and "PIP_CONSTRAINT" and exit with a non-zero status so
downstream steps do not proceed.
- Line 8: Check that RAPIDS_CUDA_VERSION is set before calling
rapids-wheel-ctk-name-gen and handle command failures: if RAPIDS_CUDA_VERSION is
empty, print a clear error and exit non-zero; when running
rapids-wheel-ctk-name-gen to assign RAPIDS_PY_CUDA_SUFFIX, capture its exit
status and on failure print the command's stderr (or a descriptive message
including the value of RAPIDS_CUDA_VERSION) and exit non-zero so downstream
wheel-name usages (the places that reference RAPIDS_PY_CUDA_SUFFIX) don't
proceed with an empty value.
- Around line 18-21: Before writing to PIP_CONSTRAINT ensure each wildcard
resolves to exactly one wheel: glob LIBRAFT_WHEELHOUSE/libraft_*.whl and
RAFT_WHEELHOUSE/pylibraft_*.whl, check the match count for each (use the
resolved filename variables, e.g., LIBRAFT_WHEEL and RAFT_WHEEL), and if a
pattern yields zero or multiple files emit a clear error and exit non‑zero; only
after validating assign the single matched paths and write the constraint lines
using those validated filenames (with file:// and absolute path) into
PIP_CONSTRAINT.
- Around line 11-16: The two assignments to LIBRAFT_WHEELHOUSE and
RAFT_WHEELHOUSE call rapids-get-pr-artifact without checking success; update the
script so each rapids-get-pr-artifact invocation is executed while capturing its
output and then validate success (check the command exit status and that the
resulting variable is non-empty/points to an existing wheel path). If a download
fails, print a clear error that includes which artifact (LIBRAFT_WHEELHOUSE or
RAFT_WHEELHOUSE) and the underlying command, and exit with a non-zero status.
Reference the exact variables LIBRAFT_WHEELHOUSE, RAFT_WHEELHOUSE and the
rapids-get-pr-artifact invocations when adding the checks and error messages.
---
Nitpick comments:
In `@ci/test_wheel_cuvs.sh`:
- Line 32: Add a GPU availability check before the pytest invocation in
ci/test_wheel_cuvs.sh so tests don't run when no GPU is present: call nvidia-smi
(or similar GPU probe) and if it fails, print a clear error like "Error: No GPU
detected. Tests require GPU availability." and exit with non‑zero status; place
this check immediately before the pytest line that runs "python -m pytest
./python/cuvs/cuvs/tests" so the script aborts early when GPU is unavailable.
- Around line 12-18: The PR artifact constraints written in
ci/use_wheels_from_prs.sh use a destructive redirect (cat > "${PIP_CONSTRAINT}")
which clobbers any existing base constraints, and since
rapids-generate-pip-constraints (called in ci/test_wheel_cuvs.sh) appends with
tee -a the current sequence will not overwrite PR constraints; update
ci/use_wheels_from_prs.sh to append (use >> "${PIP_CONSTRAINT}" instead of >)
when writing PR wheel constraints so pre-existing constraints from
rapids-init-pip are preserved, or alternatively change the write to be
conditional/merge-aware if overwriting is truly required.
In `@ci/use_conda_packages_from_prs.sh`:
- Around line 1-4: The script currently lacks bash strict mode — add strict-mode
flags immediately after the shebang in ci/use_conda_packages_from_prs.sh (i.e.,
enable errexit, nounset, and pipefail) to stop on errors, catch undefined
variables, and fail on pipeline errors; update any affected functions or code
paths (e.g., any logic in the script that assumes unset variables or non-failing
pipelines) to handle those cases explicitly so the new strict mode (set -euo
pipefail) doesn’t break expected behavior.
In `@ci/use_wheels_from_prs.sh`:
- Around line 1-4: Add bash "strict mode" to the top of the script: after the
existing shebang in ci/use_wheels_from_prs.sh enable safe failure handling by
adding set -euo pipefail (or equivalent flags) so the script exits on errors,
treats unset variables as errors, and fails pipelines on first failed command;
place this immediately after the #! /bin/bash line so functions and commands
like those in this script run under strict mode.
🪄 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: c1e834f9-ec25-4226-b143-c34fdd915024
📒 Files selected for processing (12)
ci/build_cpp.shci/build_go.shci/build_java.shci/build_python.shci/build_rust.shci/build_wheel_cuvs.shci/build_wheel_libcuvs.shci/test_cpp.shci/test_python.shci/test_wheel_cuvs.shci/use_conda_packages_from_prs.shci/use_wheels_from_prs.sh
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/rapids-get-pr-artifact`:
- Around line 46-49: The script parses --pure/--arch into pure_flag/arch_flag
and arch_arg but never injects those into RAPIDS_ARTIFACT_NAME_ARGS before
invoking rapids-artifact-name, so artifact lookup can be wrong; update the
argument assembly logic (before the rapids-artifact-name call) to append
"--pure" when pure_flag is set and append "--arch" plus the value in arch_arg
when arch_flag is set, ensuring you preserve existing RAPIDS_ARTIFACT_NAME_ARGS
and proper quoting so rapids-artifact-name receives the flags.
- Around line 35-41: The --arch and --pkg_name parsing is brittle under set -euo
pipefail and the pure_flag/arch_flag are never used when composing
RAPIDS_ARTIFACT_NAME_ARGS; update the option parsing in
ci/rapids-get-pr-artifact so that (--arch) checks that "$2" exists and is not
another flag (error and exit if missing), assign arch_arg and set arch_flag,
then shift 2 (not shift) to consume the value; do the same validation for
(--pkg_name) assigning pkg_name_arg and shift 2; finally, when building
RAPIDS_ARTIFACT_NAME_ARGS, include logic that uses pure_flag and arch_flag
(e.g., append the appropriate --pure/--arch entries or incorporate
arch_arg/pkg_name_arg into the artifact name construction) so those options
actually affect the computed artifact name.
🪄 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: 2edbe9fa-e408-464a-8da1-bdb3762409a4
📒 Files selected for processing (3)
ci/rapids-get-pr-artifactci/use_conda_packages_from_prs.shci/use_wheels_from_prs.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- ci/use_wheels_from_prs.sh
- ci/use_conda_packages_from_prs.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.