refactor: switch to rapids-artifact-name for consistent artifact naming#2370
Draft
gforsyth wants to merge 6 commits intorapidsai:mainfrom
Draft
refactor: switch to rapids-artifact-name for consistent artifact naming#2370gforsyth wants to merge 6 commits intorapidsai:mainfrom
rapids-artifact-name for consistent artifact naming#2370gforsyth wants to merge 6 commits intorapidsai:mainfrom
Conversation
gforsyth
added
non-breaking
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
Author
|
/ok to test |
|
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 (10)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors CI scripts to standardize artifact resolution by replacing multiple helper functions ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🧹 Nitpick comments (1)
ci/build_python.sh (1)
51-51: Make artifact naming explicit here for full cross-script parity.Line 51 currently relies on implicit defaults (
repoand CUDA value). Passing explicit repo/CUDA like other scripts keeps producer naming deterministic across environments (including forks) and easier to reason about.Proposed consistency tweak
-RAPIDS_PACKAGE_NAME="$(rapids-artifact-name conda_python rmm --stable --cuda)" +RAPIDS_PACKAGE_NAME="$(rapids-artifact-name conda_python rmm rmm --stable --cuda "$RAPIDS_CUDA_VERSION")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/build_python.sh` at line 51, RAPIDS_PACKAGE_NAME is being set via rapids-artifact-name with implicit defaults which can vary across environments; change the call to rapids-artifact-name used to populate RAPIDS_PACKAGE_NAME to pass the explicit --repo and --cuda arguments (matching the values used in the other build scripts) so the artifact name is deterministic across forks/environments; update the invocation around RAPIDS_PACKAGE_NAME="$(rapids-artifact-name conda_python rmm --stable --cuda)" to include the explicit repo and CUDA values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ci/tools/rapids-artifact-name`:
- Around line 107-110: The script currently allows passing '--py' for C++
artifacts but ignores cpython_version, so add a validation alongside the
existing C++ check: when pkg_lang == "cpp" and the Python-specific option (the
'--py' flag / cpython_version variable) is set, call rapids-echo-stderr "'--py'
is only valid for Python package types" and exit 1; place this new conditional
with the existing block that checks stable_flag/pure_flag to ensure callers
cannot pass --py for C++ packages.
- Around line 15-17: The script assumes the helper executable rapids-echo-stderr
is on PATH; fix by resolving it relative to this script and export a variable
(e.g., set RAPIDS_ECHO_STDERR to "$(dirname "$0")/rapids-echo-stderr" or the
appropriate resolved path using $0) so callers that run the script directly find
the helper; then replace all bare invocations of rapids-echo-stderr with
"${RAPIDS_ECHO_STDERR}" in this file and ensure any failure to locate or execute
it emits a clear error via "${RAPIDS_ECHO_STDERR}" or process exit so
RAPIDS_SCRIPT_NAME logs meaningful messages.
---
Nitpick comments:
In `@ci/build_python.sh`:
- Line 51: RAPIDS_PACKAGE_NAME is being set via rapids-artifact-name with
implicit defaults which can vary across environments; change the call to
rapids-artifact-name used to populate RAPIDS_PACKAGE_NAME to pass the explicit
--repo and --cuda arguments (matching the values used in the other build
scripts) so the artifact name is deterministic across forks/environments; update
the invocation around RAPIDS_PACKAGE_NAME="$(rapids-artifact-name conda_python
rmm --stable --cuda)" to include the explicit repo and CUDA 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: bb9989f6-c22b-4cda-9167-64f9f03ec479
📒 Files selected for processing (12)
ci/build_cpp.shci/build_docs.shci/build_python.shci/build_wheel_cpp.shci/build_wheel_python.shci/test_cpp.shci/test_python.shci/test_python_integrations.shci/test_wheel.shci/test_wheel_integrations.shci/tools/rapids-artifact-nameci/tools/rapids-echo-stderr
Comment on lines
+15
to
+17
| set -euo pipefail | ||
| export RAPIDS_SCRIPT_NAME="rapids-artifact-name" | ||
|
|
There was a problem hiding this comment.
Resolve rapids-echo-stderr relative to this script.
All of the validation branches below assume rapids-echo-stderr is on PATH, but that is only true when a caller has already prepended ci/tools. Running ci/tools/rapids-artifact-name ... directly will hit command not found on the first error instead of emitting the intended message.
Suggested fix
set -euo pipefail
export RAPIDS_SCRIPT_NAME="rapids-artifact-name"
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+RAPIDS_ECHO_STDERR="${SCRIPT_DIR}/rapids-echo-stderr"Then replace the later bare rapids-echo-stderr invocations with "${RAPIDS_ECHO_STDERR}" ....
As per coding guidelines, for CI/build scripts: verify proper error handling and meaningful error messages.
📝 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.
Suggested change
| set -euo pipefail | |
| export RAPIDS_SCRIPT_NAME="rapids-artifact-name" | |
| set -euo pipefail | |
| export RAPIDS_SCRIPT_NAME="rapids-artifact-name" | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| RAPIDS_ECHO_STDERR="${SCRIPT_DIR}/rapids-echo-stderr" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ci/tools/rapids-artifact-name` around lines 15 - 17, The script assumes the
helper executable rapids-echo-stderr is on PATH; fix by resolving it relative to
this script and export a variable (e.g., set RAPIDS_ECHO_STDERR to "$(dirname
"$0")/rapids-echo-stderr" or the appropriate resolved path using $0) so callers
that run the script directly find the helper; then replace all bare invocations
of rapids-echo-stderr with "${RAPIDS_ECHO_STDERR}" in this file and ensure any
failure to locate or execute it emits a clear error via "${RAPIDS_ECHO_STDERR}"
or process exit so RAPIDS_SCRIPT_NAME logs meaningful messages.
Comment on lines
+107
to
+110
| if [[ "${pkg_lang}" == "cpp" ]] && (( stable_flag == 1 || pure_flag == 1 )); then | ||
| rapids-echo-stderr "'--stable' and '--pure' are only valid for Python package types" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Reject --py for C++ artifact types.
--py is documented as Python-only, but C++ package types currently accept it and then ignore it because the C++ branch drops cpython_version entirely. That makes a caller typo look successful and can point the job at the wrong artifact.
Suggested fix
-if [[ "${pkg_lang}" == "cpp" ]] && (( stable_flag == 1 || pure_flag == 1 )); then
- rapids-echo-stderr "'--stable' and '--pure' are only valid for Python package types"
+if [[ "${pkg_lang}" == "cpp" ]] && (( py_flag == 1 || stable_flag == 1 || pure_flag == 1 )); then
+ rapids-echo-stderr "'--py', '--stable', and '--pure' are only valid for Python package types"
exit 1
fi📝 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.
Suggested change
| if [[ "${pkg_lang}" == "cpp" ]] && (( stable_flag == 1 || pure_flag == 1 )); then | |
| rapids-echo-stderr "'--stable' and '--pure' are only valid for Python package types" | |
| exit 1 | |
| fi | |
| if [[ "${pkg_lang}" == "cpp" ]] && (( py_flag == 1 || stable_flag == 1 || pure_flag == 1 )); then | |
| rapids-echo-stderr "'--py', '--stable', and '--pure' are only valid for Python package types" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ci/tools/rapids-artifact-name` around lines 107 - 110, The script currently
allows passing '--py' for C++ artifacts but ignores cpython_version, so add a
validation alongside the existing C++ check: when pkg_lang == "cpp" and the
Python-specific option (the '--py' flag / cpython_version variable) is set, call
rapids-echo-stderr "'--py' is only valid for Python package types" and exit 1;
place this new conditional with the existing block that checks
stable_flag/pure_flag to ensure callers cannot pass --py for C++ packages.
…pp artifact name
Contributor
Author
|
/ok to test |
This was referenced Apr 28, 2026
rapids-bot Bot
pushed a commit
to rapidsai/shared-workflows
that referenced
this pull request
Apr 28, 2026
This is part of rapidsai/build-planning#270. I'm rolling out a new `rapids-artifact-name` tool to make our artifact names more consistent - part of the first phase of this is overriding the "default" names that get created by `shared-workflows`. I previously added the capability to override the artifact name from within the ci scripts for the stable ABI work, but that only covered the `python-build` workflows, this extends that capability to the `cpp-build` workflows. Tested this in rapidsai/rmm#2370 along with the vendored version of the script from rapidsai/gha-tools#253 Authors: - Gil Forsyth (https://github.com/gforsyth) Approvers: - Kyle Edwards (https://github.com/KyleFromNVIDIA) URL: #528
rapids-bot Bot
pushed a commit
to rapidsai/gha-tools
that referenced
this pull request
Apr 28, 2026
This is part of rapidsai/build-planning#270 It adds a new tool `rapids-artifact-name` that handles naming more consistently. The arguments are: `rapids-artifact-name package_type package_name repo --cuda [version] --py [version] --stable --pure` `--stable`, `--py [version]`, and `--pure` are mutually exclusive and are only available when naming `python` artifacts. ```sh > rapids-artifact-name conda_cpp librmm rmm --cuda 12.9 rmm_conda_cpp_librmm_amd64_cu12 > rapids-artifact-name wheel_cpp librmm rmm --cuda 12.9 rmm_wheel_cpp_librmm_amd64_cu12 > rapids-artifact-name conda_python pylibcudf cudf --cuda 13.1 --stable cudf_conda_python_pylibcudf_amd64_abi3_cu13 > rapids-artifact-name wheel_python pylibcudf cudf --cuda 13.1 --stable cudf_wheel_python_pylibcudf_amd64_abi3_cu13 > rapids-artifact-name wheel_python dask-cudf cudf --cuda 13.1 --pure cudf_wheel_python_dask-cudf_amd64_pure_cu13 > rapids-artifact-name wheel_python dask-cudf cudf --pure cudf_wheel_python_dask-cudf_amd64_pure ``` I tested this in rapidsai/rmm#2370 by vendoring the script as written there (along with some related small changes to `shared-workflows` to allow overriding artifact names in the conda cpp builds) Authors: - Gil Forsyth (https://github.com/gforsyth) Approvers: - Jake Awe (https://github.com/AyodeAwe) URL: #253
Contributor
Author
|
/ok to test |
20 tasks
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
1 participant
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.
rapids-artifact-namefor all artifact namingrapids-artifact-namefor all artifact namingrapids-artifact-nameto testxref rapidsai/build-planning#270
vendoring in the new bash script to test -- going to open a PR to
gha-toolswith the changes in a bit.Once the changes go in upstream, I'll revert out the vendored version.