[DO NOT MERGE] Test RAFT 3052#1431
Conversation
|
/ok to test ec7f82a |
rgsl888prabhu
added
breaking
📝 WalkthroughWalkthroughThe PR adds two CI helper scripts and integrates them into nine existing build and test scripts, enabling the CI pipeline to use conda packages and wheels from PR artifacts. ChangesPR artifact integration for conda packages and wheels
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 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
2 participants
No description provided.