[WIP] IVF index interface by arkrishn94 · Pull Request #1187 · microsoft/DiskANN

Aditya Krishnan (arkrishn94) · 2026-06-18T15:47:46Z

This is a major work in progress!

To see what I'm going for, see diskann/src/ivf/glue.rs and index.rs.

Introduces diskann::ivf with the IvfIndex wrapper, five core glue traits (ListAccessor, SearchAccessor, SearchStrategy, InsertAccessor, IvfInsertStrategy), an example list-owned IVF-Flat provider, an exact oracle harness, and a test suite covering correctness, concurrency, inserts (including concurrent inserts), and error escalation.

Codecov Comments Bot (codecov-commenter) · 2026-06-18T16:11:20Z

Codecov Report

❌ Patch coverage is 93.97727% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.51%. Comparing base (d9ce362) to head (b70b235).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
diskann/src/ivf/test/provider.rs	88.95%	39 Missing ⚠️
diskann/src/ivf/test/cases/insert.rs	96.69%	4 Missing ⚠️
diskann/src/ivf/test/cases/correctness.rs	94.91%	3 Missing ⚠️
diskann/src/ivf/index.rs	97.75%	2 Missing ⚠️
diskann/src/ivf/test/cases/errors.rs	96.66%	2 Missing ⚠️
diskann/src/ivf/test/harness.rs	97.95%	2 Missing ⚠️
diskann/src/ivf/test/cases/concurrency.rs	98.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1187      +/-   ##
==========================================
+ Coverage   89.47%   89.51%   +0.03%     
==========================================
  Files         486      495       +9     
  Lines       92161    93050     +889     
==========================================
+ Hits        82458    83290     +832     
- Misses       9703     9760      +57

Flag	Coverage Δ
miri	`89.51% <93.97%> (+0.03%)`	⬆️
unittests	`89.16% <93.97%> (+0.04%)`	⬆️

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

Files with missing lines	Coverage Δ
diskann/src/ivf/test/cases/mod.rs	`100.00% <100.00%> (ø)`
diskann/src/ivf/test/cases/concurrency.rs	`98.73% <98.73%> (ø)`
diskann/src/ivf/index.rs	`97.75% <97.75%> (ø)`
diskann/src/ivf/test/cases/errors.rs	`96.66% <96.66%> (ø)`
diskann/src/ivf/test/harness.rs	`97.95% <97.95%> (ø)`
diskann/src/ivf/test/cases/correctness.rs	`94.91% <94.91%> (ø)`
diskann/src/ivf/test/cases/insert.rs	`96.69% <96.69%> (ø)`
diskann/src/ivf/test/provider.rs	`88.95% <88.95%> (ø)`

... and 18 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T18:13:58Z

+    }
+
+    /// Insert a vector under external id `id`.
+    pub fn insert<'a, S, T>(


Do we also want delete in this interface?

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T18:14:55Z

+        context: &'a P::Context,
+        id: &P::ExternalId,
+        vector: T,
+    ) -> impl SendFuture<ANNResult<()>>


Should we have the concept of an index that is read-only? I am thinking of designs where the index is loaded into memory for maintenance and then queries are served from a slower provider. In that case, we don't necessarily want to support insert/delete in the slower provider.

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T18:16:54Z

+    pub fn knn_search<'a, S, T, OB>(
+        &'a self,
+        k: NonZeroUsize,
+        nprobe: usize,


Should nprobe be a NonZeroUsize that we enforce is greater than or equal to k?

Also, how will this contract deal with parameters used for searching the centroid index (i.e. L_search)? Maybe it should take in a type CentroidSearchParams or something similar instead

Well, >= k maybe doesn't make sense, but maybe n_probe * min_size >= k? That would, however, mean we needed to enforce the minimum size strictly.

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T18:18:09Z

+
+            let k = k.get();
+            let mut queue = NeighborPriorityQueue::new(k);
+            let mut cmps: u32 = 0;


We should also count distance comparisons done to select centroids, right?

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T18:22:05Z

+}
+
+/// Per-call factory for IVF insert.
+pub trait InsertStrategy<'a, Provider, T>: Send + Sync


Trying to understand the vision for the dynamic case here--is the idea that we will later extend this to deletes, at which point we call it something like DynamicStrategy, or that there will be a separate DeleteStrategy object?

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T18:27:32Z

+
+After a maintenance accessor returns `Ok(())` from `apply`:
+
+1. Every visible point is assigned to exactly one live list.


This seems like something we do not want to deeply commit to in our architecture. Replication of points is a pretty standard technique in IVF.

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T18:28:23Z

