Add some basic HNSW graph checks to CheckIndex #13984
Conversation
|
this might also check for graph connectivity, see #12627 |
There was a problem hiding this comment.
Thank you for tackling this @benchaplin! This is a blind spot for CheckIndex today, which is scary. I left some minor comments...
| private static HnswGraph getHnswGraph(CodecReader reader) throws IOException { | ||
| KnnVectorsReader vectorsReader = reader.getVectorReader(); | ||
| if (vectorsReader instanceof PerFieldKnnVectorsFormat.FieldsReader) { | ||
| vectorsReader = ((PerFieldKnnVectorsFormat.FieldsReader) vectorsReader).getFieldReader("knn"); |
There was a problem hiding this comment.
Hmm the field can be anything (not necessarily knn like luceneutil uses)? Can we change this so we iterate FieldInfos and for any field that has KNN vectors enabled (FieldInfo.hasVectorValues, hmm but also has the HNSW graph (since I think a field can be only "flat" vectors?)) we do this check?
There was a problem hiding this comment.
Thanks, I didn't quite understand fields when I wrote this - I think I get it now. Alright, I've done what you suggested (as is also done in testVectors) and iterated over FieldInfos, performing the check only when it applies.
Because we might now parse several HNSW graphs, I've restructured the status object to support per-graph data. Successful output will now look like:
test: open reader.........OK [took 0.010 sec]
test: check integrity.....OK [took 2.216 sec]
test: check live docs.....OK [took 0.000 sec]
test: field infos.........OK [2 fields] [took 0.000 sec]
test: field norms.........OK [0 fields] [took 0.000 sec]
test: terms, freq, prox... test: stored fields.......OK [1500000 total field count; avg 1.0 fields per doc] [took 0.390 sec]
test: term vectors........OK [0 total term vector count; avg 0.0 term/freq vector fields per doc] [took 0.000 sec]
test: docvalues...........OK [0 docvalues fields; 0 BINARY; 0 NUMERIC; 0 SORTED; 0 SORTED_NUMERIC; 0 SORTED_SET; 0 SKIPPING INDEX] [took 0.000 sec]
test: points..............OK [0 fields, 0 points] [took 0.000 sec]
test: vectors.............OK [1 fields, 1500000 vectors] [took 0.496 sec]
test: hnsw graphs.........OK [2 fields: (field name: knn1, levels: 4, total nodes: 1547684), (field name: knn2, levels: 4, total nodes: 1547684)] [took 0.979 sec]
testVectors doesn't do this, it just sums vectors over all fields. I could do that too, but this felt most complete.
| } | ||
| status.hnswGraphSize++; | ||
| } | ||
| status.hsnwGraphNumLevels++; |
There was a problem hiding this comment.
Instead of incrementing here, you could just set it to numLevels from above?
There was a problem hiding this comment.
I could, but wanted to respect the object as a "status" - so if a check fails, numLevels will indicate on which level it failed. However we don't print the status if an exception is thrown. What do you think?
+1 -- this is a tricky thing about these HNSW graphs (that they are not necessarily born connected but rather must be stitched up somehow afterwards). But maybe do this separately? I'd love to just get this initial coverage in first... |
|
Actually, |
| hnswGraph.seek(level, node); | ||
| int nbr, lastNeighbor = -1, firstNeighbor = -1; | ||
| while ((nbr = hnswGraph.nextNeighbor()) != NO_MORE_DOCS) { | ||
| if (firstNeighbor == -1) { |
There was a problem hiding this comment.
perhaps at some point we could also check for uniqueness of the neighbors, and also check that the neighbors are in range [0, numGraphNodes], and finally on levels > 0, we would want to assert that the neighbors are on this level. But this can all be done separately; maybe we could add a comment here?
There was a problem hiding this comment.
Sounds good, I'll add a TODO (and add those checks in a followup PR). A couple questions first:
- Doesn't (neighbors are in order) and (neighbors are not repeated) verify uniqueness?
- Also, why wouldn't we assert neighbors are on this level for level 0?
duh, yes :) yes, we should, it's just that for level 0 this is the same as node in [0, size) wheras other levels are sparse - they have the same max = size-1, but not every node is included |
|
Gotcha, thanks for the comments @msokolov! |
Description
This is a first pass on #13724 - I've added the following checks:
The code was adapted from KnnGraphTester in luceneutil.
Here's some test results from an index containing the cohere-wikipedia-docs-768d dataset:
I manually corrupted some neighbors lists in order to check the failure cases:
I intend to dig into the HNSW creation and think about more checks that would be useful here, but I wanted to start small with these neighbor checks only.