Bump versions of packages missed in 26.10 rollover - #23315
Conversation
|
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 (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdates streaming-related RAPIDS dependency constraints from ChangesStreaming dependency update
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
bdice
left a comment
There was a problem hiding this comment.
Please fix the scripting in ci/release/update-version.sh to ensure this does not reoccur.
e51d9f1 to
4450d6a
Compare
Done |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci/release/update-version.sh (1)
147-161: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd regression coverage for the release rewrite entries.
Please add or verify a focused test that confirms both
cudf-streamingandlibcudf-streamingare rewritten correctly, without modifying unrelated dependencies. Run pre-commit, codespell, and CI before merging.As per coding guidelines, “Add unit tests and unit benchmarks for code contributions, and verify that all CI status checks pass before merging.”
🤖 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/release/update-version.sh` around lines 147 - 161, Add focused regression coverage for the release rewrite logic covering both cudf-streaming and libcudf-streaming, asserting each is rewritten correctly while unrelated dependencies remain unchanged. Run pre-commit, codespell, and CI checks before merging.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@ci/release/update-version.sh`:
- Around line 147-161: Add focused regression coverage for the release rewrite
logic covering both cudf-streaming and libcudf-streaming, asserting each is
rewritten correctly while unrelated dependencies remain unchanged. Run
pre-commit, codespell, and CI checks before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 39f81dc1-763f-4647-a64e-20e0fff27ae6
📒 Files selected for processing (9)
ci/release/update-version.shconda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-133_arch-aarch64.yamlconda/environments/all_cuda-133_arch-x86_64.yamldependencies.yamlpython/cudf_polars/pyproject.tomlpython/cudf_streaming/pyproject.tomlpython/libcudf_streaming/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (7)
- conda/environments/all_cuda-129_arch-aarch64.yaml
- conda/environments/all_cuda-133_arch-x86_64.yaml
- conda/environments/all_cuda-129_arch-x86_64.yaml
- python/libcudf_streaming/pyproject.toml
- python/cudf_polars/pyproject.toml
- python/cudf_streaming/pyproject.toml
- dependencies.yaml
|
Also added rewrite for UCXX deps, cargo-culted from a combination of the dask-cuda and ucxx update-versions.sh scripts |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci/release/update-version.sh (1)
186-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign UCXX sed patterns with the main dependency loop.
Two differences from the main
DEPENDENCIESloop (lines 177–184) could cause silent misses or confusion:
- Missing conda selector handling (line 188): The main loop's conda YAML pattern includes
\(\[.*\]\)\{0,1\}to match optional conda selectors (e.g.,[not aarch64]), but the UCXX loop omits it. If a conda selector is ever added to alibucxxorucxxline, the version bump would silently skip that entry.- Inconsistent
\.*in replacement (line 191): The UCXX pyproject.toml replacement uses\.*where the main loop uses.*. Both produce the same literal output in GNU sed, but the inconsistency is unnecessary and could confuse future maintainers.♻️ Proposed alignment
for DEP in "${UCXX_DEPENDENCIES[@]}"; do for FILE in dependencies.yaml conda/environments/*.yaml python/cudf/cudf_pandas_tests/third_party_integration_tests/dependencies.yaml; do - sed_runner "/-.* ${DEP}\(-cu[[:digit:]]\{2\}\)\{0,1\}==/ s/==.*/==${NEXT_UCXX_SHORT_TAG_PEP440}.*,>=0.0.0a0/g" "${FILE}" + sed_runner "/-.* ${DEP}\(-cu[[:digit:]]\{2\}\)\{0,1\}\(\[.*\]\)\{0,1\}==/ s/==.*/==${NEXT_UCXX_SHORT_TAG_PEP440}.*,>=0.0.0a0/g" "${FILE}" done for FILE in python/*/pyproject.toml; do - sed_runner "/\"${DEP}\(-cu[[:digit:]]\{2\}\)\{0,1\}==/ s/==.*\"/==${NEXT_UCXX_SHORT_TAG_PEP440}\.*,>=0.0.0a0\"/g" "${FILE}" + sed_runner "/\"${DEP}\(-cu[[:digit:]]\{2\}\)\{0,1\}==/ s/==.*\"/==${NEXT_UCXX_SHORT_TAG_PEP440}.*,>=0.0.0a0\"/g" "${FILE}" done done🤖 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/release/update-version.sh` around lines 186 - 193, Align the UCXX dependency patterns in the loop over UCXX_DEPENDENCIES with the main DEPENDENCIES loop: add the optional conda selector pattern to the YAML sed match, and use the same `.*` replacement form in the pyproject.toml sed command instead of `\.*`. Preserve the existing dependency names and version replacement behavior.
🤖 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/release/update-version.sh`:
- Line 76: Update the NEXT_UCXX_TAG fetch in update-version.sh to make curl fail
on HTTP errors and handle fetch failures explicitly, emitting a clear error
message before exiting instead of passing an empty or error response to the
downstream Python command.
---
Nitpick comments:
In `@ci/release/update-version.sh`:
- Around line 186-193: Align the UCXX dependency patterns in the loop over
UCXX_DEPENDENCIES with the main DEPENDENCIES loop: add the optional conda
selector pattern to the YAML sed match, and use the same `.*` replacement form
in the pyproject.toml sed command instead of `\.*`. Preserve the existing
dependency names and version replacement behavior.
🪄 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: 6bb53dd3-9cc2-4701-9aa8-ff633aad2473
📒 Files selected for processing (8)
ci/release/update-version.shconda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-133_arch-aarch64.yamlconda/environments/all_cuda-133_arch-x86_64.yamldependencies.yamlpython/cudf_streaming/pyproject.tomlpython/libcudf_streaming/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- python/libcudf_streaming/pyproject.toml
- conda/environments/all_cuda-129_arch-aarch64.yaml
- dependencies.yaml
| NEXT_PATCH=$(echo "$NEXT_FULL_TAG" | awk '{split($0, a, "."); print a[3]}') | ||
| NEXT_SHORT_TAG=${NEXT_MAJOR}.${NEXT_MINOR} | ||
|
|
||
| NEXT_UCXX_TAG="$(curl -s https://version.gpuci.io/rapids/"${NEXT_SHORT_TAG}")" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add error handling for the UCXX version fetch.
curl -s without --fail returns exit 0 on HTTP errors (404, 500), so an empty or error body would propagate to the Python command on line 81 and produce a confusing traceback instead of a clear failure message. As per path instructions, CI/build scripts should have proper error handling and meaningful error messages.
🛡️ Proposed fix
-NEXT_UCXX_TAG="$(curl -s https://version.gpuci.io/rapids/"${NEXT_SHORT_TAG}")"
+NEXT_UCXX_TAG="$(curl -sf https://version.gpuci.io/rapids/"${NEXT_SHORT_TAG}")" \
+ || { echo "Error: Failed to fetch UCXX version from version.gpuci.io" >&2; exit 1; }
+if [ -z "${NEXT_UCXX_TAG}" ]; then
+ echo "Error: Empty UCXX version response from version.gpuci.io" >&2
+ 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.
| NEXT_UCXX_TAG="$(curl -s https://version.gpuci.io/rapids/"${NEXT_SHORT_TAG}")" | |
| NEXT_UCXX_TAG="$(curl -sf https://version.gpuci.io/rapids/"${NEXT_SHORT_TAG}")" \ | |
| || { echo "Error: Failed to fetch UCXX version from version.gpuci.io" >&2; exit 1; } | |
| if [ -z "${NEXT_UCXX_TAG}" ]; then | |
| echo "Error: Empty UCXX version response from version.gpuci.io" >&2 | |
| exit 1 | |
| fi |
🤖 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/release/update-version.sh` at line 76, Update the NEXT_UCXX_TAG fetch in
update-version.sh to make curl fail on HTTP errors and handle fetch failures
explicitly, emitting a clear error message before exiting instead of passing an
empty or error response to the downstream Python command.
Source: Path instructions
|
/merge |
|
There's an error solving a conda env in the xgboost tests: https://github.com/rapidsai/cudf/actions/runs/29611036052/job/88004744495?pr=23315#step:13:21579 I'm not immediately sure how to handle that. |
These were missed by the scripting, I think. Authors: - Lawrence Mitchell (https://github.com/wence-) - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Bradley Dice (https://github.com/bdice) - Matthew Roeschke (https://github.com/mroeschke) URL: NVIDIA#23315
Description
These were missed by the scripting, I think.
Checklist