Skip to content

PiPNN 5/6: add dedicated benchmark pipelines - #1294

Open
weiyaoluo (SeliMeli) wants to merge 16 commits into
pipnn-stack/04-integrationfrom
pipnn-stack/05-benchmark
Open

PiPNN 5/6: add dedicated benchmark pipelines#1294
weiyaoluo (SeliMeli) wants to merge 16 commits into
pipnn-stack/04-integrationfrom
pipnn-stack/05-benchmark

Conversation

@SeliMeli

@SeliMeli weiyaoluo (SeliMeli) commented Jul 29, 2026

Copy link
Copy Markdown

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. BuildStats measures the aggregate batch-build/install interval; PiPNN reports no per-insert percentile because it performs no individual inserts. /usr/bin/time remains the process-wide wall/RSS oracle.

Code map

  1. diskann-benchmark/src/inputs/{graph_index,disk}.rs deserialize PiPNN into production BuildAlgorithm parameters.
  2. index/build.rs::pipnn_build creates the requested Rayon pool, calls diskann::graph::pipnn, then installs the complete graph into a provider.
  3. index/benchmarks.rs selects the dedicated batch route instead of incremental insertion.
  4. disk_index/build.rs passes PiPNN to the production diskann-disk builder; no benchmark-only serializer exists.
  5. main.rs carries algorithm selection through CLI registration/integration tests.
  6. example/pipnn-{graph,disk}-index.json are runnable inputs for both routes.
  7. Benchmark feature pipnn forwards to diskann/pipnn and diskann-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

  • Selecting PiPNN never enters incremental Vamana insertion. A PiPNN-tagged graph input fails closed when the binary lacks the pipnn feature, and dynamic graph jobs reject the batch-only algorithm instead of falling back.
  • Real-point adjacency completes before provider allocation; installation does not feed back into construction.
  • Core row IDs cover real points only. Frozen/start IDs are provider-owned and point to mapped real IDs.
  • Every start strategy must produce a valid nonempty graph entry without exceeding degree.
  • BuildStats includes batch construction and installation but excludes provider allocation to match existing comparison policy; insert_latencies is absent for the batch builder rather than synthesized from aggregate time.
  • Process wall/RSS claims use external process measurement, not BuildStats.
  • Disk route uses production configuration and serialization from PiPNN 4/6: integrate disk build pipeline #1291.

Review path

  1. Follow algorithm deserialization in both input types.
  2. Review pipnn_build in lifecycle order: adjacency, start resolution, provider allocation, vector/row installation, frozen slots.
  3. Check start strategy ID mapping, exact source adjacency, and degree bound.
  4. Follow disk selection into diskann-disk; confirm no benchmark-only format/provider policy.
  5. Check timing scopes independently from process-level measurement.

Validation

  • Private benchmark tests prove the frozen start slot exposes exactly the selected real source row, batch builds omit insert percentiles, feature-disabled requests fail closed, and dynamic jobs reject PiPNN.
  • CLI integration tests cover graph and disk parse/dispatch using existing repository fixtures.
  • Example inputs exercise production PiPNN configuration rather than test-only types.
  • cargo test -p diskann-benchmark --features disk-index,pipnn covers the private lifecycle plus graph and disk CLI routes; all-target Clippy passes.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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-benchmark and 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 pipnn feature.
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.

Comment thread diskann-benchmark/Cargo.toml Outdated
]

# Enable PiPNN graph construction.
pipnn = ["dep:diskann-pipnn", "diskann-disk/pipnn"]
Comment on lines +163 to +170
data.row_iter()
.enumerate()
.min_by(|(_, left), (_, right)| {
distance
.evaluate_similarity(start, left)
.total_cmp(&distance.evaluate_similarity(start, right))
})
.map(|(index, _)| index)
Copilot AI review requested due to automatic review settings July 30, 2026 13:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 (position then min_by). For large start_point_strategy sample 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_algorithm is set to PiPNN, the benchmark bypasses the incremental builder and ignores multi_insert if 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 reject multi_insert for 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.