+2. `assignment[point] == list` if and only if `point` is a member of `list`.
+3. Every live list has exactly one live centroid with the same logical id.
+4. Every list member has an available canonical vector and scan payload.
+5. Retired list ids are never reused for a different centroid.


Why can't we reuse list ids? This seems like something we might want to do.

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T18:29:26Z

+5. Retired list ids are never reused for a different centroid.
+6. Point identity mappings agree with point visibility in the partition.
+7. Inserts may trigger splits but not dissolves.
+8. Deletes may trigger dissolves but not splits.


Do we want to commit that deletes can't trigger splits? I think this is a bit contrary to some of our discussions on a batching API, and iirc we did at least one experiment that shows that this policy would degrade recall.

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T18:36:53Z

+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct PointMove<Id, ListId> {
+    /// Internal point id being inserted, deleted, or reassigned.
+    pub id: Id,
+    /// Previous list, or `None` for a newly inserted point.
+    pub from: Option<ListId>,
+    /// New list, or `None` for a deleted point.
+    pub to: Option<ListId>,
+}


It might make debugging/reading code easier if we made this an enum over delete, insert, and reassign, but just a thought

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T18:45:00Z

+### Split Semantics
+
+A split retires one parent and installs two children, for a net increase of one
+live cluster. Parent ids are never reused. Candidate centroids for a region are


Why can't parent ids be reused?

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T18:46:21Z

+   current assignment.
+2. Stage deletion of each point id.
+3. Group deletes by list and compute projected post-delete sizes.
+4. Admit underfull victims without crossing `min_clusters`.


Is a victim a centroid that is being deleted, or the points in the deleted centroid's posting list? What does it mean to "admit" them?

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T18:49:05Z

+centroid cannot make a point already assigned to another surviving centroid
+prefer the removed centroid. Deletes never trigger a split in the same
+operation. A survivor above the split threshold remains eligible for the next
+insert-driven split.


We should add a section for a combined insert/delete batch API since that will likely be necessary. Also, I don't think we should allow deletes to over-fill a posting list. In fact, we should maintain an invariant that posting list updates passed via the PartitionUpdate can never exceed the maximum number of points. We can be flexible on the minimum number of points, but by being inflexible on the max we maintain openness to providers that do a fixed allocation for each posting list.

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T19:08:57Z

+- Design blob coordination separately rather than forcing disk and blob through
+  one generic publication protocol.
+
+## Testing Strategy


We should add some integration tests with exact baselines using the same framework as is used for DiskANN.

Magdalen Dobson Manohar (magdalendobson) · 2026-09-21T19:10:01Z

 // Index Implementations
 pub mod flat;
 pub mod graph;
+pub mod ivf;


Meta-question, do we want a separate diskann-ivf crate or do we want inclusion within diskann?

Magdalen Dobson Manohar (magdalendobson) · 2026-09-22T17:41:26Z

+    /// Errors from list selection or scanning.
+    type Error: ToRanked + Debug + Send + Sync + 'static;
+
+    /// Select exactly the available requested number of lists for the bound query.


This should probably also return some kind of stats object

Magdalen Dobson Manohar (magdalendobson) · 2026-09-23T18:24:08Z

+///
+/// The centroid catalog is authoritative. Approximate implementations, such as
+/// a graph navigator, must retain enough catalog information to reject retired
+/// ids and recover with exact selection when navigation cannot produce a usable


We expect a graph index to handle fallback to flat scan? Why is that? That's not typical for DiskANN in general, is there a reason it would be needed for an IVF centroid index?

Co-authored-by: Magdalen Manohar <magdalen@magdalen.localdomain>

Magdalen Dobson Manohar (magdalendobson) · 2026-10-01T17:18:37Z

+/// Staged points, their canonical vectors, and their routes.
+///
+/// Batch position `i` refers to `ids[i]`, row `i` of `vectors`, and `routes[i]`.
+pub(in crate::ivf) struct StagedBatch<Id, L> {


What is the difference between pub(in crate::ivf) and pub(crate) here?

Magdalen Dobson Manohar (magdalendobson) · 2026-10-01T17:19:11Z

+    utils::VectorId,
+};
+
+/// Staged points, their canonical vectors, and their routes.


I think we can just say "vector" instead of "canonical vector" in the docstrings here. Canonical doesn't have a defined meaning in the repo

Magdalen Dobson Manohar (magdalendobson) · 2026-10-01T17:35:37Z

+/// The source and destination of reassigned points. The two never match.
+///
+/// Ordered by destination, then source.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]


We want to be able to hash this?

Magdalen Dobson Manohar (magdalendobson) · 2026-10-01T17:38:55Z

