Refactor build scripts to use RAPIDS_BRANCH and remove RAPIDS_VERSION#517
Conversation
bdice
requested review from
jakirkham and
tmckayus
and removed request for
a team
WalkthroughConsolidates version/branch sources: adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Release as update-version.sh
participant VERSION as VERSION
participant RAPIDSBR as RAPIDS_BRANCH
Release->>VERSION: write NEXT_FULL_TAG
Note right of Release: no longer writes to RAPIDS_VERSION
Release->>RAPIDSBR: read (or validate) RAPIDS_BRANCH as branch input
sequenceDiagram
autonumber
participant CI as ci/build_*.sh
participant OldVersion as ./RAPIDS_VERSION
participant NewVersion as ./VERSION
participant BranchFile as ./RAPIDS_BRANCH
CI->>OldVersion: (previous) read & export DEPENDENT_PACKAGE_VERSION
Note right of CI: export removed
CI->>NewVersion: rely on `VERSION` for versioning
CI->>BranchFile: read `RAPIDS_BRANCH` for branch identifier
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bdice
added
non-breaking
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
RAPIDS_BRANCH(1 hunks)ci/build_cpp.sh(0 hunks)ci/build_python.sh(0 hunks)ci/release/update-version.sh(0 hunks)cmake/RAPIDS.cmake(3 hunks)cmake/rapids_config.cmake(2 hunks)conda/recipes/cuopt/recipe.yaml(2 hunks)conda/recipes/libcuopt/recipe.yaml(4 hunks)cpp/CMakeLists.txt(0 hunks)cpp/cmake/thirdparty/get_raft.cmake(2 hunks)
💤 Files with no reviewable changes (4)
- ci/build_cpp.sh
- ci/build_python.sh
- ci/release/update-version.sh
- cpp/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.1, 3.13, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.1, 3.13, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.1, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.1, 3.11, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.1, 3.12, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.1, 3.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.1, 3.11, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.1, 3.12, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-sh-client / 13.0.1, 3.10, amd64, rockylinux8
- GitHub Check: checks / check-style
🔇 Additional comments (11)
conda/recipes/cuopt/recipe.yaml (2)
5-6: ✓ Unified version context setup looks correct.The introduction of
minor_versionderived directly fromversion(viaRAPIDS_PACKAGE_VERSION) is clean and deterministic. The template formula(version | split("."))[:2] | join(".")correctly extracts major.minor components.
62-62: ✓ Dependency version pinning is consistent and intentional.All RAPIDS ecosystem dependencies (cudf, cuvs, pylibraft, rmm, raft-dask, rapids-dask-dependency) are correctly pinned to
minor_version, enabling coordinated minor-version releases while allowing patch-level flexibility. Internal packages (libcuopt, cuopt-mps-parser) use exact version pinning as appropriate.Also applies to: 65-65, 73-73, 76-76, 83-83, 85-85, 86-86, 87-87
conda/recipes/libcuopt/recipe.yaml (3)
6-6: ✓ Context variable refactoring aligns with cuopt recipe.The
minor_versionderivation in libcuopt/recipe.yaml mirrors the unified approach in cuopt/recipe.yaml, ensuring consistent version sourcing across both packages.
66-66: ✓ Dependency pinning consistent with cuopt recipe.libraft-headers and librmm are correctly pinned to
minor_versionacross build/host/run contexts. This matches the pinning strategy in the cuopt recipe.Also applies to: 67-67, 150-150, 159-159
5-6: Verify consistency with broader CMake and build script changes in this PR.These conda recipe changes introduce
minor_versionderived fromRAPIDS_PACKAGE_VERSION. Per the PR objectives, corresponding CMake configuration and other build scripts should also be refactored to use unified version sources (RAPIDS_BRANCH and centralized version variables). Ensure the broader PR changes maintain consistency in how versions are sourced and derived across all build systems.RAPIDS_BRANCH (1)
1-1: LGTM!The branch identifier aligns with the target branch (branch-25.12) and integrates correctly with the new branch handling logic in
cmake/rapids_config.cmake.cmake/RAPIDS.cmake (2)
21-25: LGTM!The relaxed condition now requires at least one of
rapids-cmake-branchorrapids-cmake-version(instead of both), which properly supports the new defaulting logic introduced incmake/rapids_config.cmake.
36-36: LGTM!The default branch naming convention change from
branch-${rapids-cmake-version}torelease/${rapids-cmake-version}aligns with standard RAPIDS repository branching practices.cmake/rapids_config.cmake (3)
28-35: LGTM!The new RAPIDS_BRANCH file reading with proper error handling integrates well with the new branch-based versioning strategy.
37-43: LGTM!The defaulting logic for
rapids-cmake-versionandrapids-cmake-branchprovides sensible fallbacks and properly coordinates with the relaxed requirements incmake/RAPIDS.cmake.
14-26: VERSION file verified—code change is valid.The VERSION file exists at the repository root with the correct
XX.XX.XXformat (currently25.12.00), matching the regex pattern used in the CMake code. The file path reference is correct, and version parsing will work as expected.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
bdice
requested review from
akifcorduk and
kaatish
and removed request for
a team
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Thank you @bdice with this PR, this change will benefit several users.
akifcorduk
left a comment
akifcorduk
left a comment
There was a problem hiding this comment.
Approving cpp changes.
|
|
||
| set(CUOPT_MIN_VERSION_raft "${DEPENDENT_LIB_MAJOR_VERSION}.${DEPENDENT_LIB_MINOR_VERSION}.00") | ||
| set(CUOPT_BRANCH_VERSION_raft "${DEPENDENT_LIB_MAJOR_VERSION}.${DEPENDENT_LIB_MINOR_VERSION}") | ||
| set(CUOPT_MIN_VERSION_raft "${RAPIDS_VERSION_MAJOR}.${RAPIDS_VERSION_MINOR}.00") |
There was a problem hiding this comment.
Do we need _raft suffix now that we are fully consolidated with rapids branching?
There was a problem hiding this comment.
Think we can directly use it.
There was a problem hiding this comment.
This is structured the same way that cuML does it. I'd like to leave this alone. https://github.com/rapidsai/cuml/blob/d6fa8525533d56c0e8345712870b3117cf9bc581/cpp/cmake/thirdparty/get_raft.cmake#L17
However, I did find an additional fix that was needed, and I addressed that in 955c60f.
|
/merge |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/nightly.yaml (1)
25-25: Consider adding error handling for the gh api call.If the
gh apicall fails (e.g., invalid branch or API issues), the workflow continues with an empty or undefined SHA. Consider adding error handling to fail fast or provide a meaningful error message.run: | export DATE=$(date +%F) - export SHA=$(gh api -q '.commit.sha' "repos/nvidia/cuopt/branches/${CUOPT_BRANCH}") + export SHA=$(gh api -q '.commit.sha' "repos/nvidia/cuopt/branches/${CUOPT_BRANCH}") || { echo "Failed to fetch SHA for branch ${CUOPT_BRANCH}"; exit 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/nightly.yaml(1 hunks)cpp/cmake/thirdparty/get_raft.cmake(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/cmake/thirdparty/get_raft.cmake
🔇 Additional comments (1)
.github/workflows/nightly.yaml (1)
16-16: LGTM! Clean alignment with branch-based workflow pattern.The refactoring from version-based matrix input to branch-based (
cuopt_branch: "branch-25.12") is well-executed. Variable scoping is correct, and the branch identifier is properly threaded through the workflow dispatch call. The existing comment (lines 27–28) clearly explains the distinction between workflow source (--ref) and target source code branch (branchinput).Also applies to: 22-35
4 participants
Description
This adopts some CMake and build changes from other RAPIDS repositories. Now that cuOpt uses the same release cadence as RAPIDS, we can simplify the builds and align various scripts more closely with the rest of RAPIDS.
Specifically, this adopts
RAPIDS_BRANCHas the source for fetchingrapids-cmake, and that will allow us to migrate cuOpt to themainbranching strategy.xref:
Checklist
Summary by CodeRabbit