Copilot AI review requested due to automatic review settings July 31, 2026 04:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 pipnn feature enables diskann-disk/pipnn but does not enable the optional diskann-disk dependency itself. Since the benchmark code behind cfg(feature = "pipnn") references diskann_disk::BuildAlgorithm (even outside the disk-index feature), building with --features pipnn can fail due to diskann-disk not being activated.
# Enable PiPNN graph construction.
pipnn = ["dep:diskann-pipnn", "diskann-disk/pipnn"]

diskann-benchmark/src/index/benchmarks.rs:241

  • When build_algorithm selects PiPNN, this path bypasses run_build/single_or_multi_insert, so any multi_insert configuration 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.

Copilot AI review requested due to automatic review settings August 3, 2026 02:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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_algorithm selects PiPNN, the multi_insert setting from IndexBuild is 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.

Copilot AI review requested due to automatic review settings August 3, 2026 10:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 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 returns NonZeroUsize::get() for sampled strategies and 1 for the single-start strategies), so unwrap_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.

Copilot AI review requested due to automatic review settings August 5, 2026 11:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

  • pipnn enables diskann-disk/pipnn but does not explicitly enable the optional diskann-disk dependency. Since the pipnn-gated Rust code imports diskann_disk::*, cargo build/test --features pipnn can 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_by comparator recomputes evaluate_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 then min_by the 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 uses NonZeroUsize for sampled strategies and returns 1 for the others), so unwrap_or(NonZeroUsize::MIN) can silently mask an invariant break. Using expect here 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.

Copilot AI review requested due to automatic review settings August 5, 2026 12:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 pipnn feature enables diskann-disk/pipnn but does not enable the optional diskann-disk dependency itself. With --features pipnn (without disk-index), cfg(feature = "pipnn") code in this crate references diskann_disk::* and will fail to compile because diskann-disk is still disabled.
# Enable PiPNN graph construction.
pipnn = ["diskann/pipnn", "diskann-disk/pipnn"]

diskann-benchmark/src/index/build.rs:188

  • start_sources currently scans the full dataset twice for every start vector (first position, then a second full pass for min_by when 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 uses NonZeroUsize for multi-start variants). Falling back to NonZeroUsize::MIN would 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_points stores 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.

@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.76471% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.26%. Comparing base (0e8f942) to head (26c078d).

Files with missing lines Patch % Lines
diskann-benchmark/src/inputs/disk.rs 0.00% 12 Missing ⚠️
diskann-benchmark/src/inputs/graph_index.rs 85.71% 5 Missing ⚠️
diskann-benchmark/src/index/streaming/stats.rs 55.55% 4 Missing ⚠️
diskann-benchmark/src/index/benchmarks.rs 90.90% 2 Missing ⚠️
diskann-benchmark/src/index/build.rs 80.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (71.76%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@                      Coverage Diff                       @@
##           pipnn-stack/04-integration    #1294      +/-   ##
==============================================================
- Coverage                       91.28%   91.26%   -0.02%     
==============================================================
  Files                             518      518              
  Lines                           98734    98789      +55     
==============================================================
+ Hits                            90127    90161      +34     
- Misses                           8607     8628      +21     
Flag Coverage Δ
miri 91.26% <71.76%> (-0.02%) ⬇️
unittests 90.94% <71.76%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
diskann-benchmark/src/main.rs 92.07% <100.00%> (+0.02%) ⬆️
diskann-benchmark/src/index/build.rs 84.84% <80.00%> (-0.54%) ⬇️
diskann-benchmark/src/index/benchmarks.rs 71.05% <90.90%> (+0.40%) ⬆️
diskann-benchmark/src/index/streaming/stats.rs 94.81% <55.55%> (-2.90%) ⬇️
diskann-benchmark/src/inputs/graph_index.rs 54.30% <85.71%> (+1.60%) ⬆️
diskann-benchmark/src/inputs/disk.rs 1.38% <0.00%> (-0.07%) ⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants