refactor: switch to rapids-artifact-name for consistent artifact naming - #22327
Conversation
|
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. |
66b9e07 to
f3284f7
Compare
|
/ok to test |
1 similar comment
|
/ok to test |
e508706 to
47e8325
Compare
|
/ok to test |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates CI build, wheel, and test scripts plus PR/test workflows to use rapids-artifact-name and rapids-download-from-github with explicit RAPIDS_CUDA_VERSION for conda channels and wheel artifacts; adds ChangesCI Artifact Naming Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_python.sh`:
- Line 69: RAPIDS_PACKAGE_NAME is built using rapids-artifact-name with --cuda
but missing the CUDA version argument; update the rapids-artifact-name
invocation that sets RAPIDS_PACKAGE_NAME so it passes "$RAPIDS_CUDA_VERSION"
immediately after --cuda (matching other calls), ensuring the function/command
that sets RAPIDS_PACKAGE_NAME includes the CUDA version parameter.
🪄 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: 28de4c7e-b370-41c5-8cc2-bec36ec7a2c1
📒 Files selected for processing (22)
.github/workflows/pr.yamlci/build_cpp.shci/build_docs.shci/build_python.shci/build_python_noarch.shci/build_wheel_cudf.shci/build_wheel_cudf_polars.shci/build_wheel_dask_cudf.shci/build_wheel_libcudf.shci/build_wheel_pylibcudf.shci/cudf_pandas_scripts/pandas-tests/run.shci/cudf_pandas_scripts/run_tests.shci/cudf_pandas_scripts/third-party-integration/test.shci/test_cpp.shci/test_cpp_common.shci/test_cudf_polars_polars_tests.shci/test_java.shci/test_notebooks.shci/test_python_common.shci/test_wheel_cudf.shci/test_wheel_cudf_polars.shci/test_wheel_dask_cudf.sh
💤 Files with no reviewable changes (1)
- ci/test_cpp.sh
47e8325 to
e6c37f4
Compare
|
Actionable comments posted: 0 |
1 similar comment
|
Actionable comments posted: 0 |
4d52f7c to
9a2be9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yaml (1)
445-462:⚠️ Potential issue | 🟡 MinorInclude streaming wheel publish jobs in telemetry-summarize dependencies
.github/workflows/build.yaml’stelemetry-summarizejob includes mostwheel-publish-*jobs inneeds, but omits the newly addedwheel-publish-libcudf-streamingandwheel-publish-cudf-streaming(which exist as separate publish jobs). Add both toneedsif telemetry should cover these 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 @.github/workflows/build.yaml around lines 445 - 462, The telemetry-summarize job is missing the new streaming publish jobs from its needs list; update the telemetry-summarize job (named telemetry-summarize) to include wheel-publish-libcudf-streaming and wheel-publish-cudf-streaming in its needs array so those publish jobs are considered dependencies for telemetry summarization.
🤖 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_wheel_cudf_streaming.sh`:
- Line 18: Quote the LIBCUDF_STREAMING_WHEELHOUSE variable inside the command
substitution so paths with spaces are handled safely; update the echo command
that builds the wheel URL to use "$(echo
"${LIBCUDF_STREAMING_WHEELHOUSE}"/libcudf_streaming_*.whl)" (or otherwise ensure
${LIBCUDF_STREAMING_WHEELHOUSE} is wrapped in double quotes) when constructing
the file:// path so the variable expansion is quoted while preserving the glob
for the wheel filename.
---
Outside diff comments:
In @.github/workflows/build.yaml:
- Around line 445-462: The telemetry-summarize job is missing the new streaming
publish jobs from its needs list; update the telemetry-summarize job (named
telemetry-summarize) to include wheel-publish-libcudf-streaming and
wheel-publish-cudf-streaming in its needs array so those publish jobs are
considered dependencies for telemetry summarization.
🪄 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: 1ba901da-8525-4508-9e5e-bcecad4118ab
📒 Files selected for processing (26)
.github/workflows/build.yaml.github/workflows/pr.yamlci/build_cpp.shci/build_docs.shci/build_python.shci/build_python_noarch.shci/build_wheel_cudf.shci/build_wheel_cudf_polars.shci/build_wheel_cudf_streaming.shci/build_wheel_dask_cudf.shci/build_wheel_libcudf.shci/build_wheel_libcudf_streaming.shci/build_wheel_pylibcudf.shci/cudf_pandas_scripts/pandas-tests/run.shci/cudf_pandas_scripts/run_tests.shci/cudf_pandas_scripts/third-party-integration/test.shci/test_cpp.shci/test_cpp_common.shci/test_cudf_polars_polars_tests.shci/test_java.shci/test_notebooks.shci/test_python_common.shci/test_wheel_cudf.shci/test_wheel_cudf_polars.shci/test_wheel_cudf_streaming.shci/test_wheel_dask_cudf.sh
✅ Files skipped from review due to trivial changes (1)
- ci/test_cpp.sh
🚧 Files skipped from review as they are similar to previous changes (19)
- ci/cudf_pandas_scripts/third-party-integration/test.sh
- .github/workflows/pr.yaml
- ci/build_wheel_cudf_polars.sh
- ci/cudf_pandas_scripts/pandas-tests/run.sh
- ci/build_wheel_dask_cudf.sh
- ci/build_wheel_libcudf.sh
- ci/build_docs.sh
- ci/test_cudf_polars_polars_tests.sh
- ci/build_python.sh
- ci/test_cpp_common.sh
- ci/test_java.sh
- ci/test_wheel_cudf.sh
- ci/test_notebooks.sh
- ci/test_python_common.sh
- ci/build_python_noarch.sh
- ci/build_cpp.sh
- ci/test_wheel_dask_cudf.sh
- ci/build_wheel_cudf.sh
- ci/build_wheel_pylibcudf.sh
2311599 to
556feeb
Compare
…ming fix: test `shared-workflows` post-build-check fix(artifact): set `--arch` manually for pure python packages chore: add env-var for explicit cuda version refactor: use artifact name with new streaming package fix(test.yaml): add package_name to conda-cpp-checks
82576de to
2bc2823
Compare
|
/merge |
|
Looking like a new version of |
… artifacts (#22902) Part of rapidsai/build-planning#270 Followup to #22327 `dask-cuda` and `cudf-polars` wheels use a cuda-suffix, so we build them per-CUDA version, but they are `$ARCH`-agnostic, so we should set the `--arch` to `any` so ARM runners can download artifacts built on x86_64 Authors: - Gil Forsyth (https://github.com/gforsyth) Approvers: - James Lamb (https://github.com/jameslamb) URL: #22902
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