+/// Parameters for the online split policy.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct DynamicIvfConfig {
+    /// Split a list once an insert batch would grow it beyond this many points.


We can make all of these usize NonZeroUsize instead

Magdalen Dobson Manohar (magdalendobson) · 2026-10-01T17:40:42Z

+pub struct DynamicIvfConfig {
+    /// Split a list once an insert batch would grow it beyond this many points.
+    pub split_threshold: usize,
+    /// Hard cap on live lists; `None` allows unbounded growth.


Is there an advantage to allowing unbounded growth? It seems like it might introduce too much complication without us getting much back in return

Magdalen Dobson Manohar (magdalendobson) · 2026-10-02T15:11:54Z

+    pub(super) ids: Vec<Id>,
+    pub(super) vectors: Matrix<f32>,
+    /// The nearest live list of each point.
+    pub(super) routes: Vec<L>,


Can we call this something like nearest_centroids instead?

Magdalen Dobson Manohar (magdalendobson) · 2026-10-02T15:24:46Z

+    /// The nearest live list of each point.
+    pub(super) routes: Vec<L>,
+    /// Batch positions grouped by route.
+    pub(super) by_route: Grouped<L, usize>,


Do we really need the Grouped struct for this? Could we just have a Vec<(L, Vec<usize>)> instead? Seems like it could save a lot of code and I find the Csr thing a little hard to understand

Magdalen Dobson Manohar (magdalendobson) · 2026-10-02T15:30:59Z

+    ///
+    /// Reserves two child ids per parent, selects each parent's nearest surviving
+    /// lists as its neighbors, and reads the members and canonical vectors of every
+    /// parent and neighbor. `parents` must be ascending.


"Ascending" = in sorted order by ID, right? Can we just say that explicitly?

Magdalen Dobson Manohar (magdalendobson) · 2026-10-02T15:48:09Z

+pub(in crate::ivf) struct SplitPlan<Id, L> {
+    batch: StagedBatch<Id, L>,
+    lists: Vec<L>,
+    parents: usize,


Can we just call this num_parents instead?

Magdalen Dobson Manohar (magdalendobson) · 2026-10-02T15:49:49Z

+        P: Provider<InternalId = Id, ListId = L>,
+        A: MaintenanceAccessor<P>,
+    {
+        let count = 2 * parents.len();


num_children instead of count maybe?

Magdalen Dobson Manohar (magdalendobson) · 2026-10-02T15:54:20Z

+                .centroid(parent)
+                .ok_or_else(|| index_error(format!("split parent {parent} is not live")))?;
+            let selected = centroids
+                .select(anchor, config.reassign_neighbors.saturating_add(1))


Do we just add +1 here to reassign_neighbors to get some headroom in case one of the parent centroid's nearest neighbors is also deleted?

Aditya Krishnan (arkrishn94) added 4 commits June 16, 2026 10:45

init rfc

64dedef

Merge remote-tracking branch 'origin/main' into u/adkrishnan/ivf-index

70ad067

delete rfc

b70b235

dynamic index glue/trait initial brainstorm

4a18959

Magdalen Dobson Manohar (magdalendobson) reviewed Sep 21, 2026

View reviewed changes

Magdalen Dobson Manohar (magdalendobson) reviewed Sep 22, 2026

View reviewed changes

Magdalen Dobson Manohar (magdalendobson) reviewed Sep 23, 2026

View reviewed changes

Add co-location sets to the initial contract (#1438)

092aa7d

Co-authored-by: Magdalen Manohar <magdalen@magdalen.localdomain>

Aditya Krishnan (arkrishn94) added this to DiskANN backlog Sep 29, 2026

Aditya Krishnan (arkrishn94) removed this from DiskANN backlog Sep 29, 2026

Aditya Krishnan (arkrishn94) linked an issue Sep 29, 2026 that may be closed by this pull request

Implement the accessor and the insert path #1191

Open

Aditya Krishnan (arkrishn94) added 3 commits September 29, 2026 11:55

initial index impl, lots more to go

d076377

lots to change but we're getting somewhere

83c451a

cutting superfluous things

a15c479

Magdalen Dobson Manohar (magdalendobson) reviewed Oct 1, 2026

View reviewed changes

move to enum based point updates

935b088

Magdalen Dobson Manohar (magdalendobson) reviewed Oct 2, 2026

View reviewed changes


		After a maintenance accessor returns `Ok(())` from `apply`:

		1. Every visible point is assigned to exactly one live list.

Conversation

Aditya Krishnan (arkrishn94) commented Jun 18, 2026

Uh oh!

Codecov Comments Bot (codecov-commenter) commented Jun 18, 2026

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants