Conversation
Test the breaking change in rapidsai/raft#3005
|
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)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe RAFT CMake dependency configuration was changed to fetch from the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/cmake/thirdparty/get_raft.cmake`:
- Around line 9-10: The CMake defaults currently force a personal fork/tag by
setting RAFT_FORK="achirkin" and RAFT_PINNED_TAG="enh-predictable-resources",
which makes all builds use that test fork; change this so defaults point back to
the upstream (e.g., rapidsai and ${rapids-cmake-checkout-tag}) and/or add a
guarded option (e.g., a new CMake option like CUVS_USE_RAFT_TEST_PIN default
OFF) that, when enabled, overrides RAFT_FORK/RAFT_PINNED_TAG to the test values;
update the logic around RAFT_FORK and RAFT_PINNED_TAG to read that option before
assigning the fork/tag so the personal fork is only used when explicitly
opted-in.
🪄 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: f0d5615b-55fc-4318-9224-8440f9c8c60f
📒 Files selected for processing (1)
cpp/cmake/thirdparty/get_raft.cmake
| set(RAFT_FORK "achirkin") | ||
| set(RAFT_PINNED_TAG "enh-predictable-resources") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== get_raft.cmake (relevant lines) =="
target="$(fd -a 'get_raft.cmake$' | head -n1)"
nl -ba "$target" | sed -n '1,120p'
echo
echo "== Default RAFT fork/tag assignments across CMake files =="
rg -n --type=cmake 'set\(RAFT_(FORK|PINNED_TAG)\s+"[^"]+"\)' -C2
echo
echo "== Call sites using find_and_configure_raft(...) =="
rg -n --type=cmake 'find_and_configure_raft\s*\(' -C2Repository: rapidsai/cuvs
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== get_raft.cmake file (first 130 lines) =="
target="$(fd -a 'get_raft.cmake$' | head -n1)"
if [ -z "$target" ]; then
echo "ERROR: get_raft.cmake not found"
exit 1
fi
cat -n "$target" | head -130
echo ""
echo "== All RAFT_FORK and RAFT_PINNED_TAG assignments in CMake files =="
rg -n 'RAFT_(FORK|PINNED_TAG)' -t cmake
echo ""
echo "== Check for any env vars or options controlling RAFT source =="
rg -n 'CUVS.*RAFT|override.*RAFT' -t cmake | head -20Repository: rapidsai/cuvs
Length of output: 4265
🏁 Script executed:
#!/bin/bash
# Check git context: is this a feature branch or main?
echo "== Current branch (if available) =="
git rev-parse --abbrev-ref HEAD 2>/dev/null || echo "Could not determine branch"
echo ""
echo "== Recent commits touching get_raft.cmake =="
git log --oneline -20 cpp/cmake/thirdparty/get_raft.cmake 2>/dev/null | head -10
echo ""
echo "== Check if there are CI config files that reference achirkin/enh-predictable-resources =="
fd -t f '\.(yaml|yml|sh)$' | xargs rg -l 'achirkin|enh-predictable-resources' 2>/dev/null || echo "No matches in CI configs"Repository: rapidsai/cuvs
Length of output: 293
Revert default RAFT source to upstream or gate fork/tag override behind an explicit option.
Lines 9–10 set every default build to use a personal fork/tag (achirkin/enh-predictable-resources). While the file comments (line 59) acknowledge this is for testing, there's no mechanism preventing this from accidentally reaching the main branch. Either revert to rapidsai/${rapids-cmake-checkout-tag} defaults, or gate this behind a CMake option (e.g., CUVS_USE_RAFT_TEST_PIN) to prevent silent reproducibility issues in default builds.
Suggested approach
set(RAFT_VERSION "${RAPIDS_VERSION_MAJOR_MINOR}")
-set(RAFT_FORK "achirkin")
-set(RAFT_PINNED_TAG "enh-predictable-resources")
+set(RAFT_FORK "rapidsai")
+set(RAFT_PINNED_TAG "${rapids-cmake-checkout-tag}")
+
+# Temporary override for targeted testing only
+option(CUVS_USE_RAFT_TEST_PIN "Use temporary RAFT fork/tag override for testing" OFF)
+if(CUVS_USE_RAFT_TEST_PIN)
+ set(RAFT_FORK "achirkin")
+ set(RAFT_PINNED_TAG "enh-predictable-resources")
+endif()📝 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(RAFT_FORK "achirkin") | |
| set(RAFT_PINNED_TAG "enh-predictable-resources") | |
| set(RAFT_FORK "rapidsai") | |
| set(RAFT_PINNED_TAG "${rapids-cmake-checkout-tag}") | |
| # Temporary override for targeted testing only | |
| option(CUVS_USE_RAFT_TEST_PIN "Use temporary RAFT fork/tag override for testing" OFF) | |
| if(CUVS_USE_RAFT_TEST_PIN) | |
| set(RAFT_FORK "achirkin") | |
| set(RAFT_PINNED_TAG "enh-predictable-resources") | |
| endif() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/cmake/thirdparty/get_raft.cmake` around lines 9 - 10, The CMake defaults
currently force a personal fork/tag by setting RAFT_FORK="achirkin" and
RAFT_PINNED_TAG="enh-predictable-resources", which makes all builds use that
test fork; change this so defaults point back to the upstream (e.g., rapidsai
and ${rapids-cmake-checkout-tag}) and/or add a guarded option (e.g., a new CMake
option like CUVS_USE_RAFT_TEST_PIN default OFF) that, when enabled, overrides
RAFT_FORK/RAFT_PINNED_TAG to the test values; update the logic around RAFT_FORK
and RAFT_PINNED_TAG to read that option before assigning the fork/tag so the
personal fork is only used when explicitly opted-in.
1 participant
Test the breaking change in rapidsai/raft#3005