fix: add retriever upsert semantics - #86
UdayKumar9381 wants to merge 1 commit into
Conversation
adaumsilva
left a comment
There was a problem hiding this comment.
Thanks @UdayKumar9381 for implementing upsert semantics! I found issues that need fixing before merging:
-
Duplicate IDs within one batch break the contract. Both retrievers retain duplicate logical chunks when the initial batch contains the same ID twice. InMemoryRetriever also duplicates new IDs repeated within later batches. Please validate the complete batch, then consolidate duplicate IDs with a documented policy such as “last occurrence wins.”
-
FAISS can return stale content after replacement. I reproduced
add([old, new])with the same ID, followed byadd([latest]). Retrieval still returnsnew, notlatest: the replacement updates the first logical duplicate, while retrieval maps the active label to the last duplicate. Add regression coverage for this sequence, checking length, content, embedding, and uniqueness. -
FAISS retrieval requests every physical vector.
search(..., self._index.ntotal)runs even fortop_k=1on an index without tombstones, creating a substantial scaling regression. Please use bounded over-fetching and expand only when filtering leaves too few active results. Also replace the per-chunk linear scan of_chunksduring upsert with an ID-to-logical-position mapping.
Please remove the unreachable rebuild code after the unconditional return in FAISSRetriever.add(), add the requested Chroma replacement regression and Unreleased changelog entry, and resolve conflicts with main while preserving #85’s lazy matrix consolidation.
All four CI checks and 64 targeted tests pass, but the current tests miss these cases. Requesting changes.
|
Thanks for the detailed review, @adaumsilva. I appreciate you reproducing these edge cases. I’ll address all of the requested changes:
After making the changes, I’ll run the targeted tests plus the full test suite and lint checks again before pushing the update. Thanks again for the thorough review — this helped identify cases that the initial regression tests were missing. |
adaumsilva
left a comment
There was a problem hiding this comment.
Thanks for confirming the planned fixes. I checked again, and the PR still points to caf6661, so the previously requested changes remain outstanding.
Duplicate IDs within one batch still produce duplicate results, and the FAISS replacement and retrieval issues remain unchanged. The branch also has conflicts with main.
Please push the fixes and regression tests described in your response, then request another review. My request-changes recommendation remains in place.
Summary
Closes #28.
Validation
pytest tests\test_retriever\test_in_memory.py -q— 35 passedpytest tests\test_retriever\test_faiss.py -q— 29 passedpytest -q— 292 passedruff check ragframework\ tests\— passedgit diff --check— passed