Fix invalid L2 distances from IndexHNSWFlat with stale cached_l2norms#5326
Fix invalid L2 distances from IndexHNSWFlat with stale cached_l2norms#5326notandruu wants to merge 1 commit into
Conversation
IndexFlatL2::get_FlatCodesDistanceComputer() used the cached-norms distance computer whenever cached_l2norms was non-empty. That computer indexes cached_l2norms[i] for every vector up to ntotal, but add() after a sync_l2norms() does not extend (or invalidate) cached_l2norms, so its size can be smaller than ntotal. The computer then read stale or out-of-range norms and returned invalid squared L2 distances, including negative values, with inconsistent top-k results. Only take the cached path when cached_l2norms covers every vector (size == ntotal); otherwise fall back to the on-the-fly L2 computer. The fast path is unchanged for the normal case (add all vectors, then call sync_l2norms once). Fixes facebookresearch#5320
|
Hi @notandruu! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
| // Only use the cached-norms distance computer when the cache covers | ||
| // every vector. add() after a sync_l2norms() leaves cached_l2norms | ||
| // shorter than ntotal; using it then would read stale/out-of-range | ||
| // norms and yield invalid (e.g. negative) L2 distances. |
There was a problem hiding this comment.
Adding the check seems correct, but we can trim the AI comment a bit:
// Only use the cached-L2norm path when the cache covers every vector.
// add() does not maintain it.
| # Regression test for #5320: calling sync_l2norms() after a partial | ||
| # add() and then adding more vectors left cached_l2norms shorter than | ||
| # ntotal. The cached-norms distance computer then read stale/out-of- | ||
| # range norms, returning invalid (e.g. negative) squared L2 distances | ||
| # and inconsistent top-k results. |
| for row in range(nq): | ||
| self.assertEqual(I[row, 0], Iref[row, 0]) | ||
| np.testing.assert_almost_equal(D[row, 0], Dref[row, 0], decimal=4) |
There was a problem hiding this comment.
I think this tests a slightly different issue.
Right now it is saying, "did HNSW find the true nearest neighbor?". But HNSW is ANN, so it will not always find it.
Instead, we should make sure the returned distances are valid.
for row in range(nq):
for j in range(k):
true_d = ((xb[I[row, j]] - xq[row]) ** 2).sum()
np.testing.assert_almost_equal(D[row, j], true_d, decimal=4)
This tests, "for the returned results from HNSW, recompute the distance, and make sure we got the right value"
2 participants
Fixes #5320
Root cause
IndexFlatL2::get_FlatCodesDistanceComputer()selects the cached-norms distance computer (FlatL2WithNormsDis) whenevercached_l2normsis non-empty. That computer readscached_l2norms[i]for every vector up tontotal. Howeveradd()after async_l2norms()does not extend or invalidatecached_l2norms, so its size can be smaller thanntotal. The computer then reads stale / out-of-range norms and returns invalid squared L2 distances (including negative values), with inconsistent and wrong top-k results. This is reachable fromIndexHNSWFlat(METRIC_L2), whose storage is anIndexFlatL2.Fix
Take the cached path only when
cached_l2norms.size() == ntotal, i.e. when the cache covers every vector; otherwise fall back to the on-the-fly L2 computer. The fast path is unchanged for the normal usage (add all vectors, thensync_l2norms()once). One line, plus a comment.Reproduction and validation
Repro: add a partial batch to an
IndexHNSWFlat,sync_l2norms()on the downcastIndexFlatL2, add the rest, then search. Validated with a from-source build (BLAS=Accelerate, libomp) using a C++ negative control:Regression test
TestSyncL2Norms.test_indexflat_l2_sync_norms_stale_after_addadded next to the existingsync_l2normstest. It reproduces the scenario and asserts no negative distances and that each query's nearest neighbour matches an exactIndexFlatL2oracle. Run against the released wheel (which has the bug) the test fails (min distance -11114); with this fix the underlying cause is removed.Note: I could not build the Python bindings locally (no swig), so the C++ fix was validated via a from-source C++ build and the Python regression test was validated to fail on the unpatched build; CI will exercise it against the patched bindings.