Consolidate insert into PruneAccessor#1138
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1138 +/- ##
==========================================
- Coverage 89.43% 89.41% -0.02%
==========================================
Files 484 484
Lines 91495 91419 -76
==========================================
- Hits 81829 81744 -85
- Misses 9666 9675 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR completes the “insert-side” consolidation started in #1067 by collapsing multiple provider/workingset traits used during pruning/index construction into a single glue::PruneAccessor abstraction, and then plumbing that change through the index build path and all in-tree providers/strategies.
Changes:
- Introduces
glue::PruneAccessor(neighbors delegation + fill returning(View, Distance)), and simplifiesPruneStrategy/MultiInsertStrategyaround accessor construction (includingseeded_prune_accessor). - Removes/rewires now-redundant provider/workingset traits (e.g.,
BuildDistanceComputer,HasElementRef,Fill,AsWorkingSet, neighbor delegation helpers) and updates call sites accordingly. - Updates core index build/prune logic and all in-tree providers (async inmem, Garnet, BfTree, benchmarks/tests) to the new accessor model; adds
DistanceFunctionimpl for&Tto ease borrowing.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| diskann/src/provider.rs | Simplifies neighbor accessor traits (now &mut self + ANNResult<()>) and introduces provider::Neighbors adaptor. |
| diskann/src/graph/workingset/mod.rs | Removes Fill/AsWorkingSet API surface; keeps View as the pruning read interface (now intended to pair with PruneAccessor). |
| diskann/src/graph/workingset/map.rs | Updates Map docs/seed story to align with seeded_prune_accessor; removes AsWorkingSet impl + related test. |
| diskann/src/graph/test/provider.rs | Updates test provider + strategies to implement glue::PruneAccessor and new neighbor accessor method signatures. |
| diskann/src/graph/test/cases/inplace_delete.rs | Adjusts tests to pass mutable neighbor accessors under new NeighborAccessor signature. |
| diskann/src/graph/test/cases/index.rs | Adjusts prune-range test neighbor access to the new mutable accessor pattern. |
| diskann/src/graph/index.rs | Reworks prune/build flows to use PruneAccessor directly (no external working set); updates multi-insert to build per-task accessors from seeds. |
| diskann/src/graph/glue.rs | Adds PruneAccessor, simplifies PruneStrategy/MultiInsertStrategy contracts and docs to match the new model. |
| diskann-vector/src/traits.rs | Adds DistanceFunction implementation for &T to allow passing borrowed computers/functors. |
| diskann-providers/src/model/graph/provider/async_/inmem/spherical.rs | Migrates inmem spherical pruning to glue::PruneAccessor and constructs distance computer eagerly. |
| diskann-providers/src/model/graph/provider/async_/inmem/scalar.rs | Migrates inmem scalar pruning to glue::PruneAccessor, moving distance computer creation into accessor construction. |
| diskann-providers/src/model/graph/provider/async_/inmem/provider.rs | Updates inmem neighbor provider impls to new NeighborAccessor{,Mut} method signatures. |
| diskann-providers/src/model/graph/provider/async_/inmem/product.rs | Migrates PQ/product prune accessors (including hybrid behavior) to glue::PruneAccessor and internalizes “working set” state. |
| diskann-providers/src/model/graph/provider/async_/inmem/mod.rs | Removes the old PassThrough working-set ZST now that working-set traits are gone. |
| diskann-providers/src/model/graph/provider/async_/inmem/full_precision.rs | Migrates full-precision inmem prune accessor to glue::PruneAccessor and caches the distance computer in the accessor. |
| diskann-providers/src/model/graph/provider/async_/distances.rs | Removes hybrid working-set wrapper/overlay glue tied to the old workingset traits. |
| diskann-providers/src/model/graph/provider/async_/common.rs | Removes the old Unseeded seed type since seeding is now handled differently. |
| diskann-providers/src/index/wrapped_async.rs | Updates wrapped async index helpers to use NeighborAccessor{,Mut} bounds instead of AsNeighbor{,Mut}. |
| diskann-providers/src/index/diskann_async.rs | Updates async index tests/helpers to use NeighborAccessor{,Mut} bounds. |
| diskann-garnet/src/provider.rs | Migrates Garnet prune accessor to glue::PruneAccessor, internalizes caching in the accessor, and adjusts neighbor delegation. |
| diskann-bftree/src/provider.rs | Migrates BfTree prune accessors to glue::PruneAccessor, internalizing map state + distance computers and updating multi-insert seeding. |
| diskann-benchmark-core/src/streaming/graph/drop_deleted.rs | Updates benchmark build stage bounds/call sites to new neighbor accessor traits. |
| diskann-benchmark-core/src/build/graph/single.rs | Updates benchmark test neighbor traversal to use mutable accessors. |
| diskann-benchmark-core/src/build/graph/multi.rs | Updates benchmark test neighbor traversal to use mutable accessors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The insert followup to #1067, consolidating the following traits:
diskann::provider::{DelegateNeighbor, AsNeighbor, AsNeighborMut}: Neighbor accessor delegation in this manner is no longer needed.diskann::provider::{HasElementRef, BuildDistanceComputer}: Consolidated intoPruneAccessor.diskann::graph::workingset::{Fill, AsWorkingSet}: Consolidated intoPruneAccessorInto a single
PruneAccessortrait:For insert/prune, the accessor has two jobs:
neighborsmethod for delegation mainly because it's a pattern we use quite heavily already: implement oneNeighborAccessorand have all the other accessors use that.fill, which now returns both aViewand the distance computer. I think this should still remain as a method with a view because it scopes the duration for which pruning is being performed, allowing implementation to acquire locks or otherwise enter some critical region for a duration shorter than the lifetime of the whole accessor.Interaction with the Working Set
The working set is used to cache entries across prunes to reduce trips to the backing provider. With this change, the working set and distance computers become part of the accessor, which adds a little bloat to the accessors, but simplifies how these objects get along.
Without an external working-set,
PruneStrategybecomes much simpler (now it's likeSearchStrategywith just two associated types and a single method.MultiInsertStrategyis slightly changed withfinishstill returning an opaqueSeedbut with thePruneAccessorbeing constructed viaseeded_prune_accessor. This is the replacement for theAsWorkingSettransformation.Suggested Review Order
diskann/src/graph/glue.rs: The newPruneAccessortraitdiskann/src/provider.rs: Simplification to provider-related traits.diskann/src/graph/workingset/: Simplification to workingset API.diskann/src/graph/index.rs: Updated indexing algorithm to take aPruneAccessorrather than a strategy + provider + context.diskann-providers/: Mostly mechanical clean-up.diskann-bftree/: Mostly mechanical clean-up.diskann-garnet/: Mostly mechanical, with one simplification. Since the workingset is now part of the accessor, we can no longer run into situations where the working set contains full-precision vectors but the accessor wants quantized vectors, resulting in some simplification.