Skip to content

fix: add retriever upsert semantics - #86

Open
UdayKumar9381 wants to merge 1 commit into
adaumsilva:mainfrom
UdayKumar9381:fix/retriever-upsert
Open

UdayKumar9381 wants to merge 1 commit into
adaumsilva:mainfrom
UdayKumar9381:fix/retriever-upsert

Conversation

@UdayKumar9381

Copy link
Copy Markdown
Contributor

Summary

Closes #28.

  • Define upsert semantics in the base Retriever contract.
  • Make InMemoryRetriever replace existing chunks by ID without increasing len().
  • Make FAISSRetriever replace existing logical chunks while keeping superseded physical vectors out of active retrieval.
  • Preserve existing HNSW cosine-similarity behavior and validation/error handling.
  • Add regression tests for duplicate ingestion and replacement behavior.
  • Chroma already uses native upsert semantics.

Validation

  • pytest tests\test_retriever\test_in_memory.py -q — 35 passed
  • pytest tests\test_retriever\test_faiss.py -q — 29 passed
  • pytest -q — 292 passed
  • ruff check ragframework\ tests\ — passed
  • git diff --check — passed

@adaumsilva adaumsilva left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @UdayKumar9381 for implementing upsert semantics! I found issues that need fixing before merging:

  1. 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.”

  2. FAISS can return stale content after replacement. I reproduced add([old, new]) with the same ID, followed by add([latest]). Retrieval still returns new, not latest: 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.

  3. FAISS retrieval requests every physical vector. search(..., self._index.ntotal) runs even for top_k=1 on 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 _chunks during 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.

@UdayKumar9381

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review, @adaumsilva. I appreciate you reproducing these edge cases.

I’ll address all of the requested changes:

  • Consolidate duplicate IDs within each batch using a documented last occurrence wins policy.
  • Fix the FAISS replacement flow so repeated upserts always retrieve the latest content, embedding, and logical chunk uniquely.
  • Add regression coverage for the add([old, new]) → add([latest]) scenario, including length, content, embedding, and uniqueness checks.
  • Change FAISS retrieval to use bounded over-fetching instead of requesting self._index.ntotal, expanding only when necessary after filtering.
  • Replace the linear _chunks lookup during upsert with an ID-to-logical-position mapping.
  • Remove the unreachable rebuild code after the unconditional return in FAISSRetriever.add().
  • Add the requested Chroma replacement regression test and Unreleased changelog entry.
  • Resolve conflicts with main while preserving the lazy matrix consolidation from fix(retriever): avoid re-normalizing full in-memory matrix #85.

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 adaumsilva left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-ingesting the same source duplicates chunks in InMemoryRetriever and FAISSRetriever

2 participants