Add synchronous load_with constructors and run escape hatch to wrapped_async#864
Add synchronous load_with constructors and run escape hatch to wrapped_async#864suri-kumkaran wants to merge 2 commits intomainfrom
load_with constructors and run escape hatch to wrapped_async#864Conversation
There was a problem hiding this comment.
Pull request overview
Adds synchronous entry points to load prebuilt async DiskANN indexes in diskann-providers, and introduces a run escape hatch for calling async-only inner methods from the sync wrapper.
Changes:
- Added
load_with_multi_thread_runtime,load_with_current_thread_runtime, andload_with_handlesync constructors that wrapLoadWith::load_with. - Added
runto execute an arbitrary async closure on the wrapper’s runtime/handle. - Added an end-to-end save→sync-load→search round-trip test and refactored runtime creation into helpers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #864 +/- ##
==========================================
- Coverage 90.45% 89.28% -1.17%
==========================================
Files 442 442
Lines 83248 83372 +124
==========================================
- Hits 75301 74439 -862
- Misses 7947 8933 +986
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
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Construct a synchronous `DiskANNIndex` with its own `tokio::runtime::Runtime`. | ||
| /// | ||
| /// A default configured runtime that uses the curren thread will be created and used behind the scenes. To use | ||
| /// a specific Toktio runtime, use `DiskANNIndex::new_with_multi_thread_runtime()` or `DiskANNIndex::new_with_handle()`. | ||
| /// A default configured runtime that uses the current thread will be created and used behind the scenes. To use | ||
| /// a specific Tokio runtime, use `DiskANNIndex::new_with_multi_thread_runtime()` or `DiskANNIndex::new_with_handle()`. | ||
| pub fn new_with_current_thread_runtime(config: graph::Config, data_provider: DP) -> Self { |
There was a problem hiding this comment.
The doc comment for new_with_current_thread_runtime points callers to new_with_multi_thread_runtime() for using a “specific Tokio runtime”, but neither method accepts a caller-provided runtime. Consider rephrasing to point to new_with_handle() for an external runtime handle, and to the other constructor only for selecting the runtime flavor (multi-thread vs current-thread).
| /// Construct a synchronous `DiskANNIndex` with its own `tokio::runtime::Runtime`. | ||
| /// | ||
| /// A default configured multi-threaded runtime will be created and used behind the scenes. To use | ||
| /// a specific Toktio runtime, use `DiskANNIndex::new_with_multi_thread_runtime()` or `DiskANNIndex::new_with_handle()`. | ||
| /// a specific Tokio runtime, use `DiskANNIndex::new_with_multi_thread_runtime()` or `DiskANNIndex::new_with_handle()`. | ||
| pub fn new_with_multi_thread_runtime(config: graph::Config, data_provider: DP) -> Self { |
There was a problem hiding this comment.
The doc comment for new_with_multi_thread_runtime says that to use a specific Tokio runtime you should call new_with_multi_thread_runtime() or new_with_handle(), but this method itself always creates a default runtime. Please update the wording to distinguish between (a) choosing the multi-thread vs current-thread default runtime and (b) supplying an externally-owned runtime via new_with_handle().
Why
Sync callers who need to load a prebuilt index must manually create a tokio runtime and call the async
LoadWithtrait — leaking async internals. Similarly, there's no way to call an async method on the inner index if a sync wrapper doesn't exist for it yet.What
All changes are in
diskann-providers/src/index/wrapped_async.rs.load_with_multi_thread_runtime,load_with_current_thread_runtime,load_with_handle— Sync constructors that load a prebuilt index from storage, mirroring the existingnew_with_*pattern.run— Escape hatch: runs any async closure against the inner index on the wrapper's runtime, for methods that don't have a dedicated sync wrapper.create_multi_thread_runtime/create_current_thread_runtimehelpers to reduce duplication.test_save_then_sync_load_round_trip— End-to-end test: build → insert → save (viarun) → load → search.