Skip to content

fix: validate embedding model dimensions - #19

Open
medlouaynjima wants to merge 3 commits into
AgentPostmortem:mainfrom
medlouaynjima:fix/embedding-dimension-validation
Open

fix: validate embedding model dimensions#19
medlouaynjima wants to merge 3 commits into
AgentPostmortem:mainfrom
medlouaynjima:fix/embedding-dimension-validation

Conversation

@medlouaynjima

Copy link
Copy Markdown

Fixes #9

What changed

  • Detect the actual embedding dimension when LocalEmbedder loads a model.
  • Expose the loaded dimension through LocalEmbedder.dims.
  • Raise a clear error when the model dimension differs from the expected vector(384) schema.
  • Add regression tests for matching and mismatched embedding dimensions.
  • Document the schema update and re-ingestion requirement when changing EMBED_MODEL.

Validation

  • pytest -q tests/test_embeddings.py
  • 3 passed
  • git diff --check

@github-actions

Copy link
Copy Markdown

Welcome, @medlouaynjima, and thanks for your first pull request to VaultRAG.

A quick look at CONTRIBUTING.md covers how things are set up and run here.

I'll review this shortly. Ask anything in the thread if something is unclear.

@royalpinto007 royalpinto007 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The dimension check only fails once because the invalid model and dimension are cached before validation. After the first ValueError, a second dims access returns 768 and embed() can reuse the incompatible model. Please validate before caching, or persist the failure, and add a regression test for repeated access. Everything else looks good.

@royalpinto007 royalpinto007 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The repeated dimension validation issue is fixed, but LocalEmbedder._load() no longer returns the validated model, so embed() fails with AttributeError even for a valid 384-dimensional model. Please restore the return value and add a regression test that calls embed().

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.

EMBED_MODEL can be changed freely, but dims stays hardcoded at 384

2 participants