Skip to content

migrating multi-hop tests from diskann-providers to diskann#928

Merged
JordanMaples merged 2 commits into
mainfrom
jordanmaples/migrate_unittests
May 26, 2026
Merged

migrating multi-hop tests from diskann-providers to diskann#928
JordanMaples merged 2 commits into
mainfrom
jordanmaples/migrate_unittests

Conversation

@JordanMaples
Copy link
Copy Markdown
Contributor

@JordanMaples JordanMaples commented Apr 8, 2026

This is a first step in the effort to migrate unit tests from diskann_async to diskann #927. If you don't think tests that have been brought over should be moved, they probably shouldn't have been. Please point them out and I'll do my best to put them back where I found them.

  • Remove multihop and filter-related tests from diskann_async.rs
  • Add multihop.rs test module in diskann with equivalent coverage using test_provider::Provider::grid() instead
    of the inmem provider
  • Add baseline JSON files for callback filtering, even filtering, and two-hop-through-non-matching scenarios
  • Make multihop_search module visible to tests (pub(crate) export in search/mod.rs)

Key differences from the original tests:

  • Tests exercise the graph algorithm directly via Provider::grid() rather than going through the inmem provider
    with PQ quantization — appropriate since these test traversal behavior, not quantization
  • Start point filtering: the test provider doesn't strip start points from results (unlike the inmem
    post-processor), so reject_all_returns_zero_results asserts zero non-start-point results rather than zero total
    results

@JordanMaples JordanMaples force-pushed the jordanmaples/migrate_unittests branch 2 times, most recently from 5a59f22 to 5655eea Compare April 9, 2026 21:12
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 9, 2026

Codecov Report

❌ Patch coverage is 95.37445% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.60%. Comparing base (3d09483) to head (0c77da1).

Files with missing lines Patch % Lines
diskann/src/graph/test/cases/multihop.rs 95.11% 21 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #928      +/-   ##
==========================================
+ Coverage   89.46%   90.60%   +1.13%     
==========================================
  Files         458      459       +1     
  Lines       85530    85452      -78     
==========================================
+ Hits        76522    77425     +903     
+ Misses       9008     8027     -981     
Flag Coverage Δ
miri 90.60% <95.37%> (+1.13%) ⬆️
unittests 90.56% <95.37%> (+1.47%) ⬆️

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

Files with missing lines Coverage Δ
diskann-providers/src/index/diskann_async.rs 96.37% <ø> (+0.37%) ⬆️
diskann/src/graph/search/multihop_search.rs 99.44% <100.00%> (+1.36%) ⬆️
diskann/src/graph/test/cases/multihop.rs 95.11% <95.11%> (ø)

... and 42 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JordanMaples JordanMaples force-pushed the jordanmaples/migrate_unittests branch from 8105440 to b7d238b Compare April 13, 2026 15:12
@JordanMaples JordanMaples marked this pull request as ready for review April 14, 2026 16:22
@JordanMaples JordanMaples requested review from a team and Copilot April 14, 2026 16:22
@JordanMaples JordanMaples changed the title [partial][draft][in-progress] migrating unit tests migrating multi-hop tests from diskann-provider to diskann Apr 14, 2026
@JordanMaples JordanMaples changed the title migrating multi-hop tests from diskann-provider to diskann migrating multi-hop tests from diskann-async to diskann Apr 14, 2026
@JordanMaples JordanMaples changed the title migrating multi-hop tests from diskann-async to diskann migrating multi-hop tests from diskann-providers to diskann Apr 14, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Migrates a first set of multi-hop search traversal unit tests from diskann_async into the diskann crate’s graph test suite, and introduces shared search ground-truth utilities to support those tests.

Changes:

  • Added multihop test cases under diskann/src/graph/test/cases/ and wired them into the test module.
  • Introduced diskann::graph::test::search_utils with ground-truth + assertion helpers for search verification.
  • Removed the migrated multi-hop test helpers/cases from diskann-providers/src/index/diskann_async.rs and adjusted diskann-providers test utils module visibility/docs.

Reviewed changes

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

Show a summary per file
File Description
diskann/src/graph/test/search_utils.rs Adds ground-truth computation and assertion helpers for graph search tests.
diskann/src/graph/test/mod.rs Exposes the new search_utils module in the graph test module.
diskann/src/graph/test/cases/multihop.rs Adds migrated multi-hop traversal/filtering/termination/callback tests.
diskann/src/graph/test/cases/mod.rs Registers the new multihop test module.
diskann-providers/src/test_utils/search_utils.rs Updates/clarifies docs around duplicated ground-truth helpers for provider-side tests.
diskann-providers/src/test_utils/mod.rs Makes search_utils publicly accessible from diskann-providers::test_utils.
diskann-providers/src/index/diskann_async.rs Removes migrated multi-hop-related tests and supporting helpers.

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

Comment thread diskann/src/graph/test/search_utils.rs Outdated
Comment thread diskann/src/graph/test/search_utils.rs Outdated
Comment thread diskann/src/graph/test/cases/multihop.rs Outdated
Comment thread diskann/src/graph/test/cases/multihop.rs Outdated
Comment thread diskann-providers/src/test_utils/search_utils.rs Outdated
Copy link
Copy Markdown
Contributor

