PiPNN 5/6: add dedicated benchmark pipelines - #1294
Conversation
28f253a to
4834da3
Compare
4834da3 to
79a9407
Compare
79a9407 to
4f40c35
Compare
4f40c35 to
05933e6
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The pipnn feature definition likely does not enable the optional diskann-disk dependency (breaking --features pipnn builds) and should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds PiPNN-specific benchmark entry points and fixtures so benchmarks can build PiPNN graphs (in-memory) and PiPNN disk indexes through the production disk builder, with integration tests validating the selected algorithm and basic results shape.
Changes:
- Add a dedicated in-memory PiPNN graph build pipeline in
diskann-benchmarkand route graph-index benchmarks to it when requested. - Extend disk-index benchmark input/configuration to support selecting
BuildAlgorithm(Vamana vs PiPNN), and pass PiPNN through the production disk build pipeline. - Add PiPNN benchmark example JSONs and integration tests gated on the
pipnnfeature.
File summaries
| File | Description |
|---|---|
| diskann-benchmark/src/main.rs | Extends CLI integration tests to cover PiPNN graph/disk benchmark examples. |
| diskann-benchmark/src/inputs/graph_index.rs | Adds optional PiPNN build algorithm selection for graph-index builds (feature-gated). |
| diskann-benchmark/src/inputs/disk.rs | Adds alpha, makes quantization optional, and adds build-algorithm selection/validation for disk-index builds. |
| diskann-benchmark/src/index/build.rs | Implements the dedicated PiPNN in-memory build pipeline and a unit test for start strategy handling. |
| diskann-benchmark/src/index/benchmarks.rs | Dispatches graph-index builds to PiPNN vs incremental insertion based on requested algorithm. |
| diskann-benchmark/src/disk_index/build.rs | Plumbs BuildAlgorithm into disk index build parameters (Vamana vs PiPNN). |
| diskann-benchmark/example/pipnn-graph-index.json | New example config exercising PiPNN graph-index build + TopK search. |
| diskann-benchmark/example/pipnn-disk-index.json | New example config exercising PiPNN disk-index build + search. |
| diskann-benchmark/Cargo.toml | Adds the pipnn feature and optional dependency on diskann-pipnn. |
| Cargo.lock | Adds diskann-pipnn to the workspace lockfile dependency graph. |
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 2
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| ] | ||
|
|
||
| # Enable PiPNN graph construction. | ||
| pipnn = ["dep:diskann-pipnn", "diskann-disk/pipnn"] |
| data.row_iter() | ||
| .enumerate() | ||
| .min_by(|(_, left), (_, right)| { | ||
| distance | ||
| .evaluate_similarity(start, left) | ||
| .total_cmp(&distance.evaluate_similarity(start, right)) | ||
| }) | ||
| .map(|(index, _)| index) |
05933e6 to
b085972
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The PiPNN benchmark path has at least one confirmed config/behavior mismatch (silently ignoring multi_insert) and an avoidable performance issue in start-point source mapping that can skew benchmark timings.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
diskann-benchmark/src/index/build.rs:160
- The PiPNN start-point source selection scans the dataset twice when an exact byte match is not found (
positionthenmin_by). For largestart_point_strategysample counts this adds avoidable O(2*N) work per start vector and skews the benchmark timing. Consider doing a single pass that checks for an exact match and otherwise tracks the best (minimum) distance as you iterate once.
let start_sources = start_points
.row_iter()
.map(|start| {
let bytes: &[u8] = bytemuck::cast_slice(start);
data.row_iter()
diskann-benchmark/src/index/benchmarks.rs:240
- When
build_algorithmis set toPiPNN, the benchmark bypasses the incremental builder and ignoresmulti_insertif it was provided in the input. This can silently mislead users into thinking multi-insert settings are applied to PiPNN builds. It would be safer to rejectmulti_insertfor PiPNN with a clear error.
let result = match build.build_algorithm() {
diskann_disk::BuildAlgorithm::PiPNN(parameters) => {
let data =
Arc::new(datafiles::load_dataset(datafiles::BinFile(build.data()))?);
build::pipnn_build(data, build, parameters)
- Files reviewed: 9/10 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
b085972 to
b5b5cad
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The pipnn feature configuration and PiPNN dispatch path have correctness issues that can cause build failures and silently ignored user configuration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
diskann-benchmark/Cargo.toml:88
- The
pipnnfeature enablesdiskann-disk/pipnnbut does not enable the optionaldiskann-diskdependency itself. Since the benchmark code behindcfg(feature = "pipnn")referencesdiskann_disk::BuildAlgorithm(even outside thedisk-indexfeature), building with--features pipnncan fail due todiskann-disknot being activated.
# Enable PiPNN graph construction.
pipnn = ["dep:diskann-pipnn", "diskann-disk/pipnn"]
diskann-benchmark/src/index/benchmarks.rs:241
- When
build_algorithmselectsPiPNN, this path bypassesrun_build/single_or_multi_insert, so anymulti_insertconfiguration in the input is silently ignored. That can mislead users who expect batched insertion settings to take effect; it should be rejected explicitly for PiPNN builds.
diskann_disk::BuildAlgorithm::PiPNN(parameters) => {
let data =
Arc::new(datafiles::load_dataset(datafiles::BinFile(build.data()))?);
build::pipnn_build(data, build, parameters)
}
- Files reviewed: 9/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
b5b5cad to
7872932
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The PiPNN benchmark build path currently ignores multi_insert settings without rejecting them, which can lead to silently-misconfigured benchmark runs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
diskann-benchmark/src/index/build.rs:154
- When
build_algorithmselects PiPNN, themulti_insertsetting fromIndexBuildis silently ignored (PiPNN always runs the one-shot batch build). This can lead to confusing configs where the user thinks multi-insert is active but it has no effect; it should be rejected explicitly for PiPNN builds.
use anyhow::Context;
let npoints = data.nrows();
let dimensions = data.ncols();
let metric = input.distance().into();
let graph = input.try_as_config()?.build()?;
- Files reviewed: 9/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
6656dda to
a4b3c1a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and coverage gaps (notably the pipnn feature not enabling the optional diskann-disk dependency, plus missing assertions and some build-path hardening) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (4)
diskann-benchmark/src/index/build.rs:194
- The fallback path that maps a start vector to the nearest real row computes distances repeatedly inside
min_by(re-evaluating the left/right distances for every comparison). This inflates the start-point mapping cost (especially when multiple start vectors are requested) and can skew the benchmark's build timing. Compute each row's distance once and then take the min over the precomputed scalar distances.
.or_else(|| {
data.row_iter()
.enumerate()
.min_by(|(_, left), (_, right)| {
distance
diskann-benchmark/src/index/build.rs:152
diskann_async::new_indexwill internally castmax_pointstou32when constructing the provider's start-point range. For datasets larger thanu32::MAX, this can truncate silently and produce inconsistent behavior; the lateru32::try_from(id)check happens only after the provider has already been allocated. Fail fast before allocating the provider by validatingnpoints <= u32::MAX.
let npoints = data.nrows();
let dimensions = data.ncols();
let metric = input.distance().into();
let graph = input.try_as_config()?.build()?;
let pool = rayon::ThreadPoolBuilder::new()
diskann-benchmark/src/main.rs:334
pipnn_disk_index_integrationcurrently only checks that the run completes. This would still pass if the disk build accidentally fell back to a non-PiPNN path (the key regression called out in the PR description). Sincerun_integration_testnow returns the parsed results, assert that the build kind isPiPNN(and optionally other key fields) to make this an actual end-to-end contract test.
fn pipnn_disk_index_integration() {
let mut raw = value_from_file(&example_directory().join("pipnn-disk-index.json"));
let directory = tempfile::tempdir().unwrap();
let save_path = directory.path().join("pipnn_disk_index");
*raw.pointer_mut("/jobs/0/content/source/save_path")
.expect("PiPNN disk example must declare save_path") =
Value::String(save_path.to_string_lossy().into_owned());
run_integration_test(raw);
diskann-benchmark/src/inputs/graph_index.rs:686
- When
build_algorithmisPiPNN, the benchmark bypasses the incremental insert path entirely, so fields likemulti_insertandinsert_retrybecome no-ops even if they are present in JSON. That makes the input schema ambiguous and can hide misconfiguration. Consider rejecting incompatible knobs during input validation whenbuild_algorithmis explicitlyPiPNN(e.g., error ifmulti_insert/insert_retryare set).
#[cfg(feature = "pipnn")]
#[serde(default)]
build_algorithm: diskann_disk::BuildAlgorithm,
}
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| # Enable PiPNN graph construction. | ||
| pipnn = ["diskann/pipnn", "diskann-disk/pipnn"] |
There was a problem hiding this comment.
🟢 Ready to approve
The new PiPNN benchmark routes are consistently gated/validated, covered by integration tests/examples, and the only feedback is a small maintainability nit.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
diskann-benchmark/src/inputs/graph_index.rs:753
StartPointStrategy::count()is always nonzero today (it returnsNonZeroUsize::get()for sampled strategies and1for the single-start strategies), sounwrap_or(NonZeroUsize::MIN)is dead code and would silently mask a future bug if a zero-count strategy is ever added. Prefer keeping this strict and failing loudly if the invariant is broken.
let frozen_points =
NonZeroUsize::new(self.start_point_strategy.count()).unwrap_or(NonZeroUsize::MIN);
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
a4b3c1a to
3c0bb25
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new pipnn feature configuration likely does not reliably enable the optional diskann-disk dependency, and there are also concrete maintainability/performance fixes needed in the new PiPNN path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
diskann-benchmark/Cargo.toml:87
pipnnenablesdiskann-disk/pipnnbut does not explicitly enable the optionaldiskann-diskdependency. Since thepipnn-gated Rust code importsdiskann_disk::*,cargo build/test --features pipnncan fail if the optional dep is not activated.
# Enable PiPNN graph construction.
pipnn = ["diskann/pipnn", "diskann-disk/pipnn"]
diskann-benchmark/src/index/build.rs:197
- The
min_bycomparator recomputesevaluate_similarity(start, row)for both sides on every comparison, roughly doubling the amount of distance work when mapping synthetic start vectors to their nearest real row. You can compute the score once per row and thenmin_bythe cached scores to cut this overhead ~2x.
.min_by(|(_, left), (_, right)| {
distance
.evaluate_similarity(start, left)
.total_cmp(&distance.evaluate_similarity(start, right))
})
diskann-benchmark/src/inputs/graph_index.rs:753
StartPointStrategy::count()is already non-zero for every variant (it usesNonZeroUsizefor sampled strategies and returns 1 for the others), sounwrap_or(NonZeroUsize::MIN)can silently mask an invariant break. Usingexpecthere both documents the invariant and avoids silently changing behavior if a zero-count strategy is ever added.
let frozen_points =
NonZeroUsize::new(self.start_point_strategy.count()).unwrap_or(NonZeroUsize::MIN);
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Forward the benchmark feature through diskann and keep the dedicated batch lifecycle without a direct implementation-crate dependency.
3c0bb25 to
ce4b316
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The pipnn feature currently doesn’t enable the optional diskann-disk dependency, so --features pipnn will fail to compile where diskann_disk::* is referenced.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (4)
diskann-benchmark/Cargo.toml:88
- The
pipnnfeature enablesdiskann-disk/pipnnbut does not enable the optionaldiskann-diskdependency itself. With--features pipnn(withoutdisk-index),cfg(feature = "pipnn")code in this crate referencesdiskann_disk::*and will fail to compile becausediskann-diskis still disabled.
# Enable PiPNN graph construction.
pipnn = ["diskann/pipnn", "diskann-disk/pipnn"]
diskann-benchmark/src/index/build.rs:188
start_sourcescurrently scans the full dataset twice for every start vector (firstposition, then a second full pass formin_bywhen the vector isn't found). For start strategies with many start points, this becomes a large, avoidable O(2·N·S) pass over the dataset.
let start_sources = start_points
.row_iter()
.map(|start| {
let bytes: &[u8] = bytemuck::cast_slice(start);
data.row_iter()
diskann-benchmark/src/inputs/graph_index.rs:753
StartPointStrategy::count()is always non-zero today (it returns 1 for single-start variants and usesNonZeroUsizefor multi-start variants). Falling back toNonZeroUsize::MINwould silently mask any future regression to 0 and can create a frozen-slot/start-vector count mismatch later in the pipeline.
let frozen_points =
NonZeroUsize::new(self.start_point_strategy.count()).unwrap_or(NonZeroUsize::MIN);
diskann-benchmark/src/index/build.rs:206
- This comment says the frozen start slot "carries the chosen source vector", but
DefaultProvider::set_start_pointsstores the start vector into the frozen slot (which can be synthetic for strategies like Medoid/RandomVectors). The source row ID is only used to choose which real row's adjacency to mirror into the slot.
// A frozen start slot carries the chosen source vector, so expanding it
// must expose exactly that real source's outgoing row. Prepending the source
// ID would consume one degree slot and discard a graph edge, changing every
// search from the graph produced by the core builder.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
PiPNN constructs all real-point adjacency before a searchable provider exists. Reusing the incremental Vamana benchmark lifecycle would measure a different algorithm and could accidentally run insertion after PiPNN selection. This PR adds dedicated graph-index and disk-index benchmark routes.
The graph route measures batch adjacency construction plus installation into a searchable provider. The disk route exercises the production builder from #1291.
Concepts
A searchable graph provider owns vectors, real-point adjacency, and start/frozen slots used to enter the graph. Start/frozen IDs are not rows returned by the PiPNN core.
BuildStatsmeasures the existing algorithm/install timing boundary;/usr/bin/timeremains the process-wide wall/RSS oracle.Code map
diskann-benchmark/src/inputs/{graph_index,disk}.rsdeserialize PiPNN into productionBuildAlgorithmparameters.index/build.rs::pipnn_buildcreates the requested Rayon pool, callsdiskann::graph::pipnn, then installs the complete graph into a provider.index/benchmarks.rsselects the dedicated batch route instead of incremental insertion.disk_index/build.rspasses PiPNN to the productiondiskann-diskbuilder; no benchmark-only serializer exists.main.rscarries algorithm selection through CLI registration/integration tests.example/pipnn-{graph,disk}-index.jsonare runnable inputs for both routes.pipnnforwards todiskann/pipnnanddiskann-disk/pipnn; no direct implementation-crate dependency exists.End-to-end flow
Graph index: parse input → create caller-owned Rayon pool → build all real-point adjacency → resolve start strategy → allocate provider → install vectors and real rows → populate frozen start slots with degree-bounded edges to mapped real IDs → run existing search benchmark.
Disk index: parse input → pass explicit PiPNN to production disk builder → use #1291 core adapter and common serialization/layout → open and benchmark the resulting disk index through the existing path.
Invariants and boundaries
BuildStatsincludes batch construction and installation but excludes provider allocation to match existing comparison policy.BuildStats.Review path
pipnn_buildin lifecycle order: adjacency, start resolution, provider allocation, vector/row installation, frozen slots.diskann-disk; confirm no benchmark-only format/provider policy.Validation
cargo test -p diskann-benchmark --features pipnnand all-target Clippy pass.Stack relation
Stack 5/6. Depends on #1291 production disk integration. The graph route calls #1290 directly; the disk route intentionally calls #1291. #1295 extends both routes with optional HashPrune configuration.
Stack 5/6: #1291 → #1295