[DO NOT MERGE] Test RAFT 3052#1431
Conversation
|
/ok to test ec7f82a |
ramakrishnap-nv
added
breaking
📝 WalkthroughWalkthroughThe PR adds two CI helper scripts and sources them from existing build and test scripts. One helper configures PR-provided conda channels, and the other configures pip constraints from PR-provided wheels. ChangesPR artifact integration for conda packages and wheels
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci/test_wheel_cuopt.sh (1)
23-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve helper-generated RAFT constraints instead of overwriting them.
Line 23 sources
ci/use_wheels_from_prs.sh, but Lines 26-30 usecat > "${PIP_CONSTRAINT}", which replaces the file and drops the RAFT entries from the helper. That makes this integration a no-op for RAFT PR wheels.Suggested patch
-cat > "${PIP_CONSTRAINT}" <<EOF +cat >> "${PIP_CONSTRAINT}" <<EOF cuopt-${RAPIDS_PY_CUDA_SUFFIX} @ file://$(echo "${CUOPT_WHEELHOUSE}"/cuopt_"${RAPIDS_PY_CUDA_SUFFIX}"-*.whl) cuopt-sh-client @ file://$(echo "${CUOPT_SH_CLIENT_WHEELHOUSE}"/cuopt_sh_client-*.whl) libcuopt-${RAPIDS_PY_CUDA_SUFFIX} @ file://$(echo "${LIBCUOPT_WHEELHOUSE}"/libcuopt_"${RAPIDS_PY_CUDA_SUFFIX}"-*.whl) EOF🤖 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_cuopt.sh` around lines 23 - 30, The script currently overwrites PIP_CONSTRAINT with the cuopt lines, dropping RAFT entries added by sourcing ci/use_wheels_from_prs.sh; change behavior so helper-generated RAFT constraints are preserved by merging rather than replacing: read existing "${PIP_CONSTRAINT}" (created by use_wheels_from_prs.sh), extract and keep any RAFT-related lines, then append or write the cuopt lines (using e.g. >> to append or a temp file that first writes preserved RAFT lines then the cuopt entries) so variables like PIP_CONSTRAINT, CUOPT_WHEELHOUSE, CUOPT_SH_CLIENT_WHEELHOUSE and LIBCUOPT_WHEELHOUSE are used but RAFT entries from the helper script are not lost.
🤖 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 6-7: The script currently hard-codes raft PR id 3052 in the
rapids-get-pr-artifact calls for LIBRAFT_CHANNEL and RAFT_CHANNEL; change these
to read the PR id from a CI input/env var (e.g., RAFT_PR or INPUT_RAFT_PR) and
use that variable in the rapids-get-pr-artifact invocations, providing a
sensible fallback or failing fast with a clear error if the variable is missing;
update the references to LIBRAFT_CHANNEL and RAFT_CHANNEL so they call
rapids-get-pr-artifact with the parameterized PR id instead of the literal 3052.
- Around line 1-5: Add strict shell options to the script by inserting the line
enabling "set -euo pipefail" immediately after the existing shebang
(#!/bin/bash) so the script exits on errors, treats unset variables as failures,
and propagates failures through pipes; do not add custom guards—rely on set -u
for unbound-variable behavior as preferred by the repo.
In `@ci/use_wheels_from_prs.sh`:
- Around line 1-8: This script lacks Bash strict-mode so failures can be masked;
add a strict-mode declaration by inserting set -euo pipefail (and optionally
IFS=$'\n\t' if desired) immediately after the shebang so any failure in sourcing
rapids-init-pip, the rapids-wheel-ctk-name-gen call, or use of
RAPIDS_PY_CUDA_SUFFIX will abort and surface errors; ensure the declaration
appears before the source rapids-init-pip and before any use of
RAPIDS_CUDA_VERSION/RAPIDS_PY_CUDA_SUFFIX.
---
Outside diff comments:
In `@ci/test_wheel_cuopt.sh`:
- Around line 23-30: The script currently overwrites PIP_CONSTRAINT with the
cuopt lines, dropping RAFT entries added by sourcing ci/use_wheels_from_prs.sh;
change behavior so helper-generated RAFT constraints are preserved by merging
rather than replacing: read existing "${PIP_CONSTRAINT}" (created by
use_wheels_from_prs.sh), extract and keep any RAFT-related lines, then append or
write the cuopt lines (using e.g. >> to append or a temp file that first writes
preserved RAFT lines then the cuopt entries) so variables like PIP_CONSTRAINT,
CUOPT_WHEELHOUSE, CUOPT_SH_CLIENT_WHEELHOUSE and LIBCUOPT_WHEELHOUSE are used
but RAFT entries from the helper script are not lost.
🪄 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: afac498b-1894-4763-a55b-b03e4a1e7bad
📒 Files selected for processing (14)
ci/build_cpp.shci/build_docs.shci/build_python.shci/build_wheel_cuopt.shci/build_wheel_libcuopt.shci/test_cpp.shci/test_cpp_memcheck.shci/test_notebooks.shci/test_python.shci/test_self_hosted_service.shci/test_wheel_cuopt.shci/test_wheel_cuopt_server.shci/use_conda_packages_from_prs.shci/use_wheels_from_prs.sh
| #!/bin/bash | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # download CI artifacts |
There was a problem hiding this comment.
Add strict shell options in this new CI helper.
This new script lacks set -euo pipefail, so standalone invocation can miss failures or unset-variable mistakes.
💡 Proposed fix
#!/bin/bash
# SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION.
# SPDX-License-Identifier: Apache-2.0
+
+set -euo pipefailAs per coding guidelines, new scripts under ci/**/*.sh should follow set -euo pipefail hygiene. Based on learnings, this repository prefers relying on set -u unbound-variable behavior instead of custom guards.
🤖 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 - 5, Add strict shell
options to the script by inserting the line enabling "set -euo pipefail"
immediately after the existing shebang (#!/bin/bash) so the script exits on
errors, treats unset variables as failures, and propagates failures through
pipes; do not add custom guards—rely on set -u for unbound-variable behavior as
preferred by the repo.
Sources: Coding guidelines, Learnings
| LIBRAFT_CHANNEL=$(rapids-get-pr-artifact raft 3052 cpp conda) | ||
| RAFT_CHANNEL=$(rapids-get-pr-artifact raft 3052 python conda) |
There was a problem hiding this comment.
Avoid hard-coding a single upstream PR artifact ID.
Line 6 and Line 7 pin artifact retrieval to raft PR 3052, which makes these jobs validate against one fixed external PR and can fail when that artifact is unavailable. Parameterize the PR ID from CI input.
💡 Proposed fix
- LIBRAFT_CHANNEL=$(rapids-get-pr-artifact raft 3052 cpp conda)
- RAFT_CHANNEL=$(rapids-get-pr-artifact raft 3052 python conda)
+ RAFT_PR_ID="${RAFT_PR_ID}"
+ LIBRAFT_CHANNEL=$(rapids-get-pr-artifact raft "${RAFT_PR_ID}" cpp conda)
+ RAFT_CHANNEL=$(rapids-get-pr-artifact raft "${RAFT_PR_ID}" python conda)📝 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) | |
| RAFT_PR_ID="${RAFT_PR_ID}" | |
| LIBRAFT_CHANNEL=$(rapids-get-pr-artifact raft "${RAFT_PR_ID}" cpp conda) | |
| RAFT_CHANNEL=$(rapids-get-pr-artifact raft "${RAFT_PR_ID}" python conda) |
🤖 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 - 7, The script currently
hard-codes raft PR id 3052 in the rapids-get-pr-artifact calls for
LIBRAFT_CHANNEL and RAFT_CHANNEL; change these to read the PR id from a CI
input/env var (e.g., RAFT_PR or INPUT_RAFT_PR) and use that variable in the
rapids-get-pr-artifact invocations, providing a sensible fallback or failing
fast with a clear error if the variable is missing; update the references to
LIBRAFT_CHANNEL and RAFT_CHANNEL so they call rapids-get-pr-artifact with the
parameterized PR id instead of the literal 3052.
| #!/bin/bash | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # initialize PIP_CONSTRAINT | ||
| source rapids-init-pip | ||
|
|
||
| RAPIDS_PY_CUDA_SUFFIX=$(rapids-wheel-ctk-name-gen "${RAPIDS_CUDA_VERSION}") |
There was a problem hiding this comment.
Add strict mode in the helper script entry.
Line 1 introduces a standalone CI helper, but set -euo pipefail is not enabled. If this file is sourced by a non-strict caller, failed artifact lookup or constraint generation can continue silently.
Suggested patch
#!/bin/bash
# SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION.
# SPDX-License-Identifier: Apache-2.0
+set -euo pipefail
+
# initialize PIP_CONSTRAINT
source rapids-init-pipAs per coding guidelines, CI shell scripts should keep set -euo pipefail hygiene, and based on learnings this repo prefers relying on Bash’s built-in unbound-variable handling via set -u.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 6-6: Not following: rapids-init-pip 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/use_wheels_from_prs.sh` around lines 1 - 8, This script lacks Bash
strict-mode so failures can be masked; add a strict-mode declaration by
inserting set -euo pipefail (and optionally IFS=$'\n\t' if desired) immediately
after the shebang so any failure in sourcing rapids-init-pip, the
rapids-wheel-ctk-name-gen call, or use of RAPIDS_PY_CUDA_SUFFIX will abort and
surface errors; ensure the declaration appears before the source rapids-init-pip
and before any use of RAPIDS_CUDA_VERSION/RAPIDS_PY_CUDA_SUFFIX.
Sources: Coding guidelines, Learnings
|
🔔 Hi @anandhkb @divyegala, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ci/test_cpp.sh (1)
14-17: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSource the PR-channel helper only once in this script.
use_conda_packages_from_prs.shis invoked twice back-to-back in the same setup path, which can re-run artifact/channel mutations unnecessarily. Keep a single invocation before dependency generation.Proposed change
CPP_CHANNEL=$(rapids-download-from-github "$(rapids-artifact-name conda_cpp libcuopt cuopt --cuda "$RAPIDS_CUDA_VERSION")") -source ./ci/use_conda_packages_from_prs.sh rapids-logger "Generate C++ testing dependencies" source ./ci/use_conda_packages_from_prs.sh🤖 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 14 - 17, The PR-channel helper is being sourced twice in the same setup flow, which can repeat channel/artifact mutations unnecessarily. Update the setup in ci/test_cpp.sh so use_conda_packages_from_prs.sh is sourced only once before rapids-logger "Generate C++ testing dependencies", keeping the single invocation in the dependency-generation path and removing the duplicate call.ci/test_cpp_memcheck.sh (1)
18-22: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove duplicate helper sourcing in memcheck setup.
This script sources
use_conda_packages_from_prs.shtwice before env generation; a single call is enough and avoids repeated setup side effects.Proposed change
-source ./ci/use_conda_packages_from_prs.sh RAPIDS_VERSION="$(rapids-version)" rapids-logger "Generate C++ testing dependencies" source ./ci/use_conda_packages_from_prs.sh🤖 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_memcheck.sh` around lines 18 - 22, The memcheck setup in the script sources use_conda_packages_from_prs.sh twice before generating C++ testing dependencies, which should be reduced to a single invocation. Remove the redundant source call near the rapids-logger step and keep only one sourcing of use_conda_packages_from_prs.sh in this setup flow so the environment is initialized once.
🤖 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_notebooks.sh`:
- Line 22: The test_notebooks.sh script sources use_conda_packages_from_prs.sh
twice, which repeats its side effects unnecessarily. Remove the duplicate source
call and keep only one sourcing point in the script so the conda
channel/artifact setup runs once, referencing the source invocation in
test_notebooks.sh and the use_conda_packages_from_prs.sh helper.
---
Nitpick comments:
In `@ci/test_cpp_memcheck.sh`:
- Around line 18-22: The memcheck setup in the script sources
use_conda_packages_from_prs.sh twice before generating C++ testing dependencies,
which should be reduced to a single invocation. Remove the redundant source call
near the rapids-logger step and keep only one sourcing of
use_conda_packages_from_prs.sh in this setup flow so the environment is
initialized once.
In `@ci/test_cpp.sh`:
- Around line 14-17: The PR-channel helper is being sourced twice in the same
setup flow, which can repeat channel/artifact mutations unnecessarily. Update
the setup in ci/test_cpp.sh so use_conda_packages_from_prs.sh is sourced only
once before rapids-logger "Generate C++ testing dependencies", keeping the
single invocation in the dependency-generation path and removing the duplicate
call.
🪄 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: 069211da-826e-40f4-a91b-4d419fd9fd56
📒 Files selected for processing (4)
ci/test_cpp.shci/test_cpp_memcheck.shci/test_notebooks.shci/test_python.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- ci/test_python.sh
|
|
||
| ENV_YAML_DIR="$(mktemp -d)" | ||
|
|
||
| source ./ci/use_conda_packages_from_prs.sh |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove duplicate sourcing of use_conda_packages_from_prs.sh.
The script is already sourced at Line 17; sourcing again at Line 22 re-runs artifact/channel side effects (including conda config --system --add channels) unnecessarily. Keep a single source call.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 22-22: 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_notebooks.sh` at line 22, The test_notebooks.sh script sources
use_conda_packages_from_prs.sh twice, which repeats its side effects
unnecessarily. Remove the duplicate source call and keep only one sourcing point
in the script so the conda channel/artifact setup runs once, referencing the
source invocation in test_notebooks.sh and the use_conda_packages_from_prs.sh
helper.
2 participants
No description provided.