Skip to content

Commit b4c66ba

Browse files
Kayvan-Zahirimeta-codesync[bot]
authored andcommitted
Fix reverse_index_factory/get_code_size round-trip for HNSW, IMI and IVFPQR (#5564)
Summary: Three defects in `contrib/factory_tools.py` where a reversed factory string rebuilds a different index or fails to parse. `test_get_code_size_hnsw_roundtrip` already pins this invariant for flat HNSW; these cases escape it. 1. **HNSW storage dropped.** The `IndexHNSW` branch never read `index.storage`, so `HNSW32,SQ8` and `HNSW32,PQ16x8` both reversed to `"HNSW32"` and rebuilt as `IndexHNSWFlat`. `get_code_size` then applied the flat `d*4` formula: at d=128 it reported 768 bytes/vector for an index whose storage is 128 plus the 256-byte graph estimate, so 384. 2. **IMI regex fields transposed.** `index_factory.cpp:295` parses `IMI2x([0-9]+)`, but the regex read `IMI\d+x2`. Every real IMI key (`IMI2x10,PQ16`, and the ones in `demos/demo_auto_tune.py`) raised `RuntimeError("cannot parse")`. 3. **IndexIVFPQR matched by the IndexIVFPQ check**, dropping the refinement stage: `IVF64,PQ8+16` reversed to `IVF64,PQ8x8`, 8 bytes instead of 24. Consumers are `benchs/bench_all_ivf/parse_bench_all_ivf.py` (imports `get_code_size` as `unitsize`) and `benchs/bench_fw/index.py`. Flat storage stays implicit as the bare `HNSW<M>` string, and a null `storage` pointer still returns `HNSW<M>` instead of raising. **Behavior change worth flagging:** `IndexHNSW2Level` storage is `Index2Layer`, which `reverse_index_factory` does not handle, so it now raises `NotImplementedError` instead of silently returning a wrong `"HNSW32"`. It is not reachable from `index_factory`. Pull Request resolved: #5564 Test Plan: Four tests added to `TestFactoryTools`. All four fail on unpatched `factory_tools.py` and pass with it: ``` # pristine contrib/factory_tools.py, new tests present 4 failed, 7 passed FAILED test_get_code_size_hnsw_non_flat_storage FAILED test_get_code_size_imi FAILED test_hnsw_storage_reverse_index_factory FAILED test_ivfpqr_reverse_index_factory # with the fix 11 passed ``` Full `tests/test_contrib.py tests/test_factory.py`, pristine vs branch: ``` pristine: 2 failed, 88 passed branch: 2 failed, 92 passed ``` Failure sets are identical. The two are `TestComputeGT::test_compute_GT_gpu` and `test_compute_GT_ip_gpu`, pre-existing and unrelated (`AttributeError: module 'faiss' has no attribute 'GpuMultipleClonerOptions'` on a CPU-only build). An A/B sweep over 40 factory strings changes exactly the 7 expected lines and nothing else: ``` - HNSW32,SQ8 IndexHNSWSQ HNSW32 768 + HNSW32,SQ8 IndexHNSWSQ HNSW32,SQ8 384 - HNSW32,SQfp16 IndexHNSWSQ HNSW32 768 + HNSW32,SQfp16 IndexHNSWSQ HNSW32,SQfp16 512 - HNSW32,PQ16x8 IndexHNSWPQ HNSW32 768 + HNSW32,PQ16x8 IndexHNSWPQ HNSW32,PQ16x8 272 - IVF64,PQ8+16 IndexIVFPQR IVF64,PQ8x8 8 + IVF64,PQ8+16 IndexIVFPQR IVF64,PQ8+16 24 - IMI2x5,Flat IndexIVFFlat IMI2x5,Flat <RuntimeError> + IMI2x5,Flat IndexIVFFlat IMI2x5,Flat 512 - IMI2x5,PQ8 IndexIVFPQ IMI2x5,PQ8x8 <RuntimeError> + IMI2x5,PQ8 IndexIVFPQ IMI2x5,PQ8x8 8 - IMI2x8,PQ8x8 IndexIVFPQ IMI2x8,PQ8x8 <RuntimeError> + IMI2x8,PQ8x8 IndexIVFPQ IMI2x8,PQ8x8 8 ``` flake8 at the 80-char limit from CONTRIBUTING reports the same 12 pre-existing findings before and after, none in the changed lines. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed By: mnorris11 Differential Revision: D118661065 Pulled By: trang-nm-nguyen fbshipit-source-id: b50a9522361e3fd14f276b36196d3c728567b980
1 parent cfbf4ec commit b4c66ba

2 files changed

Lines changed: 65 additions & 2 deletions

File tree

‎contrib/factory_tools.py‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ def get_code_size(d, indexkey):
2525
if mo:
2626
return get_code_size(d, mo.group(1))
2727

28-
mo = re.match("IMI\\d+x2,(.*)$", indexkey)
28+
mo = re.match("IMI\\d+x\\d+,(.*)$", indexkey)
2929
if mo:
3030
return get_code_size(d, mo.group(1))
3131

@@ -50,6 +50,11 @@ def get_code_size(d, indexkey):
5050
M = int(mo.group(1))
5151
return d * 4 + M * 2 * 4 # roughly
5252

53+
mo = re.match("HNSW(\\d+),(.*)$", indexkey)
54+
if mo:
55+
M = int(mo.group(1))
56+
return get_code_size(d, mo.group(2)) + M * 2 * 4 # roughly
57+
5358
if indexkey == "SQ8":
5459
return d
5560
elif indexkey == "SQ4":
@@ -127,6 +132,9 @@ def reverse_index_factory(index):
127132
return prefix + ",Flat"
128133
if isinstance(index, faiss.IndexIVFScalarQuantizer):
129134
return prefix + "," + sq_names[index.sq.qtype]
135+
# IndexIVFPQR subclasses IndexIVFPQ, so it must be checked first.
136+
if isinstance(index, faiss.IndexIVFPQR):
137+
return prefix + f",PQ{index.pq.M}+{index.refine_pq.M}"
130138
if isinstance(index, faiss.IndexIVFPQ):
131139
return prefix + f",PQ{index.pq.M}x{index.pq.nbits}"
132140
if isinstance(index, faiss.IndexIVFPQFastScan):
@@ -152,7 +160,12 @@ def reverse_index_factory(index):
152160
return f"{prefix},{reverse_index_factory(index.index)}"
153161

154162
elif isinstance(index, faiss.IndexHNSW):
155-
return f"HNSW{get_hnsw_M(index)}"
163+
prefix = f"HNSW{get_hnsw_M(index)}"
164+
storage = faiss.downcast_index(index.storage)
165+
# flat storage is implicit in the bare HNSW<M> factory string
166+
if storage is None or isinstance(storage, faiss.IndexFlat):
167+
return prefix
168+
return f"{prefix},{reverse_index_factory(storage)}"
156169

157170
elif isinstance(index, faiss.IndexRefine):
158171
return (

‎tests/test_contrib.py‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1021,6 +1021,56 @@ def test_get_code_size_hnsw_roundtrip(self):
10211021
code_size = factory_tools.get_code_size(d, factory_str)
10221022
self.assertEqual(code_size, d * 4 + 16 * 2 * 4)
10231023

1024+
def test_hnsw_storage_reverse_index_factory(self):
1025+
d = 128
1026+
# flat storage stays implicit, everything else must be spelled out
1027+
cases = {
1028+
"HNSW32,Flat": "HNSW32",
1029+
"HNSW32,SQ8": "HNSW32,SQ8",
1030+
"HNSW32,SQfp16": "HNSW32,SQfp16",
1031+
"HNSW32,PQ16x8": "HNSW32,PQ16x8",
1032+
}
1033+
for key, expected in cases.items():
1034+
index = faiss.index_factory(d, key)
1035+
factory_str = factory_tools.reverse_index_factory(index)
1036+
self.assertEqual(factory_str, expected)
1037+
# the string must rebuild the same index class
1038+
rebuilt = faiss.index_factory(d, factory_str)
1039+
self.assertEqual(type(rebuilt), type(index))
1040+
1041+
def test_get_code_size_hnsw_non_flat_storage(self):
1042+
d = 128
1043+
graph = 32 * 2 * 4
1044+
self.assertEqual(
1045+
factory_tools.get_code_size(d, "HNSW32,SQ8"), d + graph
1046+
)
1047+
self.assertEqual(
1048+
factory_tools.get_code_size(d, "HNSW32,SQfp16"), d * 2 + graph
1049+
)
1050+
self.assertEqual(
1051+
factory_tools.get_code_size(d, "HNSW32,PQ16x8"), 16 + graph
1052+
)
1053+
1054+
def test_get_code_size_imi(self):
1055+
# the coarse quantizer adds no per-vector bytes
1056+
d = 64
1057+
self.assertEqual(factory_tools.get_code_size(d, "IMI2x10,PQ16"), 16)
1058+
self.assertEqual(factory_tools.get_code_size(d, "IMI2x5,Flat"), d * 4)
1059+
# round-trip: get_code_size must parse what reverse emits
1060+
index = faiss.index_factory(d, "IMI2x5,PQ8")
1061+
factory_str = factory_tools.reverse_index_factory(index)
1062+
self.assertEqual(factory_str, "IMI2x5,PQ8x8")
1063+
self.assertEqual(factory_tools.get_code_size(d, factory_str), 8)
1064+
1065+
def test_ivfpqr_reverse_index_factory(self):
1066+
d = 128
1067+
index = faiss.index_factory(d, "IVF64,PQ8+16")
1068+
factory_str = factory_tools.reverse_index_factory(index)
1069+
self.assertEqual(factory_str, "IVF64,PQ8+16")
1070+
rebuilt = faiss.index_factory(d, factory_str)
1071+
self.assertEqual(type(rebuilt), type(index))
1072+
self.assertEqual(factory_tools.get_code_size(d, factory_str), 8 + 16)
1073+
10241074
def test_rabitq_reverse_index_factory(self):
10251075
d = 64
10261076
quantizer = faiss.IndexFlatL2(d)

0 commit comments

Comments
 (0)