Skip to content

fix(rag): drop unnecessary sentence-transformers hard dependency#1979

Merged
kovtcharov-amd merged 3 commits into
mainfrom
fix/rag-drop-sentence-transformers-dep
Jul 9, 2026
Merged

fix(rag): drop unnecessary sentence-transformers hard dependency#1979
kovtcharov-amd merged 3 commits into
mainfrom
fix/rag-drop-sentence-transformers-dep

Conversation

@kovtcharov-amd

Copy link
Copy Markdown
Collaborator

Why this matters

RAG embeds through Lemonade, yet the RAG SDK hard-required sentence-transformers to import — a heavy, fragile dependency (torch/torchcodec) it never actually uses. On any machine where sentence-transformers failed to import (e.g. broken torchcodec/FFmpeg native libs), every RAG document-indexing call silently produced 0 chunks even though the Lemonade EmbeddingGemma embedder was loaded and ready — a hard-to-diagnose "RAG returns nothing" failure. After this change, RAG initializes and indexes normally regardless of sentence-transformers health; the library is now only what it should be — an optional dependency of the memory cross-encoder reranker (which already degrades gracefully).

This also closes the test gap that let it through: the RAG dependency-guard test previously encoded the wrong behavior (it required sentence-transformers), and the Test RAG CI workflow didn't run tests/unit/rag/ at all — so nothing exercised the guard.

Test plan

  • pytest tests/test_rag.py tests/unit/rag/ — green (115 tests; previously 28 errors from obsolete ST mocks)
  • New regression guard: RAG initializes with sentence-transformers forcibly unavailable (tests/unit/rag/test_dependency_guard.py)
  • End-to-end: index a document + retrieve with ST blocked → EmbeddingGemma 768-dim embeddings, correct answer
  • black --check / flake8 clean on changed files
  • CI Test RAG now runs tests/unit/rag/ (path filter + test command updated)

Notes

  • Bundles a small pre-existing fix: test_pptx_extraction.py::test_table_extraction was env-dependent (failed when LibreOffice was installed, since the PPTX→PDF path flattens tables); it now forces the pure-python markdown path so the assertion is deterministic.

RAG embeds via Lemonade, but RAGSDK._check_dependencies() required
sentence-transformers to import — a heavy, fragile dep (torch/torchcodec) it
never uses. On any box where sentence-transformers failed to import (e.g.
broken torchcodec/FFmpeg native libs), every RAG indexing call produced 0
chunks even though the Lemonade EmbeddingGemma embedder was loaded and ready.

Remove the import and the requirement, stop eagerly importing it at UI-server
boot, and drop it from the "rag" extra (kept in "ui" for the optional memory
cross-encoder reranker, which already degrades gracefully).

Closes the test gap that let this through: the dependency-guard test encoded
the wrong behavior (required ST), and the Test RAG workflow never ran
tests/unit/rag/. Adds a regression guard (RAG must init with ST absent/broken)
and wires tests/unit/rag/ into CI. Also fixes a pre-existing env-dependent
pptx table test (forces the pure-python markdown path deterministically).
@github-actions github-actions Bot added dependencies Dependency updates devops DevOps/infrastructure changes rag RAG system changes tests Test changes performance Performance-critical changes labels Jul 9, 2026
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Verdict: Approve with suggestions — safe to merge; the two items below are optional cleanups, not blockers.

This PR removes sentence-transformers as a hard RAG dependency. RAG actually embeds through Lemonade, so importing the heavy torch/torchcodec stack at RAG import time was dead weight — and when that native stack was broken it took down all RAG indexing (0 chunks) even though the embedder was healthy. The fix is correct and the reasoning holds up: RAG embeds via the Lemonade client, and the memory cross-encoder reranker (the library's only real user) already degrades gracefully when it's absent, so dropping it from the rag extra doesn't silently break anything. It also closes the test gap that let this through — the dependency-guard test now encodes the right behavior and CI's Test RAG job actually runs tests/unit/rag/.

Nothing blocking. Two small leftovers from the removal are worth tidying while you're here.

🔍 Technical details

Verified claims

  • RAG embedding path is Lemonade, not sentence-transformers: src/gaia/rag/sdk.py:581,613 call self.embedder.embeddings(...); sdk.py:519 sets self.embedder = self.llm_client. ✅
  • Reranker degrades gracefully: src/gaia/agents/base/memory.py:263-290 (_get_cross_encoder) returns None on ImportError/load failure and caches the miss. Removing ST from [rag] is safe; it's retained in [ui]. ✅
  • Test dict key mock_dependencies["lemonade"] exists in every yielded fixture (tests/test_rag.py:80,212,938,1258). ✅

🟢 Minor — stale hint in the broken-dep error message (src/gaia/rag/sdk.py:251)
The only remaining broken-load candidate is faiss (arch mismatch / illegal instruction). The torchcodec/FFmpeg example only ever applied to the now-removed sentence-transformers, so it now points users at the wrong cause.

                    "reinstalling won't help until the underlying error is fixed "
                    "(e.g. an arch-mismatched faiss build):\n"

🟢 Minor — single-element loop reads as leftover scaffolding (src/gaia/rag/sdk.py:234-235)
for pkg, label in (("faiss", "faiss"),): with an inner if pkg == "faiss" is an awkward remnant of the old two-item loop. Since only faiss remains, it can flatten to a plain block — clearer intent, same behavior. Optional.

Strengths

  • Fix is backed by a real regression test that asserts the absence of the ST import (test_rag_does_not_import_sentence_transformers) plus a guard that the dependency check never fails on a broken ST — exactly the failure mode being fixed.
  • Doc/comment updates land consistently across setup.py, sdk.py, and server.py, all naming why ST isn't a RAG dep.
  • The path-filter + test-command change to test_rag.yml closes the "guard existed but nothing ran it" gap in the same PR.
  • Bundled pptx test fix correctly forces the pure-python markdown path so the table assertion is deterministic regardless of LibreOffice presence.

Ovtcharov added 2 commits July 9, 2026 13:02
Address PR review: flatten the now single-element broken-dependency loop to a
plain faiss block, and correct the broken-load hint — the torchcodec/FFmpeg
example only ever applied to the removed sentence-transformers, so it now points
users at the wrong cause; faiss (arch mismatch) is the only remaining candidate.
@kovtcharov-amd

Copy link
Copy Markdown
Collaborator Author

Addressed both suggestions in 8cf2752:

  • Corrected the broken-load hint — it now names an arch-mismatched faiss build instead of the stale torchcodec/FFmpeg example (which only applied to the removed sentence-transformers).
  • Flattened the single-element for pkg, label in (("faiss", "faiss"),) loop to a plain if faiss is None: block.

Also merged latest main (no conflicts). Dependency-guard tests pass (5), black/flake8 clean.

@kovtcharov-amd
kovtcharov-amd enabled auto-merge July 9, 2026 20:30
@kovtcharov-amd
kovtcharov-amd added this pull request to the merge queue Jul 9, 2026
Merged via the queue into main with commit 638487a Jul 9, 2026
56 checks passed
@kovtcharov-amd
kovtcharov-amd deleted the fix/rag-drop-sentence-transformers-dep branch July 9, 2026 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Dependency updates devops DevOps/infrastructure changes performance Performance-critical changes rag RAG system changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants