Skip to content

fix(retriever): avoid re-normalizing full in-memory matrix - #85

Open
UdayKumar9381 wants to merge 5 commits into
adaumsilva:mainfrom
UdayKumar9381:fix/in-memory-retriever-normalization
Open

UdayKumar9381 wants to merge 5 commits into
adaumsilva:mainfrom
UdayKumar9381:fix/in-memory-retriever-normalization

Conversation

@UdayKumar9381

Copy link
Copy Markdown
Contributor

Description

Closes #27.

This PR optimizes InMemoryRetriever.add() so that incremental ingestion does not rebuild and re-normalize the entire stored vector matrix on every call.

Changes

  • Normalize only newly added vectors.
  • Store newly normalized vectors as matrix blocks.
  • Concatenate matrix blocks lazily on the first retrieval after new data is added.
  • Preserve existing retrieval behavior and result ordering.
  • Add a regression test comparing single-batch and multi-batch ingestion.
  • Update CHANGELOG.md under [Unreleased] -> ### Fixed.

Validation

  • pytest tests/test_retriever/test_in_memory.py -q — 33 passed
  • pytest -q — 266 passed
  • ruff check ragframework/ tests/ — passed
  • pre-commit run --files CHANGELOG.md ragframework/retriever/in_memory.py tests/test_retriever/test_in_memory.py — passed
  • git diff --check — passed

Type of change

  • Performance improvement
  • Bug fix
  • Tests added/updated
  • Documentation/changelog updated

@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.

@UdayKumar9381 Thanks for the optimization! Tests and CI pass, and retrieval ordering remains correct.

One issue needs fixing before merging: after concatenation in retrieve(), _matrix_blocks still retains every original array alongside the new full matrix. This doubles persistent NumPy vector storage.

Please consolidate the blocks so they reference the cached matrix after concatenation, and reuse the existing array when there is only one block. Add regression coverage confirming that consolidation avoids duplicate storage and that subsequent add/retrieve cycles preserve all results.

Please request another review once these changes are pushed and CI passes.

@UdayKumar9381

Copy link
Copy Markdown
Contributor Author

Thanks for the review.

I’ve addressed the requested changes:

Consolidated _matrix_blocks after retrieval to avoid duplicate NumPy storage.
Reused the existing array when only one matrix block is present.
Added regression coverage for consolidation and subsequent add/retrieve cycles.
Verified the full test suite and Ruff checks pass.

The changes have been pushed to the PR. Please take another look when convenient.

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.

InMemoryRetriever.add() re-normalises the entire matrix on every call (O(N²))

2 participants