Skip to content

Removing debug_provider.rs in favor of graph/test/provider#875

Draft
JordanMaples wants to merge 4 commits intomainfrom
user/jordanmaples/remove_debugprovider
Draft

Removing debug_provider.rs in favor of graph/test/provider#875
JordanMaples wants to merge 4 commits intomainfrom
user/jordanmaples/remove_debugprovider

Conversation

@JordanMaples
Copy link
Copy Markdown

@JordanMaples JordanMaples commented Apr 2, 2026

This is an attempt at addressing the observations in #852 that the Debug Provider is largely redundant given that the majority of the tests are already covered by the graph/test/provider. Copilot already attempted to address this in #855, however it deleted a number of needed tests from model/graph/provider/_async/caching/example.rs.

The bulk of the changes were in enabling the example.rs file to be tested without any dropped coverage, as was the problem with the other PR that attempted this migration. This necessitated a number of changes in the provider to allow parity.

A DefaultContextProvider and DefaultContextStrategy structure were added and extended with features as needed to get the tests to compile.

Here is a copilot summary of the various added impls:

  1. SearchStrategy — needed by grid_search tests (any search operation)
  2. DefaultPostProcessor — needed because CachingProvider delegates post-processing through the inner strategy
  3. PruneStrategy — needed by grid_search_with_build (insert triggers pruning)
  4. InsertStrategy — needed by grid_search_with_build (inserting vectors into the index)
  5. MultiInsertStrategy — needed by grid_search_with_build (batch insert path)
  6. InplaceDeleteStrategy — needed by the inplace_delete test

I needed to guide copilot through the majority of the changes and migrations to get through the compiler errors and get the tests to execute as expected. I'm still fairly new to rust please forgive me if I missed something terribly wrong while I reviewed the code it generated.

Any other comments?

This Fixes #852 and Closes #855

@JordanMaples
Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree company="Microsoft"

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

This PR removes the async DebugProvider from diskann-providers and migrates remaining tests/examples to use the shared diskann::graph::test::provider infrastructure (including adding adapters to make the test provider compatible with diskann-providers async test utilities that require DefaultContext).

Changes:

  • Remove debug_provider module and delete debug_provider.rs from diskann-providers.
  • Extend diskann::graph::test::provider with caching support (CacheableAccessor) and add DefaultContextProvider/DefaultContextStrategy adapters.
  • Update diskann-providers caching example/tests to use the test provider adapters; adjust Cargo features accordingly.

Reviewed changes

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

Show a summary per file
File Description
diskann/src/graph/test/provider.rs Adds accessor caching support and introduces DefaultContextProvider/DefaultContextStrategy to interop with diskann-providers test infra.
diskann-providers/src/model/graph/provider/async_/mod.rs Stops exporting the removed debug_provider module.
diskann-providers/src/model/graph/provider/async_/debug_provider.rs Deletes the redundant provider implementation and its tests.
diskann-providers/src/model/graph/provider/async_/caching/example.rs Migrates caching example/tests from DebugProvider to the test provider adapters.
diskann-providers/Cargo.toml Enables diskann testing feature to access the test provider types.
.gitignore Ignores generated label files from synthetic label tests.

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

