Repository navigation
PiPNN 3/6: add core graph construction - #1290
weiyaoluo (SeliMeli) wants to merge 234 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces the “core” PiPNN build pipeline in the diskann-pipnn crate, wiring together deterministic partitioning, leaf-local candidate construction, and final pruning into a public build_graph API with a validated build context.
Changes:
- Adds
PiPNNConfigvalidation and aPiPNNBuildContextthat binds PiPNN policy to DiskANN graph pruning policy and a caller-owned Rayon thread pool. - Implements the three main stages: partitioning (
partitioning.rs), leaf candidate construction (leaf_build.rs), and final pruning via shared Vamana robust prune (finalization.rs). - Adds comprehensive unit/integration tests and a Criterion benchmark for core scenarios; updates dependencies, lockfile, and mutation-test exclusions.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| diskann-pipnn/src/lib.rs | Adds public PiPNN API (PiPNNConfig, PiPNNBuildContext, build_graph) and stage orchestration. |
| diskann-pipnn/src/partitioning.rs | Implements deterministic overlapping partition construction and leader assignment/scatter. |
| diskann-pipnn/src/partitioning/tests.rs | Adds unit tests covering partition determinism, invariants, error cases, and helpers. |
| diskann-pipnn/src/leaf_build.rs | Builds leaf-local symmetric k-NN candidates and accumulates global candidates safely in parallel. |
| diskann-pipnn/src/leaf_build/tests.rs | Adds unit tests for candidate correctness, invariants, type support, and error handling. |
| diskann-pipnn/src/finalization.rs | Orders/prunes candidate rows using shared robust_prune and validates candidate IDs/shape. |
| diskann-pipnn/src/finalization/tests.rs | Adds unit tests for pruning behavior and candidate validation failures. |
| diskann-pipnn/src/tests.rs | Tests effective_metric behavior for integer cosine-normalized handling. |
| diskann-pipnn/tests/config.rs | Integration tests for config validation and graph-policy compatibility checks. |
| diskann-pipnn/tests/build_graph.rs | Integration tests for end-to-end graph building, invariants, determinism, and type/metric support. |
| diskann-pipnn/benches/core.rs | Adds a Criterion benchmark for stage-focused core build scenarios. |
| diskann-pipnn/Cargo.toml | Updates crate dependencies/dev-dependencies and registers the new core benchmark target. |
| Cargo.lock | Records dependency graph changes for the updated diskann-pipnn crate dependencies. |
| .cargo/mutants.toml | Adds mutation-test exclusions for key PiPNN public boundary checks and partitioning invariants. |
Comments suppressed due to low confidence (1)
diskann-pipnn/src/partitioning.rs:604
size_of::<f32>()is used without being in scope (nouse std::mem::size_of;and not qualified), so this function won’t compile as written.
fn assignment_stripe_rows(leaders: usize) -> usize {
(ASSIGNMENT_CACHE_TARGET_BYTES / (leaders.max(1) * size_of::<f32>()))
.clamp(MIN_ASSIGNMENT_STRIPE_ROWS, MAX_ASSIGNMENT_STRIPE_ROWS)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5be0c9c to
50047c6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
diskann-pipnn/src/partitioning.rs:637
size_ofis used without being in scope (std::mem::size_of), which will not compile. Qualify the call or import it.
fn assignment_stripe_rows(leaders: usize) -> usize {
(ASSIGNMENT_CACHE_TARGET_BYTES / (leaders.max(1) * size_of::<f32>()))
.clamp(MIN_ASSIGNMENT_STRIPE_ROWS, MAX_ASSIGNMENT_STRIPE_ROWS)
}
diskann-pipnn/src/partitioning.rs:19
Normis imported but never used in this module, which will tripunused_importswarnings (and can become CI failures under-D warnings). Remove it from the import list.
use diskann::{utils::VectorRepr, ANNError, ANNResult};
use diskann_linalg::Transpose;
use diskann_utils::views::MatrixView;
use diskann_vector::{distance::Metric, norm::FastL2NormSquared, Norm};
use rand::{prelude::IndexedRandom, SeedableRng};
use rayon::prelude::*;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
diskann-pipnn/src/partitioning.rs:390
gather_rowsusesTypeId::of::<T>(), which implicitly requiresT: 'static. Making that bound explicit here avoids surprising/indirect trait-bound errors later and matches the publicbuild_graphboundary (which already requires'static).
fn gather_rows<T>(data: MatrixView<'_, T>, indices: &[u32], output: &mut [f32]) -> ANNResult<()>
where
T: VectorRepr,
1324668 to
857e200
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
diskann-pipnn/src/partitioning.rs:641
size_of::<f32>()is used without being imported or qualified, which will fail to compile. Qualify it withstd::mem::size_of(or add an explicit import).
let rows = ASSIGNMENT_CACHE_TARGET_BYTES / (leaders.max(1) * size_of::<f32>());
857e200 to
f642204
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
diskann-pipnn/src/leaf_build.rs:222
build_leafis executed from a Rayon parallel context (viabuild_leaf_candidates), so it should also explicitly requireT: Send + Syncto reflect the actual thread-safety requirement.
where
T: VectorRepr + 'static,
{
diskann-pipnn/src/leaf_build/tests.rs:154
assert_source_typeforwardsTinto the parallel leaf build path, so it should also includeSend + Syncbounds to match the production requirements.
fn assert_source_type<T>(data: &[T])
where
T: diskann::utils::VectorRepr + 'static,
{
diskann-pipnn/src/leaf_build.rs:193
build_leaf_candidatesuses Rayon parallel iteration overdata, soTmust beSend + Sync. Making this explicit in the signature avoids confusing trait-bound errors at call sites and documents the thread-safety requirement.
This issue also appears on line 220 of the same file.
where
T: VectorRepr + 'static,
{
diskann-pipnn/src/leaf_build/tests.rs:35
- This test helper calls
build_leaf_candidates, which (via Rayon) requiresT: Send + Sync. Add the bounds here so the test continues to compile once the production signature is tightened.
This issue also appears on line 151 of the same file.
where
T: diskann::utils::VectorRepr + 'static,
{
f642204 to
b1181d6
Compare
Each kernel now calls one top-k function: - select_top_k_ids ranks partition rows; select_top_k_symmetric runs the leaf pair scan. Both take k from the output width and choose fixed-size or slice nearest sets internally. - Remove Ranker, TopKVisitor, BatchRanker, Batch, BatchVisitor, with_topk, and with_batch. - Share output-row validation and checked distance scratch between the kernels. Remove leaf_neighbor_count, LeafKernelError, and UNASSIGNED_LEADER. - Load SIMD groups through one unsafe load whose bounds come from &[f32; LANES]. A load from a copied group cost 11% of leaf ranking time on AVX2. - Leaf CosineNormalized delegates to InnerProduct and adds 1. - Top-k and kernel tests run on every supported architecture and cross SIMD groups. Point the nightly Miri step at the renamed tests.
Adapt core to the kernel API changes: - Inline leaf_neighbor_count in leaf_build. - Use topk::UNASSIGNED in place of UNASSIGNED_LEADER. - Update the LeafMetric test stub to take MutMatrixView.
The kernel entry points already return an error for a bad output shape, so the top-k functions now check their shape preconditions with debug assertions only. Select distance rows by index. A distance matrix without columns then leaves every slot UNASSIGNED instead of panicking in row_iter.
#1287 landed on main as one squash commit, so core now merges main to make main the merge base of #1290 again. - Take main's top-k module, CI coverage flags, and diskann-linalg entry (the final kernels state); keep core's pipnn module and feature list. - Remove seven #[allow] attributes that main's allow_attributes lint rejects. As #[expect] they are unfulfilled: neither disallowed_methods nor too_many_arguments fires at those sites.
Partitioning: - Replace the 30-level cap and `PartitionError::IterationLimit` with a progress rule. When a fanout-one split sends every point to one leader, the copies of that leader become equal-size leaves and the rest is re-split. Duplicate vectors no longer fail the build. - Remove `PartitionError::ShapeOverflow` and `checked_area`. Split IDs are distinct dataset rows, so buffer products cannot overflow. - Lease stripe buffers per stripe instead of per worker chunk, and drop the power-of-two stripe rounding and the unreachable 32-point minimum. BigANN-10M fp16 partition, 16 threads, 4 interleaved runs: medians base 51.7 s, per-stripe 52.2 s, no rounding 52.1 s; identical leaves. - Keep the parallel scatter: 10M points at fanout 10 scatter in 0.59 s serially and 0.15 s in parallel; the serial share grows with threads. - Document the reason and measurement for each tuning constant. Leaf build and conversion: - Remove `LeafBuildError`; its shape, conversion and kernel variants are unreachable. Errors propagate as `ANNError`. - Keep the FP16 gather dispatch and document why: `as_f32_into` uses the build target's instructions, and dependents of `diskann` do not inherit the repository's x86-64-v3 flag. Baseline builds gather 4.6-22x slower without it; x86-64-v3 builds are within 10%. Tests: - Replace rstest cases with table loops; PR3 test instances 117 -> 45. - Add seam oracles at production shape: partition assignment against brute force, and leaf candidates against the leaf kernel per leaf. - Add a multi-level build check that 1 and 4 workers give the same graph for every metric. The previous suite passed with unsorted candidates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Partitioning: - When a level's fanout is not below its leader count, every point would join every leader and each child would repeat the cluster. Requeue the cluster at the next level instead of splitting it. - When a fanout-one split sends every point to one leader, cut the child into equal leaves in point order. Copy detection is gone. - Remove `merge_undersized_leaves` and `c_min`. SIFT-1M and Enron-1M builds (3 runs each) gave the same recall@10 at L=10..80 with and without the merge (largest difference 0.08 pp), and no isolated points. Configuration (review feedback): - `c_max`, `leaf_k`, `replicas` and the fanout entries are `NonZeroUsize`. - `PiPNNConfigError` replaces message errors for configuration and context checks. Finalization: - Keep the `u16::MAX` nearest candidates for RobustPrune, as Vamana limits its pool, instead of failing the build. `FinalizationError` is removed. Tests run under Miri when selected; the `not(miri)` gates are removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The candidate lists were sorted by point ID on one thread after the leaf jobs. ID order carries no distance information: final pruning ranks the candidates by distance, and search reads every neighbor of a point, so the sort only fixed how distance ties break. On BigANN-10M (c_max 512, fanout [10, 3], k 3, 16 threads) the serial pass took 10.4-12.1 s per build; recall@10 at L 10-100 stayed within 3e-5 without it. The graph now depends on job completion order in distance ties and in neighbor order, so the 1-vs-4-worker equality test becomes a row validity test. The leaves stay reproducible. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The stripe height came from a 512 KiB distance-block target justified by the L2 cache. Sweeps of assign_to_leaders on Zen 3, Zen 4, Emerald Rapids and Ice Lake (GCP VMs, 0% steal) showed that L2 is not the limit: one worker runs 16 MiB stripes at full speed, and slowdowns start when the stripes of all workers exceed the last-level cache. The working set also includes the gathered points, which the old rule ignored; with 64 leaders and 1,536 dimensions its 1,024-point cap was up to 2.8x slower than the best height. A fixed 256 points stayed within 11% of the best height for f32, f16 and u8 data, 128-1,536 dimensions, 64-1,000 leaders and 1-16 workers (1.4% on average). A cache-size rule did not do better: VMs report a last-level cache that other tenants share. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Final pruning kept the u16::MAX nearest candidates and said that Vamana does the same. Vamana keeps the config.max_occlusion_size() nearest (default 750). Use the graph configuration value so PiPNN and Vamana prune the same pool and long candidate lists cost the same. Lists in the usual configurations (at most l_max with HashPrune, at most 595 for direct BigANN-10M candidates) are below the cap, so their graphs do not change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewers asked why 6364136223846793005 was chosen. Name it CHILD_SEED_MULTIPLIER and state where it comes from (Knuth's MMIX LCG) and why any odd multiplier would do: StdRng::seed_from_u64 mixes the seed again, so the step only has to keep seeds distinct. Mark the partition seed and replica step values as arbitrary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The child seed came from an LCG step with Knuth's MMIX multiplier, and replica seeds from a prime step of 7,919. Neither value mattered: StdRng::seed_from_u64 already expands the seed with PCG32 and gives independent streams for adjacent seeds. Each split now seeds one StdRng, samples its leaders from it, and draws the seed of each child from the same stream. Replica r uses PARTITION_SEED + r. The leaves stay reproducible, but differ from the previous seeds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PiPNNConfig gains seed: Option<u64>, following the random_seed convention of the disk builder: Some(seed) gives the same leaves in every build, and None draws a seed from the operating system. Replica r uses base + r. The fixed PARTITION_SEED is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
"partition" named the whole process, one waiting cluster and a split result, and "scatter" named the grouping of points by leader. Keep partition/partitioning for the whole process only: - PendingPartition -> Cluster, split_partition -> split_cluster, PartitionSplit -> ClusterSplit with next_level and leaves - scatter_assignments -> group_by_leader, scatter_serial -> group_by_leader_serial, PARALLEL_SCATTER_MIN_POINTS -> PARALLEL_GROUPING_MIN_POINTS - the parallel grouping splits points into chunks, not stripes, so stripe keeps its single meaning (one assignment task) No behavior change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| if !(0.0 < self.p_samp && self.p_samp <= 1.0) { | ||
| return Err(PiPNNConfigError::SamplingFraction(self.p_samp)); | ||
| } |
There was a problem hiding this comment.
You can use Positive<f64>
There was a problem hiding this comment.
Going to leave this one as is. p_samp has to be in (0, 1], and Positive only covers the lower half, so validate would still need the <= 1 check. Positive also lives in diskann-quantization, which diskann doesn't depend on, so we'd be adding that crate for a single field.
| if metric == Metric::CosineNormalized | ||
| && (TypeId::of::<T>() == TypeId::of::<u8>() || TypeId::of::<T>() == TypeId::of::<i8>()) |
There was a problem hiding this comment.
Can you explain why this type id check is needed here?
There was a problem hiding this comment.
diskann-vector computes CosineNormalized as Cosine for u8 and i8 (the @integer arm in distance_provider.rs), since integer vectors can't be unit norm. So for u8/i8 data, search and final pruning already use cosine. The PiPNN partition and leaf kernels do their own distance math and skip that dispatch, so without this switch they'd rank by raw dot product and the leaves wouldn't match what search uses.
Agree the TypeId check is ugly. One option is to let the type say which metric it actually computes, e.g. a provided method on VectorRepr:
fn effective_metric(metric: Metric) -> Metric { metric }with u8/i8 overriding it to map CosineNormalized to Cosine. PiPNN would call T::effective_metric(metric) and this function goes away. That touches VectorRepr though, so I wanted to check with you before doing it in this PR. What do you think?
| } | ||
|
|
||
| let point_values = &mut buffers.point_values[..point_count * data.ncols()]; | ||
| gather_as_f32(data, point_ids, point_values)?; |
There was a problem hiding this comment.
If we only support ingesting f32 vectors for now, let's impose that constraint right up top, instead of doing this conversion deeper in the code. This creates unnecessary allocation duplication and is not transparent to the user.
There was a problem hiding this comment.
This buffer isn't extra. A leaf's rows are spread across the dataset, and the GEMM needs them packed into one contiguous matrix, so we copy them into a per-worker buffer even when the input is already f32. The conversion to f32 happens in that same copy, so it costs very little on top, and the caller doesn't need to convert anything. The buffer is reused across leaves, so there's no allocation per leaf either.
| if !neighbor.is_assigned() { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
For my understanding - when does this condition get triggered again?
There was a problem hiding this comment.
This skips NaN/inf distances, which the leaf kernel leaves out of a row. Normal data doesn't hit it. It's consistent with Vamana, whose search queue drops NaN distances on insert. Added a comment.
| } | ||
|
|
||
| #[test] | ||
| fn candidates_match_the_leaf_kernel_on_each_leaf() { |
There was a problem hiding this comment.
This is an important happy path to test, please make sure you're covering the right grid of parameters. For e.g. I only see L2 tests and similarly not convinced that the k, dimension, point count combination is thorough.
There was a problem hiding this comment.
The grid belongs at the kernel, where metric and dimension change behavior. The PR2 kernel tests sweep metrics, dimensions, widths and point counts. Leaf build has no metric logic: it passes the metric to the kernel as a type parameter, and the_requested_metric_determines_leaf_neighbors checks that each metric reaches the kernel. This test covers what leaf build adds: the gather, the cut of k to the leaf size, buffer reuse across leaves and the merge. In 8559ba4 it runs f32, f16, i8 and u8 data with k = 1 to 10 on leaves of 1 to 40 points.
Reviewers asked how the parameters are usually set. State the default configuration (c_max 512, p_samp 0.005, fanout [10, 3], leaf_k 2, replicas 1) and what raising each parameter does to candidates, recall and build time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
prepare was the only caller of the grow helper. Grow the two buffers in place and say why they never shrink. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… builds The leaf-build oracle test now runs f32, f16, i8 and u8 data with k = 1 to 10 on leaves of 1 to 40 points, so k falls above, equal to and below the other points of a leaf while the buffers are reused. Leaf build has no metric logic; the kernel tests cover metrics, dimensions and widths. The multi-level build test also runs on f16 data. The other f16 builds are too small to split. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PiPNNBuildContext::new rejected a graph Config whose prune kind did not match the metric. Vamana takes the same Config and metric pair without this check, and every PiPNN caller builds the Config from the metric, so the check never fires. Final pruning keeps using the prune kind of the Config. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
I've tested it on a 52 cores machine (cascade lake), the best result for bigann10m is 20s at that machine. |
The baseline x86-64 target has SSE2 instructions for u8 and i8 to f32 conversion, so the generic loop vectorizes without the x86-64-v3 flag. f16 conversion needs F16C, which starts at x86-64-v3. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
Add PiPNN graph construction under
diskann::graph::pipnn, behind the opt-indiskann/pipnnfeature.build_graphborrows one contiguousMatrixViewoff32,f16,u8ori8vectors and returns one adjacency list for each point, in three steps:c_maxpoints with the partition kernel from PiPNN 2/6: add numerical kernels #1287.leaf_knearest neighbors of each point inside each leaf with the leaf kernel from PiPNN 2/6: add numerical kernels #1287, and merge the pairs into one candidate list per point.The core does not load providers, select start points or serialize indexes. This is part 3 of the PiPNN stack; #1291 integrates it into the disk index build. Review in pipeline order:
mod.rs,partitioning.rs,leaf_build.rs,finalization.rs.Design
Take the graph policy from the DiskANN
Config. Degree, alpha,max_occlusion_sizeand the prune kind come fromConfig, as in Vamana.PiPNNConfigholds the partition and leaf parameters, withNonZeroUsizecounts and aPiPNNConfigErrorfromvalidate.Dispatch once per build.
build_graphselects the SIMD architecture and the metric, so every step compiles for one of each.CosineNormalizedonu8ori8data builds withCosine, because converted integer vectors do not have unit norm;diskann-vectorapplies the same redirect.Split clusters one level at a time. A split samples
ceil(p_samp * n)leaders, clamped to [2, 1000] as in the paper, assigns each point to itsfanoutnearest leaders, and groups the points by leader. Children of at mostc_maxpoints become leaves. Each split seeds its children from its own random stream, and children keep the input order of their points, so the leaves do not depend on the thread count.End partitioning without an iteration limit. A level whose fanout is at least the leader count would copy the cluster into every child, so the cluster moves to the next level unsplit. At fanout 1, a child that holds every point means all leaders are copies of one vector, and the split cuts it into equal leaves. Small leaves are kept as they are: on BigANN-10M, merging leaves below 64 points changed recall@10 by at most 0.0006, the run-to-run spread, and cost 3.7% to 10.9% of build time.
Assign points in stripes of 256. A stripe gathers its points as
f32and ranks them against all leaders in one kernel call. Sweeps on four CPU families kept this height within 1.4% of the best on average; the comment onASSIGNMENT_STRIPE_POINTShas the details. A cache-size rule did no better, because a VM reports a last-level cache that other tenants share.Convert
f16with the instructions of the running CPU. Crates that depend ondiskanndo not inherit itsx86-64-v3target flag, so the genericVectorRepr::as_f32_intoconvertsf16in software for them, which made gathers 4.6 to 22 times slower on an EPYC 7763.Lock each candidate list once per leaf. A leaf job collects its pairs in both directions locally, then appends them to each point's shared list under one lock. The lists stay unsorted, because final pruning ranks candidates by distance; sorting them cost 10 to 12 s of serial time per BigANN-10M build with no recall change. List order follows job completion and only breaks distance ties, so leaves are reproducible and the graph is not.
Prune only overfull lists. A longer list is ranked by full-precision distance to its source, cut to the
max_occlusion_sizenearest candidates as in Vamana, and pruned with the shared RobustPrune.