refactor: switch to rapids-artifact-name for consistent artifact naming#2370
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. |
|
/ok to test |
📝 WalkthroughWalkthroughRefactors CI scripts to standardize artifact resolution by replacing helper functions with ChangesCI channel and wheel resolution
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
| 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.
| 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.
| 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.
| 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
|
/ok to test |
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
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
|
/ok to test |
|
@gforsyth Just checking on status here. Is this still needed? |
|
Yeah, going to pick this work back up this week -- got derailed by security audits, but I'll clean this PR up to use the new artifact naming scripts without the vendoring. Oh, oops -- forgot where I was on this -- those changes were already reverted. This is probably ready to go in, but first I'm going to make sure the |
|
/ok to test |
well out-of-date. thanks! |
|
/merge |
Followup to #2370 -- need to update the prefix used to search for wheel artifacts to reflect the new naming scheme. Authors: - Gil Forsyth (https://github.com/gforsyth) Approvers: - Bradley Dice (https://github.com/bdice) URL: #2425
xref #2370 #2475 Also changed the cpp wheel name, so additional change is needed in the publishing step (for now) Authors: - Gil Forsyth (https://github.com/gforsyth) Approvers: - Bradley Dice (https://github.com/bdice) URL: #2426
2 participants
This PR swaps in
rapids-artifact-nameforrapids-package-nameeverywhere, and also removes any legacy named artifacts. All artifacts now follow the same naming convention (and that convention can be updated/expanded from a central location). Part of rapidsai/build-planning#270