Skip to content

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

Open
SeliMeli wants to merge 11 commits into
pipnn-stack/04-integrationfrom
pipnn-stack/05-benchmark
Open

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

Conversation

@SeliMeli

@SeliMeli 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 existing algorithm/install timing boundary; /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.
  • 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.
  • 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 test proves the frozen start slot exposes exactly the selected real source row.
  • 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 pipnn and 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

@SeliMeli
SeliMeli force-pushed the pipnn-stack/05-benchmark branch from 28f253a to 4834da3 Compare July 30, 2026 07:47
@SeliMeli
SeliMeli force-pushed the pipnn-stack/05-benchmark branch from 4834da3 to 79a9407 Compare July 30, 2026 08:26
@SeliMeli
SeliMeli force-pushed the pipnn-stack/05-benchmark branch from 79a9407 to 4f40c35 Compare July 30, 2026 08:55
@SeliMeli
SeliMeli requested review from a team and a lite review from Copilot July 30, 2026 11:26
@SeliMeli
SeliMeli force-pushed the pipnn-stack/05-benchmark branch from 4f40c35 to 05933e6 Compare July 30, 2026 11:26

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
@SeliMeli
SeliMeli force-pushed the pipnn-stack/05-benchmark branch from 05933e6 to b085972 Compare 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
@SeliMeli
SeliMeli force-pushed the pipnn-stack/05-benchmark branch from b085972 to b5b5cad Compare 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
@SeliMeli
SeliMeli force-pushed the pipnn-stack/05-benchmark branch from b5b5cad to 7872932 Compare 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.

🟡 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_index will internally cast max_points to u32 when constructing the provider's start-point range. For datasets larger than u32::MAX, this can truncate silently and produce inconsistent behavior; the later u32::try_from(id) check happens only after the provider has already been allocated. Fail fast before allocating the provider by validating npoints <= 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_integration currently 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). Since run_integration_test now returns the parsed results, assert that the build kind is PiPNN (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_algorithm is PiPNN, the benchmark bypasses the incremental insert path entirely, so fields like multi_insert and insert_retry become 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 when build_algorithm is explicitly PiPNN (e.g., error if multi_insert/insert_retry are 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.

Comment on lines +86 to +87
# Enable PiPNN graph construction.
pipnn = ["diskann/pipnn", "diskann-disk/pipnn"]
Copilot AI review requested due to automatic review settings August 5, 2026 10:00
Copilot AI dismissed their stale review, a newer Copilot review was requested August 5, 2026 10:06

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
@SeliMeli
SeliMeli force-pushed the pipnn-stack/05-benchmark branch from a4b3c1a to 3c0bb25 Compare 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
@SeliMeli
SeliMeli force-pushed the pipnn-stack/05-benchmark branch from 3c0bb25 to ce4b316 Compare 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.

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.

2 participants