Repository navigation
Python: Initialize key mapping for a custom FAISS index - #14569
A Aswanth Raj (aswanth-07) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The focused fix is consistent with existing index initialization paths and has comprehensive regression coverage.
0 open findings
What changed in this PR
Initializes FAISS key mappings for supplied single indexes, preventing first-upsert failures.
Changes:
- Adds missing key-map initialization while preserving existing mappings.
- Adds regression coverage for flat and HNSW custom indexes across both setup paths.
| File | Description |
|---|---|
python/semantic_kernel/connectors/faiss.py |
Initializes the vector field’s key map. |
python/tests/unit/connectors/memory/test_faiss.py |
Tests insertion, search, repeated setup, and recreation. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
PRABHU KIRAN VANDRANKI (VANDRANKI)
left a comment
There was a problem hiding this comment.
Community review (not a maintainer, does not clear the merge gate). I read the full diff and ran it on this branch (022cfc4) and on upstream main (1b1f64b), python/ with faiss-cpu installed in the venv.
The bug is real on main and the one-line fix works. I put the PR's new test test_custom_index_upsert_and_search on top of main's faiss.py: 3 of the 6 parameter sets fail, all of them the single-index ones (flat-l2, flat-ip, hnsw), with KeyError: 'vector' inside upsert, surfaced as VectorStoreOperationException: Error upserting record(s) into collection 'test': 'vector'. The index-dict variants pass on main because that branch of _create_indexes already calls indexes_key_map.setdefault(...). On this branch tests/unit/connectors/memory/test_faiss.py gives 21 passed.
setdefault in the early-return branch matches what the dict branch does, so I have no objection to the change. The test also checks that a second ensure_collection_exists keeps the same key map object, which is the right thing to pin down.
One limitation of the approach, which is not caused by this PR but is now reachable: the key map starts empty, so a custom index that already holds vectors is not supported. I tried it on this branch: faiss.IndexFlatL2(5) with 3 vectors added beforehand, ensure_collection_exists(index=idx), then upsert({"id": "first", ...}). The key map after the upsert is {'first': 0} while the index has ntotal 4 and the new vector sits at position 3. A search then fails with VectorSearchExecutionException ... list index out of range. A trained, empty index (the case the is_trained check is written for) works. If you do not want to fix that here, a line in the docstring of ensure_collection_exists saying the index must be empty would help.
I did not run the other connector tests.
|
PRABHU KIRAN VANDRANKI (@VANDRANKI) Clarified in 3879d30: supplied indexes for a new collection must contain no vectors. Records must be inserted through the collection so native index rows and key mappings stay aligned. I reproduced the pre-populated-index mismatch through both custom-index entry points and kept the follow-up documentation-only. The FAISS and inherited in-memory modules pass all 64 tests together; runtime behavior is unchanged by this follow-up. |
Motivation and Context
I found that
FaissCollection.ensure_collection_exists(index=...)accepts a trained custom index, but the firstupsert()raisesVectorStoreOperationExceptioncaused byKeyError('vector'). This prevents record insertion through the documented single-index setup path, including custom HNSW indexes.The single-index branch returns before initializing the vector field's key map. Both the automatic and
indexes={...}branches already initialize that map. The existing custom-index test covers collection creation without inserting a record.Description
I added the missing
setdefaultbefore the early return. It initializes a new mapping and preserves an existing mapping when collection setup runs again. The supplied index object, training validation, native search behavior, default setup and dictionary setup remain unchanged. The change adds no file operations or persisted-format changes.The new test uses real flat L2, flat inner-product and HNSW indexes across both custom-index entry points. It checks insertion, retrieval, ordered search, repeated setup after insertion, and collection deletion/recreation.
The
ensure_collection_existsdocstring also clarifies that supplied indexes for a new collection must contain no vectors. Insert records through the collection so native rows and key mappings stay aligned. Pre-populated external indexes remain unsupported; this documentation change adds no rejection or runtime behavior.After the documentation update in
3879d30662a7, the FAISS and in-memory modules pass all 64 tests together. Applicable changed-file formatting/syntax hooks and whitespace checks pass. Current-head hosted CI still awaits maintainer approval.Validation on Windows, Python 3.11.9 and faiss-cpu 1.13.2:
main, the three single-index regression cases fail with the missing-map error; the three dictionary-route controls pass.pytest tests/unit/connectors/memory/test_faiss.py: 21 passed.pytest tests/unit/connectors/memory/test_in_memory.py: 43 passed on base and patch.git diff --checkpassed.I did not run the complete Python suite, GPU FAISS or non-Python builds locally. The FAISS unit module emits existing SWIG deprecation warnings.
Contribution Checklist
The local validation above covers the affected connector and its inherited in-memory behavior. The full-build and complete-suite assertions remain unchecked.
AI assistance disclosure: AI-assisted development tools were used during investigation and implementation. The reported tests were run against the final diff.