Repository navigation
[diskann-garnet] Implement XVCREATE, XVIMPORT - #1455
Jack Moffitt (metajack) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Arbitrary-ID imports, malformed neighbor counts, reserved-ID mappings, and quantizer-state replacement can currently corrupt or fail imported indexes.
Review effort: Balanced
Findings: 4
Open (6)
What changed in this PR
Adds bulk import support for externally generated DiskANN graphs through new Garnet FFI operations.
Changes:
- Adds quantizer-state injection and term import/finalization APIs.
- Persists import eligibility and validates imported graph terms.
- Adds import tests, documentation, and a patch version bump.
| File | Description |
|---|---|
Cargo.lock |
Updates the package version. |
diskann-garnet/Cargo.toml |
Bumps the crate version. |
diskann-garnet/diskann-garnet.nuspec |
Bumps the NuGet version. |
diskann-garnet/README.md |
Documents bulk import usage. |
diskann-garnet/docs/ffi-design.rs |
Documents import FFI APIs. |
diskann-garnet/docs/data-design.md |
Documents imported-term storage. |
diskann-garnet/src/dyn_index.rs |
Exposes import operations through DynIndex. |
diskann-garnet/src/ffi_tests.rs |
Adds import and FFI validation tests. |
diskann-garnet/src/fsm.rs |
Adds explicit internal-ID claiming. |
diskann-garnet/src/garnet.rs |
Adds term conversion and existence checks. |
diskann-garnet/src/lib.rs |
Exposes import-related C APIs. |
diskann-garnet/src/provider.rs |
Implements import gating, validation, and finalization. |
diskann-garnet/src/quantization.rs |
Supports externally supplied quantizer state. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
50b606d to
84d419c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1455 +/- ##
==========================================
+ Coverage 90.93% 90.98% +0.04%
==========================================
Files 583 586 +3
Lines 116854 119020 +2166
==========================================
+ Hits 106266 108294 +2028
- Misses 10588 10726 +138
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
d99ff9a to
dd739d1
Compare
Mark Hildebrand (hildebrandmw)
left a comment
Mark Hildebrand (hildebrandmw)
left a comment
There was a problem hiding this comment.
My biggest concern is about the dynamic disabling of imports. Adding/requiring that dynamic check on all function seems somewhat hard to maintain - seems like it could race and actually successfully complete before any call to finish_import.
| fn external_id_exists(&self, context: &Context, id: &GarnetId) -> bool { | ||
| if !self.inner.provider().disable_import(context) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Is there a better way of structuring this than slapping
if !self.inner.provider().disable_import(context) on all these methods? It seems ... really stateful and easy to forget if new methods are needed.
There was a problem hiding this comment.
I don't actually have a good way to do that. I tried to keep everything contained to dyn_index since it is essentially the central place where all hte proxied provider functions go.
| /// This is called on most calls that use the index so that imports can only happen on | ||
| /// empty indices. | ||
| pub(crate) fn disable_import(&self, context: &Context) -> bool { | ||
| if !*self.import_enabled.read().unwrap() { |
There was a problem hiding this comment.
This is somewhat tertiary, but it's related to the decision that read methods disable future imports.
Acquiring a read guard requires at least a compare-exchange on some atomic, which in turn requires exclusive ownership of a shared cache line. If all threads are doing this, this can cause the cache line to bounce around all over the place and be slow.
Now, I know that diskann-garnet uses a bunch of locks for various pieces, but even read lock acquisition is not free under contention.
Atomically reading a value is much less stressful as the cache line holding it can stay shared.
So my questions are:
- Is any read action disabling imports really desirable? Or would it make sense for there to be an explicit "disable imports" call.
- In either case, would it make sense to keep the happy path read-only, like
struct Provider {
//...
import_disabled: AtomicBool,
import_lock: RwLock<()>,
}
impl Provider {
fn disable_import(&self) -> bool {
// happy path - read only.
if self.import_disabled.load(Orderind::Acquire) {
return true;
}
// Slower path involving locks.
}
}I mainly want to bring up awareness of this pattern 😄
| context: &Context, | ||
| task_idx: usize, | ||
| task_count: usize, | ||
| ) -> (bool, Option<bool>) { |
There was a problem hiding this comment.
Any reason to not have some kind of diagnostic for the failure reason? As in, how would one debug this if this failed?
| // verification. When they all are done, all index terms exist, but we still need | ||
| // to set up the start points and caches. | ||
| let import_finished = | ||
| self.imports_completed.fetch_add(1, Ordering::AcqRel) + 1 == task_count as u64; |
There was a problem hiding this comment.
This doesn't precisely track completion in that it doesn't differentiate the case if the same task_idx is provided multiple times.
I don't really have a strong feeling on this, but you could short it up by using Vec<AtomicBool> (or an atomic bit-set, or even an AtomicU64 if you're fine with capping the number of tasks at 64) and set bits for tasks that have completed.
Probably not a huge deal, but a bit of extra checking doesn't hurt, especially when the surrounding code is as expensive as it is.
| if !self.callbacks.write_iid(&context.term(Term::Vector), 0, &v) { | ||
| return (true, Some(false)); | ||
| } | ||
| // NOTE: unwrap will succeed because it is gated on the quantizer. |
There was a problem hiding this comment.
Couldn't you use
if let Some(qv) = qv instead and avoid the unwrap?
| .map_err(|e| GarnetQuantizerError::Compression(Box::new(e)))?; | ||
| self.inner | ||
| .read() | ||
| .unwrap() |
There was a problem hiding this comment.
Have you thought of using parking_lot for its poison-free synchronization primitives?
|
|
||
| At the FFI layer, `can_import` checks eligibility. | ||
|
|
||
| Ordinary operations (`card`, search, ID checks) and `FINISH` permanently disable importing. |
There was a problem hiding this comment.
Would it simplify the design to not have this dynamic behavior and instead require an explicit function to switch from importing to serving?
dd739d1 to
44a1107
Compare
This PR adds direct import of externally stored diskann graphs to Garnet. This can be useful for interoperating with indices created with other DiskANN systems.
It adds two new commands,
XVCREATEto create a vector set without doing aVADD, andXVIMPORTto directly import index terms to a vector set. To make quantizer state compatible,XVCREATEallows directly specifying the quantizer state.Finalization of import checks that all required terms are present and have sensical sizes, but does not verify quantize vectors match unquantized vectors or that the neighbor lists make sense. It is expected that the graph is correctly constructed in the external system.
This currently requires that the internal ID 0 not be used in the graph since it will get used by the start point once the graph is imported.