@hildebrandmw hildebrandmw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Jordan! This is moving the tests in the right direction, but we need to be careful about just moving the test infrastructure from diskann_async.rs as-is.
The utilities in search_utils.rs are extremely awkward for actually running tests, are very sensitive, and don't provide much useful information when they fire.
They've been used in diskann_async.rs because it was kind of the best thing we had back then.

My hope is that the new diskann can take a higher signal approach using baselines and VerboseEq.
Not only does this provide a really good way of viewing the expected results as a whole, it's also great for storing additional metrics.
For example, the stats, ids, and distances from multi-hop search can all be checked in as part of the baseline and get protected for free.

My ask is to not migrate the search_utils.rs as is - especially if it means including test methods that aren't actually used by diskann.
Also, use the baseline capturing mechanism to capture everything about both test setups and results.
We cannot rely solely on the baseline to protect against regression (someone could check-in a broken baseline in the future), but a baseline in combination with some invariant checks (returned items should be filtered/adjusted) will go a long way toward good tests.

Comment thread diskann/src/graph/test/cases/multihop.rs Outdated
Comment thread diskann/src/graph/test/cases/multihop.rs Outdated
Comment thread diskann/src/graph/test/cases/multihop.rs Outdated
Comment thread diskann/src/graph/test/cases/multihop.rs Outdated
Comment thread diskann/src/graph/test/cases/multihop.rs Outdated
@JordanMaples JordanMaples marked this pull request as draft April 16, 2026 14:37
@JordanMaples
Copy link
Copy Markdown
Contributor Author

converting back to draft as it needs some more human refinement

@JordanMaples
Copy link
Copy Markdown
Contributor Author

@copilot would you mind taking another review over this pr as it has changed significantly from your previous review.

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 20, 2026

Thanks Jordan! This is moving the tests in the right direction, but we need to be careful about just moving the test infrastructure from diskann_async.rs as-is.
The utilities in search_utils.rs ar...

Addressed in 9c85d26: diskann no longer carries the migrated search_utils.rs, multihop tests now use checked-in baselines with VerboseEq, and invariants remain in-test for filtered/adjusted behavior.

@copilot would you mind taking another review over this pr as it has changed significantly from your previous review.

I re-reviewed the updated PR state and the major requested shifts are present (baseline-driven multihop tests + removal of migrated diskann search utils) in 9c85d26 and follow-up cleanup in a77f01a.

@JordanMaples JordanMaples marked this pull request as ready for review April 20, 2026 19:22
- Add unit tests for multihop search and NotInMutWithLabelCheck predicate
- Add two-hop reachability test for multihop search
- Extract shared helpers in multihop tests
- Rework multihop tests with baselines, remove search_utils from providers
- Remove stale duplication comment from diskann-providers search_utils
- Revert cosmetic search_utils.rs changes and make mod private
@JordanMaples JordanMaples force-pushed the jordanmaples/migrate_unittests branch from 8b6c08f to ae4e60a Compare May 21, 2026 14:42
@JordanMaples JordanMaples enabled auto-merge (squash) May 26, 2026 15:30
@JordanMaples JordanMaples linked an issue May 26, 2026 that may be closed by this pull request
Copy link
Copy Markdown
Contributor

@hildebrandmw hildebrandmw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Jordan - sorry this fell through the cracks!

@JordanMaples JordanMaples merged commit f083035 into main May 26, 2026
25 of 26 checks passed
@JordanMaples JordanMaples deleted the jordanmaples/migrate_unittests branch May 26, 2026 15:37
arkrishn94 added a commit that referenced this pull request May 28, 2026
# DiskANN v0.53.0 Release Notes

## Breaking Changes

An AI generated, human reviewed list of changes is summarized below.

