fix(index): enforce the precomputed-input contracts in the current IVF build path - #9376
Open
LuciferYang wants to merge 3 commits into
Open
LuciferYang wants to merge 3 commits into
LuciferYang wants to merge 3 commits into
Conversation
The field docs promise that precomputed_shuffle_buffers is mutually exclusive with precomputed_partitions_file and requires centroids, but neither was validated. Combining both silently used two different assignment sources, and building without centroids trained fresh ones while the buffers were produced against different centroids — silently wrong search results. Enforce both contracts at the start of build(). Assisted-by: GLM-5.3
…ted-buffers-contract
Contributor
There was a problem hiding this comment.
This correctly enforces the centroid and mutual-exclusion contracts at the current V3 builder boundary while leaving the frozen legacy writer unchanged. The remaining risk is the existing V3 gap called out in the PR: precomputed PQ buffers can still be accepted without the originating PQ codebook, allowing raw callers to pair stored codes with a newly trained model and receive incorrect distances. The quantizer abstraction does not currently expose enough model provenance to close that gap in this focused patch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #9375
IvfBuildParamsdocuments thatprecomputed_shuffle_buffersandprecomputed_partitions_fileare mutually exclusive and that the buffers requirecentroids.sanity_check_ivf_paramsenforces that, but only the legacy IVF_PQ and IVF_HNSW_PQ writers reach it, soIvfIndexBuilderaccepted all three broken combinations. Buffers without centroids produced an index whose stored centroids came from a fresh sample while its rows kept the__ivf_part_idthe buffers were written with, so queries probed partitions chosen by centroids that say nothing about where the rows went. Both inputs at once panicked in a worker task on a duplicate__ivf_part_idcolumn.The contract now lives as
IvfBuildParams::validate(), next to the field comments that state it, andIvfIndexBuilder::newcalls it, so the build fails at construction with aninvalid_inputerror naming which pairing is wrong. The legacy writer keeps its own copy of the checks: it is a frozen write path, and routing it through the new method would change the error variant it has always returned.Two combinations that used to be accepted are now rejected, and both were already broken.
accelerator=...together withprecomputed_partition_dataset=...sets both fields from Python and panicked insidetokio::spawn; that is now a clean error. Passingprecomputed_partitions_fileorprecomputed_shuffle_buffersas a raw**kwargsentry withoutivf_centroidsbypasses the guard indataset.pyand silently built a mismatched index. Every first-party flow supplies centroids alongside a precomputed input, since the accelerator path overwritesivf_centroidswith what it just trained, theprecomputed_partition_datasetpath raises without them, and the distributed path requires precomputed centroids. So nothing that worked stops working.One rule from the legacy check is deliberately not carried over: the buffers also require a PQ codebook.
QuantizationandQuantizerBuildParamsexpose no way to ask whether a model was supplied, so that check needs a trait change or per-quantizer handling at the dispatch sites rather than a line inIvfBuildParams.How was this patch tested?
test_validate_rejects_precomputed_inputscovers the three rejected shapes and asserts the error variant and the message,test_validate_accepts_supported_combinationspins the shapes that must stay accepted, andtest_new_rejects_precomputed_buffers_without_centroidspins that the V3 builder calls the check itself rather than leaving it to a caller. Dropping the call fromnewfails that last test, and removing either rejection arm fails its case.