[FEA] Add missing pytests for cuvs-bench algorithms#1460
Open
tarang-jain wants to merge 50 commits into
Open
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. |
tarang-jain
added
feature request
bkarsin
reviewed
Mar 27, 2026
bkarsin
left a comment
bkarsin
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me, just the 1 note about using raft::copy vs. cudaMemcpy.
Contributor
|
Correct- we shoukd not be directly calling cudaMemCpy anywhere. (There are
a few places where we do it, but those need to be corrected, and not
continue to use it.
…On Fri, Mar 27, 2026 at 6:11 PM Ben Karsin ***@***.***> wrote:
***@***.**** commented on this pull request.
Looks good to me, just the 1 note about using raft::copy vs. cudaMemcpy.
------------------------------
In cpp/src/neighbors/detail/vamana/vamana_serialize.cuh
<#1460>
:
> @@ -65,13 +65,20 @@ void serialize_dataset(raft::resources const& res,
const auto* strided_dataset =
dynamic_cast<const cuvs::neighbors::strided_dataset<T, int64_t>*>(dataset);
if (strided_dataset) {
- auto h_dataset =
- raft::make_host_matrix<T, int64_t>(strided_dataset->n_rows(), strided_dataset->dim());
- raft::copy(res,
- raft::make_host_vector_view(h_dataset.data_handle(),
- strided_dataset->n_rows() * strided_dataset->dim()),
- raft::make_device_vector_view(strided_dataset->view().data_handle(),
- strided_dataset->n_rows() * strided_dataset->dim()));
+ auto nrows = strided_dataset->n_rows();
+ auto dim = strided_dataset->dim();
+ auto stride = strided_dataset->stride();
+ auto d_data = strided_dataset->view();
+ auto h_dataset = raft::make_host_matrix<T, int64_t>(nrows, dim);
+ RAFT_CUDA_TRY(cudaMemcpy2DAsync(h_dataset.data_handle(),
Preferable to use raft::copy to direct cudaMemcpy calls?
—
Reply to this email directly, view it on GitHub
<#1460>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJPKYDQXG6ZBNMYP5YPJRL4S34CLAVCNFSM6AAAAACKCEDKK6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DAMRTHE2DIMZSGE>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Contributor
|
Please make a wrapper for this. |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughFixes DiskANN build-parameter parsing and load arg; adds artifact checks, structured logging, and exception-safety in C++ wrappers and harness; adds narrow ChangesDiskANN Benchmark Configuration and Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 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 `@python/cuvs_bench/cuvs_bench/backends/cpp_gbench.py`:
- Around line 276-280: The code currently calls self._gbench_error_message(...)
after persisting/merging benchmark outputs, which allows failed rows to be
saved; move the validation step to before any file write/merge operations so you
raise RuntimeError and abort persistence when error_message is truthy.
Specifically, call self._gbench_error_message(benchmarks, "Build",
benchmark_out) (and the analogous call used for the later stage) immediately
after obtaining benchmark_out and before invoking the merge/write routines (the
functions that persist results), and raise RuntimeError(error_message) to stop
further processing; apply the same reorder/fix for the second occurrence around
the later result-writing block.
In `@python/cuvs_bench/cuvs_bench/tests/test_cli.py`:
- Around line 15-17: The is_arm_cpu() helper currently only checks for
("aarch64", "arm64") and misses common ARM variants like "armv7l", causing false
negatives; update is_arm_cpu() to detect ARM more broadly by checking
platform.machine() (in the is_arm_cpu function) for known values and patterns —
e.g., include "armv7l", "armv6l", and treat any value that starts with "arm"
(case-insensitive) or contains "aarch" as ARM — so replace the current
membership test in is_arm_cpu() with a broader pattern check to reliably detect
ARM architectures.
🪄 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: dc05fe10-338a-47c7-9bbd-dd05f211d6c2
📒 Files selected for processing (4)
cpp/bench/ann/src/common/benchmark.hppcpp/bench/ann/src/cuvs/cuvs_vamana_wrapper.hpython/cuvs_bench/cuvs_bench/backends/cpp_gbench.pypython/cuvs_bench/cuvs_bench/tests/test_cli.py
Comment on lines
+15
to
+17
| def is_arm_cpu(): | ||
| """Check if running on ARM Linux architecture.""" | ||
| return platform.machine() in ("aarch64", "arm64") |
There was a problem hiding this comment.
Broaden ARM architecture detection to avoid false negatives.
The helper currently misses common ARM variants like armv7l, which can incorrectly run DiskANN/Vamana tests on unsupported ARM runners.
Proposed fix
def is_arm_cpu():
"""Check if running on ARM Linux architecture."""
- return platform.machine() in ("aarch64", "arm64")
+ machine = platform.machine().lower()
+ return machine in ("aarch64", "arm64") or machine.startswith("arm")🤖 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 `@python/cuvs_bench/cuvs_bench/tests/test_cli.py` around lines 15 - 17, The
is_arm_cpu() helper currently only checks for ("aarch64", "arm64") and misses
common ARM variants like "armv7l", causing false negatives; update is_arm_cpu()
to detect ARM more broadly by checking platform.machine() (in the is_arm_cpu
function) for known values and patterns — e.g., include "armv7l", "armv6l", and
treat any value that starts with "arm" (case-insensitive) or contains "aarch" as
ARM — so replace the current membership test in is_arm_cpu() with a broader
pattern check to reliably detect ARM architectures.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/bench/ann/src/common/benchmark.hpp (1)
321-369:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftError paths can deadlock worker threads at the load barrier.
Thread 0 can
returnafterSkipWithError(...)before callingload_barrier.arrive(state.threads()), while other threads are blocked inload_barrier.wait(...).💡 Proposed fix direction
progress_barrier load_barrier{}; - if (load_barrier.arrive(1) == 0) { + bool init_failed = false; + if (load_barrier.arrive(1) == 0) { + struct barrier_release { + progress_barrier& b; + int n; + ~barrier_release() { b.arrive(n); } + } release{load_barrier, state.threads()}; + ... } catch (const std::exception& e) { state.SkipWithError("Failed to create an algo: " + std::string(e.what())); - return; + init_failed = true; } ... - load_barrier.arrive(state.threads()); } else { load_barrier.wait(state.threads() * 2); } + if (state.skipped()) { return; }Also applies to: 378-397, 399-423, 425-426
🤖 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 `@cpp/bench/ann/src/common/benchmark.hpp` around lines 321 - 369, The error-handling paths can return while other threads remain blocked on load_barrier.wait, so ensure the barrier is always released on failure: in the block that checks load_barrier.arrive(1) and in the similar blocks around create_algo/create_search_param and state.SkipWithError, wrap the load/create logic in a try/catch (or use RAII) and on any exception or early return call load_barrier.arrive(state.threads()) (or otherwise unblock the waiting threads) before calling state.SkipWithError and returning; apply the same change wherever you call create_algo, create_search_param, current_algo loading, or call state.SkipWithError so the barrier is not left waiting.
🤖 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 `@cpp/bench/ann/src/common/benchmark.hpp`:
- Around line 197-200: The info logs inside the timed build loop (the log_info
call guarded by state.thread_index() == 0 in the bench_build loop around
index.algo/index.name) will skew benchmark results and should be removed from
the critical path; move or change these logs so they do not execute
per-iteration—either relocate the logging outside the timed loop (before it
starts or after it completes) or restrict it to a single non-timed event (e.g.,
only on the first iteration/thread), and apply the same fix to the similar
log_info block at the other occurrence (the block referenced at ~205-209).
In `@cpp/bench/ann/src/diskann/diskann_wrapper.h`:
- Around line 119-123: The code sets configured_build_pq_bytes_ from
param.build_pq_bytes but never copies it into build_pq_bytes_, so
initialize_index_ reads the still-zero build_pq_bytes_ and PQ is disabled;
update the initialization logic to assign configured_build_pq_bytes_ into
build_pq_bytes_ (and likewise ensure any similar pairs around the other
occurrences) before calling initialize_index_ so the configured value is used;
locate the members configured_build_pq_bytes_, build_pq_bytes_ and the function
initialize_index_ (and the other similar block around the second occurrence) and
add the assignment(s) that propagate the configured value to the active
build_pq_bytes_ field.
---
Outside diff comments:
In `@cpp/bench/ann/src/common/benchmark.hpp`:
- Around line 321-369: The error-handling paths can return while other threads
remain blocked on load_barrier.wait, so ensure the barrier is always released on
failure: in the block that checks load_barrier.arrive(1) and in the similar
blocks around create_algo/create_search_param and state.SkipWithError, wrap the
load/create logic in a try/catch (or use RAII) and on any exception or early
return call load_barrier.arrive(state.threads()) (or otherwise unblock the
waiting threads) before calling state.SkipWithError and returning; apply the
same change wherever you call create_algo, create_search_param, current_algo
loading, or call state.SkipWithError so the barrier is not left waiting.
🪄 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: 13368e60-4e10-4549-887a-320f0e7794e5
📒 Files selected for processing (5)
cpp/bench/ann/src/common/benchmark.hppcpp/bench/ann/src/cuvs/cuvs_vamana_wrapper.hcpp/bench/ann/src/diskann/diskann_wrapper.hpython/cuvs_bench/cuvs_bench/backends/cpp_gbench.pypython/cuvs_bench/cuvs_bench/run/data_export.py
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/bench/ann/src/cuvs/cuvs_vamana_wrapper.h
- python/cuvs_bench/cuvs_bench/backends/cpp_gbench.py
Comment on lines
+197
to
+200
| if (state.thread_index() == 0) { | ||
| log_info( | ||
| "bench_build iteration start: algo=%s index=%s", index.algo.c_str(), index.name.c_str()); | ||
| } |
There was a problem hiding this comment.
Hot-loop info logging will skew benchmark measurements.
These log_info calls run inside the timed build loop and can materially inflate latency/throughput numbers (and flood logs). Keep iteration logging off the critical path.
💡 Proposed fix
- if (state.thread_index() == 0) {
- log_info(
- "bench_build iteration start: algo=%s index=%s", index.algo.c_str(), index.name.c_str());
- }
[[maybe_unused]] auto ntx_lap = nvtx.lap();
[[maybe_unused]] auto gpu_lap = gpu_timer.lap(!no_lap_sync);
try {
algo->build(base_set, index_size);
- if (state.thread_index() == 0) {
- log_info("bench_build iteration finish: algo=%s index=%s",
- index.algo.c_str(),
- index.name.c_str());
- }
} catch (const std::exception& e) {Also applies to: 205-209
🤖 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 `@cpp/bench/ann/src/common/benchmark.hpp` around lines 197 - 200, The info logs
inside the timed build loop (the log_info call guarded by state.thread_index()
== 0 in the bench_build loop around index.algo/index.name) will skew benchmark
results and should be removed from the critical path; move or change these logs
so they do not execute per-iteration—either relocate the logging outside the
timed loop (before it starts or after it completes) or restrict it to a single
non-timed event (e.g., only on the first iteration/thread), and apply the same
fix to the similar log_info block at the other occurrence (the block referenced
at ~205-209).
Comment on lines
+119
to
123
| configured_build_pq_bytes_ = param.build_pq_bytes; | ||
| graph_degree_ = param.R; | ||
| L_build_ = param.L_build; | ||
| alpha_ = param.alpha; | ||
| num_threads_ = param.num_threads; |
There was a problem hiding this comment.
build_pq_bytes is configured but never applied to index initialization.
configured_build_pq_bytes_ is set from param.build_pq_bytes, but initialize_index_ reads build_pq_bytes_, which remains 0. This silently disables PQ behavior even when configured.
💡 Proposed fix
configured_build_pq_bytes_ = param.build_pq_bytes;
+ build_pq_bytes_ = param.build_pq_bytes;
graph_degree_ = param.R;
L_build_ = param.L_build;
alpha_ = param.alpha;Also applies to: 145-147
🤖 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 `@cpp/bench/ann/src/diskann/diskann_wrapper.h` around lines 119 - 123, The code
sets configured_build_pq_bytes_ from param.build_pq_bytes but never copies it
into build_pq_bytes_, so initialize_index_ reads the still-zero build_pq_bytes_
and PQ is disabled; update the initialization logic to assign
configured_build_pq_bytes_ into build_pq_bytes_ (and likewise ensure any similar
pairs around the other occurrences) before calling initialize_index_ so the
configured value is used; locate the members configured_build_pq_bytes_,
build_pq_bytes_ and the function initialize_index_ (and the other similar block
around the second occurrence) and add the assignment(s) that propagate the
configured value to the active build_pq_bytes_ field.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.