B: crate::graph::SearchOutputBuffer<<Accessor<'a> as provider::HasId>::Id> + Send + ?Sized,
Next: glue::SearchPostProcess<Accessor<'a>, T> + Sync,
{
let filtered = candidates.filter(|n| !accessor.provider.is_deleted(n.id).unwrap_or(false));
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

FilterDeletedIds builds filtered using a closure that captures accessor (accessor.provider...). That keeps an immutable borrow of accessor alive for the lifetime of the iterator, but next.post_process(...) needs &mut accessor, which will fail to compile due to conflicting borrows. Copy the provider reference out first (e.g., into a local provider binding) and have the filter closure capture that instead of accessor.

Suggested change
let filtered = candidates.filter(|n| !accessor.provider.is_deleted(n.id).unwrap_or(false));
let provider = accessor.provider;
let filtered = candidates.filter(|n| !provider.is_deleted(n.id).unwrap_or(false));

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

the comment that this will fail to compile is confusing as I'm able to fully compile and test. Is copilot just confused here?

B: crate::graph::SearchOutputBuffer<<Accessor<'a> as provider::HasId>::Id> + Send + ?Sized,
Next: glue::SearchPostProcess<Accessor<'a>, T> + Sync,
{
let filtered = candidates.filter(|n| !accessor.provider.is_deleted(n.id).unwrap_or(false));
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

unwrap_or(false) here means that if is_deleted returns an error (e.g., the ID is missing), the candidate will be treated as not deleted and will remain in the stream. For delete/rewrite flows it’s usually safer to exclude unknown IDs (or propagate the error) to avoid returning stale/invalid nodes. Consider treating errors as deleted (or mapping the error into the step’s error type) rather than defaulting to false.

Suggested change
let filtered = candidates.filter(|n| !accessor.provider.is_deleted(n.id).unwrap_or(false));
let filtered = candidates.filter(|n| !accessor.provider.is_deleted(n.id).unwrap_or(true));

Copilot uses AI. Check for mistakes.
CachingError::Inner(e)
}
test_provider::AccessError::Transient(e) => {
panic!("unexpected transient error: {e}")
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

CachedFill is documented to eagerly propagate all inner errors, but this maps AccessError::Transient to a panic!, and (because TransientAccessError asserts it was acknowledged/escalated on drop) that panic path can also trigger a second panic during unwinding. It’s safer to convert transient errors into CachingError::Inner(...) (acknowledging/escalating as needed) instead of panicking.

Suggested change
panic!("unexpected transient error: {e}")
CachingError::Inner(e.escalate())

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since this only in test code, do we care about the potential double panic?

JordanMaples and others added 2 commits April 2, 2026 15:28
Delete diskann-providers' DebugProvider and migrate all 5 caching
example tests (grid_search, grid_search_with_build, inplace_delete,
test_uncacheable, and the CacheableAccessor compile-time check) to use
diskann's test provider infrastructure instead.

Changes:
- diskann/src/graph/test/provider.rs: Add DefaultContextProvider,
  DefaultContextStrategy, FilterStartPoints post-processor,
  FilterDeletedIds, InplaceDeleteStrategy, and CacheableAccessor impl
  for Accessor.
- diskann-providers/src/model/graph/provider/async_/caching/example.rs:
  Rewrite tests to use the new provider types.
- Delete debug_provider.rs and remove its module declaration.
- Remove redundant neighbor_writes counter; assertions now use the
  granular set_neighbors and append_neighbors counters directly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The generate_label_test in diskann-providers writes output files to the
cwd using relative paths. Ignore the resulting rand_labels_50_10K_*.txt
files so they don't get accidentally committed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@JordanMaples JordanMaples force-pushed the user/jordanmaples/remove_debugprovider branch from d2b6624 to bcebe41 Compare April 2, 2026 22:29
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 2, 2026

Codecov Report

❌ Patch coverage is 70.69597% with 80 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.26%. Comparing base (0ced23d) to head (a7b6f06).

Files with missing lines Patch % Lines
diskann/src/graph/test/provider.rs 70.69% 80 Missing ⚠️

❌ Your patch status has failed because the patch coverage (70.69%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #875      +/-   ##
==========================================
- Coverage   89.31%   89.26%   -0.06%     
==========================================
  Files         445      444       -1     
  Lines       84095    83777     -318     
==========================================
- Hits        75113    74785     -328     
- Misses       8982     8992      +10     
Flag Coverage Δ
miri 89.26% <70.69%> (-0.06%) ⬇️
unittests 89.10% <70.69%> (-0.06%) ⬇️

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

Files with missing lines Coverage Δ
diskann/src/graph/test/provider.rs 88.95% <70.69%> (-4.89%) ⬇️

... and 2 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 and others added 2 commits April 2, 2026 16:06
Cover the new test infrastructure added for the DebugProvider removal:
- DefaultContextProvider: deref, id conversion, delete/status, set_element,
  default_accessor
- DefaultContextStrategy: Default impl, search_accessor
- Accessor::provider() getter
- CacheableAccessor round-trip

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@harsha-simhadri
Copy link
Copy Markdown
Contributor

@JordanMaples could you please update PR description with the strategy and overview of the changes,

@JordanMaples JordanMaples marked this pull request as draft April 3, 2026 16:04
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.

Remove the Debug Provider

4 participants