Supporting spherical quantization builds for the disk index - #1331
Conversation
There was a problem hiding this comment.
Pull request overview
Adds spherical quantization as an additional quantization mode for disk index builds, wiring 1-bit spherical quantization through the disk build pipeline (training, in-memory graph construction, and RAM estimation) and extending builder tests to cover additional metrics.
Changes:
- Extend disk build quantization configuration to support
SPHERICAL_<nbits>and add serialization/parse tests. - Train and use a 1-bit spherical quantizer during disk index in-memory build (one-shot and merged/sharded paths).
- Account for spherical vector storage in build RAM estimation and add builder test coverage for L2/IP/Cosine.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| diskann-quantization/src/spherical/quantizer.rs | Adds a try_clone() convenience API for independently allocated quantizer copies. |
| diskann-disk/src/build/configuration/quantization_types.rs | Adds QuantizationType::Spherical plus parsing/formatting and tests for the new variant. |
| diskann-disk/src/build/builder/tests.rs | Extends integration tests to build/search spherical 1-bit indexes, including metric variants. |
| diskann-disk/src/build/builder/quantizer.rs | Trains a 1-bit spherical quantizer for disk builds and stores it in the build quantizer enum. |
| diskann-disk/src/build/builder/inmem_builder.rs | Plumbs spherical quantization into the async in-memory index builder via a spherical insert/prune strategy. |
| diskann-disk/src/build/builder/core.rs | Updates build RAM estimation to account for spherical quantized vector storage and adds validation coverage. |
Suppressed comments (3)
diskann-disk/src/build/configuration/quantization_types.rs:186
QuantizationType::SQ { standard_deviation: None }currently formats asSQ_<nbits>_None, butFromStronly acceptsSQ_<nbits>for the default stddev. Because serde Serialize usesto_string()and Deserialize usesfrom_str, SQ-with-default cannot roundtrip (e.g. bincode serialize then deserialize fails). Consider emittingSQ_<nbits>whenstandard_deviationisNoneto keep Display/parse/serde consistent.
This issue also appears in the following locations of the same file:
- line 253
- line 312
QuantizationType::Spherical(nbits) => write!(f, "SPHERICAL_{}", nbits),
QuantizationType::SQ {
nbits,
standard_deviation,
} => {
diskann-disk/src/build/configuration/quantization_types.rs:257
- The
fmt_quantization_typetest currently assertsSQ_8_Nonefor the default-SQ formatting, which encodes the same Display/parse mismatch described above. If Display is changed to emitSQ_<nbits>whenstandard_deviationisNone, update this expectation accordingly so the test enforces a roundtrippable representation.
#[case(QuantizationType::Spherical(1), "SPHERICAL_1")]
#[case(
QuantizationType::SQ { nbits: 8, standard_deviation: None },
"SQ_8_None"
)]
diskann-disk/src/build/configuration/quantization_types.rs:316
test_roundtrip_serializationdocuments (and works around) the fact that SQ-with-default-stddev doesn't roundtrip. If Display/parse are made consistent forstandard_deviation: None, it would be valuable to include that case in this roundtrip test to prevent regressions.
nbits: 8,
standard_deviation: Some(Positive::new(1.5).unwrap()),
},
QuantizationType::Spherical(1),
];
💡 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 #1331 +/- ##
==========================================
+ Coverage 91.55% 91.57% +0.01%
==========================================
Files 522 522
Lines 99541 99602 +61
==========================================
+ Hits 91139 91206 +67
+ Misses 8402 8396 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Mark Hildebrand (hildebrandmw)
left a comment
There was a problem hiding this comment.
Added some comments on the use of spherical quantization and some maintenance suggestions. Please get a review from the maintainers of diskann-disk regarding how this feature fits in architecturally.
Wei Wu (wuw92)
left a comment
There was a problem hiding this comment.
Thanks for your change to support spherical in disk path. Is spherical disk-index support intentionally limited to 1-bit quantization, or is support for other bit widths planned?
87bfa99 to
b6614eb
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 1-bit limitation is intentional for this PR. The immediate goal is to provide a spherical replacement for the 1-bit scalar-quantized disk builds used in production. The underlying async spherical provider already supports 1, 2, and 4 bits, and the typed |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Aditya Krishnan (arkrishn94)
left a comment
There was a problem hiding this comment.
Thanks Junkui, it mostly looks good to me. Just had a couple of clarifying questions for my understanding.
Summary
Validation
cargo test -p diskann-disk(250 unit tests and 2 doc tests passed)cargo test -p diskann-disk test_spherical_disk_index_builder_with_metric(2 passed)cargo fmt --all --checkgit diff --check