### Paged search overhauled — channel-based API
([#1078](#1078))

`PagedSearchState` and its `'static`-bound pause/resume model have been
replaced with an async, channel-based interface. The recommended way to
drive paged search is now via a `tokio::sync::mpsc` channel, with the
searcher embedded in an otherwise-`'static` future. See the [rendered
RFC](https://github.com/microsoft/DiskANN/blob/main/rfcs/01078-paged-search.md)
for the new shape. Callers wired against `PagedSearchState` must migrate
to the channel API.

Users of paged search via `wrapped_async::DiskANNIndex` that know their
inner futures will never suspend can use the new
`wrapped_async::DiskANNIndex::paged_search_no_await`; this will
efficiently run paged searches with minimal synchronization overhead.

### `DiskANNIndex::flat_search` removed
([#1076](#1076))

`DiskANNIndex::flat_search` and the `IdIterator` trait have been removed
from the `diskann` crate. Equivalent functionality lives on the new
inherent method `DiskIndexSearcher::flat_search` in `diskann-disk`. This
unblocks the experimental directions in #1067 and #983.

```rust
// Before
diskann_index.flat_search(query, ...)?;

// After
disk_index_searcher.flat_search(query, ...).await?;
```

### `DiskIndexSearcher::flat_search` now batched
([#1097](#1097))

The new `DiskIndexSearcher::flat_search` uses the bulk `pq_distances`
path instead of one-vector-at-a-time `Accessor::build_query_computer` +
`evaluate_similarity`. Downstream behavior is equivalent but tighter
resource bounds apply.

### `centroid` removed from PQ interfaces
([#1010](#1010))

The dataset-centroid argument has been removed from `FixedChunkPQTable`
construction, `populate`, and most other PQ APIs. The shift only ever
worked for L2 distance and was silently ignored for inner-product /
cosine, so passing it was a footgun. When an L2 shift is required, fold
it into the PQ pivots instead (the library now does this internally).

```rust
// Before
let table = FixedChunkPQTable::new(.., centroid, ..);

// After — drop the centroid argument
let table = FixedChunkPQTable::new(.., ..);
```

### Flat search interface
([#983](#983))

A new `flat` module in `diskann` adds a provider-agnostic brute-force
search surface, mirroring the shape of graph search. Backends implement
a single trait, `DistancesUnordered<C>` (in `flat/strategy.rs`), which
fuses iteration and distance computation, allowing any backend
(in-memory, quantized, disk, remote) to plug into a shared algorithm.
See the [rendered
RFC](https://github.com/microsoft/DiskANN/blob/main/rfcs/00983-flat-search.md).
This is additive but is the new canonical surface — direct ad-hoc
flat-search call sites should migrate.

### `bf_tree` extracted into `diskann-bftree` crate
([#1020](#1020))

The bf_tree provider has been moved out of `diskann-providers`
(previously at
`diskann-providers/src/model/graph/provider/async_/bf_tree/`) into a new
standalone `diskann-bftree` crate. Along with the move:

- Switched from PQ to spherical quantization.
- Dropped dependencies on `DeletionCheck`, `AsDeletionCheck`, and
`RemoveDeletedIdsAndCopy`.
- Simplified generics.

Consumers must update their `Cargo.toml` to depend on `diskann-bftree`
and update import paths.

### `direct_distance_impl` and `inner_product_raw` re-exposed
([#1081](#1081))

`direct_distance_impl` (free function) and
`FixedChunkPQTable::inner_product_raw` are `pub` again after being
privatized in #1044. Restored to unblock a downstream user. Not breaking
in the typical direction — this restores previously available API
surface.

### MinMax `recompress` takes a grid-scale parameter
([#1109](#1109))

The MinMax `recompress` API now accepts a grid-scale parameter. 

## New Features

- SIMD-optimized L2-squared norm
([#1107](#1107))
- Significantly faster bitmap computation
([#1099](#1099))
- Large speedup on the bitmap construction path used by filtered search.
- LLVM IR bloat regression check in CI
([#1083](#1083))
- CI now flags regressions in generated LLVM IR size, helping catch
unintended monomorphization blow-ups.
- Recall computation fix for under-k groundtruth
([#1069](#1069))

## Merged PRs

* Revise README for DiskANN3 by @harsha-simhadri in
#1046
* [CI] Try to fix publishing step by @hildebrandmw in
#1057
* [benchmark] Remove `DispatchRule` by @hildebrandmw in
#1064
* [benchmark] Automatic Input Registration by @hildebrandmw in
#1066
* Remove centroid from most PQ interfaces by @hildebrandmw in
#1010
* [diskann/disk] Remove `flat_search` from `DiskANNIndex` by
@hildebrandmw in #1076
* macos build and miri check to nightly by @harsha-simhadri in
#1058
* [API] Make some methods public again by @hildebrandmw in
#1081
* [benchmark] Simply `Inputs` more by @hildebrandmw in
#1077
* Turn on stack protection for the diskann-garnet NuGet build by
@jackmoffitt in #1082
* Fix options for diskann-garnet nuget pipeline by @jackmoffitt in
#1091
* [CI] add LLVM IR bloat regression check by @arazumov in
#1083
* Bump openssl from 0.10.79 to 0.10.80 by @dependabot[bot] in
#1093
* [Disk CI benchmarks] Use 1ES.Pool=diskann-github by @arazumov in
#869
* Fix recall computation for fewer than k groundtruth results by
@magdalendobson in #1069
* bf_tree migration away from diskann-providers by @JordanMaples in
#1020
* [RFC/diskann] Overhaul paged search by @hildebrandmw in
#1078
* Remove unsafe code from compute_vec_l2sq by @arazumov in
#1094
* Remove direct accessor call in `diskann-garnet` by @hildebrandmw in
#1098
* Refactor `DiskIndexSearcher::flat_search` to use batching by
@hildebrandmw in #1097
* [flat index] Flat Search Interface by @arkrishn94 in
#983
* migrating multi-hop tests from diskann-providers to diskann by
@JordanMaples in #928
* Significantly speed up bitmap computation by @magdalendobson in
#1099
* `compute_vecs_l2sq`: Replace scalar L2 Squared norm with
SIMD-optimized FastL2NormSquared by @arazumov in
#1107
* [minmax] Add grid scaling to recompress API by @arkrishn94 in
#1109

**Full Changelog**:
v0.52.0...v0.53.0
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.

Test migration from diskann-providers to diskann

6 participants