Remove `k_value` from `Knn` by magdalendobson · Pull Request #1151 · microsoft/DiskANN

magdalendobson · 2026-06-12T15:25:23Z

When preparing the inline filter search PR I noticed that the k_value in Knn is actually not used anymore, since the output buffer is treated as the source of truth for k. This PR removes that field. In order to do this, since diversity search made use of it, we add a field for it specifically in diversity search, and construct a wrapper struct for Knn in diskann-benchmark-core for validation of parameters and use in size hinting. This moves the k parameter out of places it is not used and directly into the places where it is used.

Copilot

Pull request overview

This PR removes the unused k_value field from diskann::graph::search::Knn, shifting k ownership to the output buffer (and other callers that actually need k). To preserve k where it’s still required (e.g., diversity search, benchmark sizing/validation), it introduces dedicated k-carrying parameter structures in the appropriate layers.

Changes:

Remove k_value and related validation/errors from diskann::graph::search::Knn and update affected graph tests/call sites shown here.
Add original_k_value to diversity search params and validate k fields as NonZeroUsize.
Introduce diskann-benchmark-core::search::graph::KnnWrapper to keep k for validation and sizing while using the updated diskann::graph::search::Knn.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
diskann/src/graph/test/cases/multihop.rs	Update multihop tests to construct `Knn` without `k`.
diskann/src/graph/test/cases/inline.rs	Update inline-filter tests to construct `Knn` without `k`.
diskann/src/graph/test/cases/grid_search.rs	Update grid search test to size outputs using a local `k_value` instead of `params.k_value()`.
diskann/src/graph/test/cases/grid_insert.rs	Update grid insert test to size outputs using a local `k_value` instead of `params.k_value()`.
diskann/src/graph/search/knn_search.rs	Remove `k_value` from `Knn`, simplify constructor/validation accordingly.
diskann/src/graph/search/diverse_search.rs	Route diversity queue construction through new diversity params’ `original_k_value`.
diskann/src/graph/misc.rs	Add validated `DiverseSearchParams` fields (`NonZeroUsize`) and introduce `DiverseSearchError`.
diskann-disk/src/search/provider/disk_provider.rs	Update disk provider to construct `Knn` with the new `(l, beam_width)` signature.
diskann-benchmark/src/backend/index/search/knn.rs	Switch benchmark runs from `diskann::Knn` to `KnnWrapper`.
diskann-benchmark/src/backend/index/result.rs	Update result reporting to read `l` from `KnnWrapper.knn`.
diskann-benchmark-core/src/search/graph/multihop.rs	Update benchmark-core multihop search to accept `KnnWrapper`.
diskann-benchmark-core/src/search/graph/mod.rs	Re-export `KnnWrapper`.
diskann-benchmark-core/src/search/graph/knn.rs	Add `KnnWrapper` and update benchmark KNN search to use it.
diskann-benchmark-core/src/search/graph/inline.rs	Update benchmark-core inline search to accept `KnnWrapper`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    /// Returns an error if `l_value` is zero,
+    /// or if `beam_width` is `Some(0)`.
+    pub fn new(l_value: usize, beam_width: Option<usize>) -> Result<Self, KnnSearchError> {
        let l_value = NonZeroUsize::new(l_value).ok_or(KnnSearchError::LZero)?;


        let strategy = self.search_strategy(vector_filter);
        let timer = Instant::now();
-        let k = k_value;
        let l = search_list_size as usize;
        let stats = if is_flat_search {


    pub fn new(
        diverse_attribute_id: usize,
        diverse_results_k: usize,
+        original_k_value: usize,
        attribute_provider: std::sync::Arc<P>,
-    ) -> Self {
-        Self {
+    ) -> Result<Self, DiverseSearchError> {


Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

codecov-commenter · 2026-06-12T21:54:42Z

Codecov Report

❌ Patch coverage is 90.00000% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.46%. Comparing base (f740bd8) to head (e0ca6f6).
⚠️ Report is 58 commits behind head on main.

Files with missing lines	Patch %	Lines
diskann-benchmark-core/src/search/graph/knn.rs	69.56%	7 Missing ⚠️
diskann-bftree/src/provider.rs	86.95%	3 Missing ⚠️
diskann-benchmark/src/backend/index/search/knn.rs	75.00%	1 Missing ⚠️
diskann-garnet/src/lib.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1151      +/-   ##
==========================================
- Coverage   90.54%   89.46%   -1.09%     
==========================================
  Files         487      487              
  Lines       92370    92103     -267     
==========================================
- Hits        83640    82397    -1243     
- Misses       8730     9706     +976

Flag	Coverage Δ
miri	`89.46% <90.00%> (-1.09%)`	⬇️
unittests	`89.11% <90.00%> (-1.40%)`	⬇️

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

Files with missing lines	Coverage Δ
diskann-benchmark-core/src/search/graph/inline.rs	`98.79% <100.00%> (ø)`
diskann-benchmark-core/src/search/graph/mod.rs	`100.00% <ø> (ø)`
...iskann-benchmark-core/src/search/graph/multihop.rs	`98.73% <100.00%> (ø)`
diskann-benchmark/src/backend/index/result.rs	`49.03% <100.00%> (ø)`
diskann-disk/src/search/provider/disk_provider.rs	`93.69% <100.00%> (+0.01%)`	⬆️
diskann-providers/src/index/diskann_async.rs	`96.19% <100.00%> (-0.01%)`	⬇️
diskann-providers/src/index/wrapped_async.rs	`59.87% <100.00%> (ø)`
diskann/src/graph/misc.rs	`80.00% <ø> (ø)`
diskann/src/graph/search/knn_search.rs	`97.00% <100.00%> (-3.00%)`	⬇️
diskann/src/graph/test/cases/grid_insert.rs	`97.43% <100.00%> (+<0.01%)`	⬆️
... and 7 more

... and 52 files with indirect coverage changes

🚀 New features to boost your workflow:

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

hildebrandmw

Thanks Magdalen - I support this movement. Just a few requests on my end.

hildebrandmw · 2026-07-07T21:42:25Z

+pub struct KnnWrapper {
+    k_value: NonZeroUsize,
+    pub knn: graph::search::Knn,
+}


Can we change this to

pub struct KnnParams { num_neighbors: NonZeroUsize, knn: graph::search::Knn, } impl KnnParams { pub fn new(num_neighbors: NonZeroUsize, knn: graph::search::Knn) -> Result<Self, Error>; pub fn num_neighbors(&self) -> NonZeroUsize; pub fn knn(&self) -> &graph::search::Knn;

Reasons:

A pub field for knn undoes the validation check in the constructor.

Taking graph::search::Knn directly allows us to use a configurable beam width and simplifies the error.

num_neighbors is IMO slightly clearer.

Alternatively, the name Knn could work too since it's in a separate module, but in this case I think KnnParams is a little less overloaded. Maybe we could call it Params and access it as knn::Params? Any one of these works, but I'm not a fan of KnnWrapper.

hildebrandmw · 2026-07-07T21:44:38Z

    pub fn new(
        diverse_attribute_id: usize,
        diverse_results_k: usize,
+        original_k_value: usize,


Bikeshed: maybe total_k_value or total_nearest_neighbors instead of original_k_value?

hildebrandmw · 2026-07-07T21:46:22Z

+}
+
+#[cfg(feature = "experimental_diversity_search")]
+use imports::*;


This is an ... odd way of structuring this. This would be a lot cleaner if the parameters were grouped with the implementation of experimental_diversity_search rather than spread around. Can we do that shuffle in this PR?

hildebrandmw · 2026-07-07T21:50:57Z

+                    diskann::ANNErrorKind::IndexError,
+                    "search list size must be at least as large as the number of results requested",
+                ));
+            }


If we're going to throw an error here - does it make sense to move this to be the first check that happens? That way, all code-paths get to benefit and we can bail earlier if something looks wrong?

hildebrandmw · 2026-07-24T15:55:50Z

+    pub(super) use crate::{ANNError, ANNErrorKind};
+    pub(super) use std::num::NonZeroUsize;
+    pub(super) use thiserror::Error;
+}


A request: About 90% of the awkwardness with this import is because DiverseSearchParams doesn't belong here - it belongs in the diverse_search module. Do you mind moving it there? Alternatively, I can open a mini-PR to do that.

hildebrandmw · 2026-07-24T16:08:31Z

+pub struct KnnWrapper {
+    k_value: NonZeroUsize,
+    pub knn: graph::search::Knn,
+}


Alternatively, the name Knn could work too since it's in a separate module, but in this case I think KnnParams is a little less overloaded. Maybe we could call it Params and access it as knn::Params? Any one of these works, but I'm not a fan of KnnWrapper.

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

diskann-garnet/src/lib.rs:714

Unresolved merge conflict markers (<<<<<<< / ======= / >>>>>>>) and inconsistent variable names leave this function uncompilable. Resolve by keeping a single knn_params binding using the updated Knn::new(l_value, beam_width) signature.

<<<<<<< HEAD
    let params = match search::Knn::new(search_exploration_factor as usize, None) {
        Ok(params) => params,
=======
    let knn_params = match search::Knn::new(

+<<<<<<< HEAD
+    let params = match search::Knn::new(search_exploration_factor as usize, None) {
+=======
    let knn_params = match search::Knn::new(
        output_distances_len,
        search_exploration_factor as usize,
        None,
    ) {
+>>>>>>> 6caca35ddcfbacb2de38e57b7b53b7a7a95e1cf0
        Ok(params) => params,
        Err(_) => return -1,
    };


        let attribute_provider = self.diverse_params.attribute_provider.clone();
        let diverse_queue = DiverseNeighborQueue::new(
            self.inner.l_value().get(),
-            self.inner.k_value(),
-            self.diverse_params.diverse_results_k,
+            self.diverse_params.total_k_value,
+            self.diverse_params.diverse_results_k.get(),


+// #[cfg(feature = "experimental_diversity_search")]
+// pub use misc::DiverseSearchParams;


+        if search_list_size < return_list_size {
+            return Err(ANNError::message(
+                diskann::ANNErrorKind::IndexError,
+                "search list size must be at least as large as the number of results requested",
+            ));
+        }


Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

diskann/src/graph/search/diverse_search.rs:85

Add a parameter check to reject diverse_results_k > total_k_value so diverse search cannot be constructed with an impossible target result count.

        let diverse_results_k =
            NonZeroUsize::new(diverse_results_k).ok_or(DiverseSearchError::DiverseKZero)?;
        let total_k_value =
            NonZeroUsize::new(total_k_value).ok_or(DiverseSearchError::TotalKZero)?;

    let knn_params = match search::Knn::new(
-        output_distances_len,
        search_exploration_factor as usize,
        None,
    ) {


    let knn_params = match search::Knn::new(
-        output_distances_len,
        search_exploration_factor as usize,
        None,
    ) {


+pub enum DiverseSearchError {
+    #[error("total k_value cannot be zero")]
+    TotalKZero,
+    #[error("diverse k_value cannot be zero")]
+    DiverseKZero,
+}


+        if search_list_size < return_list_size {
+            return Err(ANNError::message(
+                diskann::ANNErrorKind::IndexError,
+                "search list size must be at least as large as the number of results requested",
+            ));
+        }


Magdalen Manohar added 16 commits May 14, 2026 17:56

finish up recall computation patch

97b36ef

Merge branch 'main' of github.com:microsoft/DiskANN

07b3671

Merge branch 'main' of github.com:microsoft/DiskANN

eac3ffb

fix conflict

17780f8

fix conflict

43eb517

Merge branch 'main' of github.com:microsoft/DiskANN

1d3a52b

Merge branch 'main' of github.com:microsoft/DiskANN

17eac62

Merge branch 'main' of github.com:microsoft/DiskANN

0ab4baa

Merge branch 'main' of github.com:microsoft/DiskANN

4ddca60

fix conflict

54ee01b

Merge branch 'main' of github.com:microsoft/DiskANN

9e1743f

Merge branch 'main' of github.com:microsoft/DiskANN

93504e6

Merge branch 'main' of github.com:microsoft/DiskANN

b7c27ce

remove k_value from Knn within diskann crate

acd1f8c

integrate changes into benchmark

0202348

fix merge conflict

a7b696f

magdalendobson marked this pull request as ready for review June 12, 2026 15:28

magdalendobson requested review from a team and Copilot June 12, 2026 15:28

Copilot started reviewing on behalf of magdalendobson June 12, 2026 15:28 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

magdalendobson and others added 4 commits June 12, 2026 11:36

Potential fix for pull request finding

0191fee

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Potential fix for pull request finding

2f28f50

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

move changes to bftree and disk

7ee7938

address changes in more crates

10940ad

harsha-simhadri approved these changes Jun 12, 2026

View reviewed changes

fmt

e0ca6f6

hildebrandmw reviewed Jul 7, 2026

View reviewed changes

hildebrandmw reviewed Jul 24, 2026

View reviewed changes

Magdalen Manohar added 2 commits July 24, 2026 17:50

fix merge conflict

d664e44

changes requested in review

dc0145e

Copilot AI review requested due to automatic review settings July 24, 2026 18:52

Copilot started reviewing on behalf of magdalendobson July 24, 2026 18:53 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

fix problems, respond to other PR comments

a5fa47a

Copilot AI review requested due to automatic review settings July 24, 2026 20:47

Copilot started reviewing on behalf of magdalendobson July 24, 2026 20:47 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

		// #[cfg(feature = "experimental_diversity_search")]
		// pub use misc::DiverseSearchParams;

Uh oh!

Conversation

magdalendobson commented Jun 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hildebrandmw left a comment

Choose a reason for hiding this comment

Uh oh!

hildebrandmw Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

hildebrandmw Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

hildebrandmw Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

hildebrandmw Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

hildebrandmw Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

hildebrandmw Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

hildebrandmw Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jun 12, 2026 •

edited

Loading