Skip to content

Test against RAFT #3005 #2043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
achirkin wants to merge 2 commits into
base: main
Choose a base branch
from
+3 −3
Open

Test against RAFT #3005 #2043

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e40c201
Test against RAFT #3005
achirkin Apr 29, 2026
bf17db0
Update get_raft.cmake
achirkin Apr 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cpp/cmake/thirdparty/get_raft.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@

# Use RAPIDS_VERSION_MAJOR_MINOR from rapids_config.cmake
set(RAFT_VERSION "${RAPIDS_VERSION_MAJOR_MINOR}")
set(RAFT_FORK "rapidsai")
set(RAFT_PINNED_TAG "${rapids-cmake-checkout-tag}")
set(RAFT_FORK "achirkin")
set(RAFT_PINNED_TAG "enh-predictable-resources")
Comment on lines +9 to +10
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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*\(' -C2

Repository: 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 -20

Repository: 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.

Suggested change
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.


function(find_and_configure_raft)
set(oneValueArgs VERSION FORK PINNED_TAG BUILD_STATIC_DEPS ENABLE_NVTX ENABLE_MNMG_DEPENDENCIES CLONE_ON_PIN)
Expand Down
Loading