Remove the owned runtime and synchronous DiskIndexSearcher API - #1388
Remove the owned runtime and synchronous DiskIndexSearcher API#1388Wei Wu (wuw92) wants to merge 2 commits into
Conversation
Make disk search asynchronous, move runtime ownership to callers, and exercise the public search API in builder and search tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new current-thread runtime usage is shared across Rayon threads in tests/benchmarks, which can serialize execution and skew multi-thread behavior and benchmark results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR removes DiskIndexSearcher’s owned Tokio runtime and synchronous search wrappers, standardizing disk-index search on an async API so runtime/executor management is owned by callers (e.g., benchmarks/tests).
Changes:
- Removed the
Runtimefield andruntime: Option<Runtime>constructor argument fromDiskIndexSearcher. - Converted
searchandsearch_with_indexed_vectorstoasync fn, consolidating dispatch into a single asyncsearch_internalhelper. - Updated builder verification, benchmarks, and tests to await the async API or to drive it using an externally managed runtime.
File summaries
| File | Description |
|---|---|
| diskann-disk/src/search/provider/disk_provider.rs | Removes runtime ownership, makes search APIs async, and updates tests to use async calls / external runtimes. |
| diskann-disk/src/build/builder/core.rs | Updates ground-truth verification to drive async search via an external runtime. |
| diskann-benchmark/src/disk_index/search.rs | Updates benchmark execution to drive async search via a runtime managed by the benchmark code. |
Review details
Suppressed comments (1)
diskann-disk/src/search/provider/disk_provider.rs:1992
- Same concern as
test_disk_search: a single current-thread runtime shared across Rayon workers can serializeblock_oncalls and undermine the multi-threaded associated-data search test. Prefer a multi-thread runtime (or one runtime per worker thread) when driving async work from multiple threads.
let runtime = tokio::runtime::Builder::new_current_thread()
.build()
.unwrap();
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1388 +/- ##
==========================================
+ Coverage 92.55% 92.58% +0.02%
==========================================
Files 522 522
Lines 100890 101155 +265
==========================================
+ Hits 93377 93652 +275
+ Misses 7513 7503 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Return impl SendFuture from both public search methods so Send is checked at the API boundary while retaining async internal helpers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reference Issues/PRs
Fixes #1330.
What does this implement/fix? Briefly explain your changes.
Remove the owned Tokio runtime and synchronous search interface from
DiskIndexSearcher.searchandsearch_with_indexed_vectorsare asynchronous, and all search modes await the underlying operations directly. Synchronous benchmark execution manages its runtime at the caller.Search modes, result layouts, filtering, and parameter validation remain unchanged.
Any other comments?
None.