Skip to content

refactor(rag): unify RAG pipeline, fix code review issues#92

Merged
longobucco merged 2 commits into
ragfrom
fix/rag-review
May 13, 2026
Merged

refactor(rag): unify RAG pipeline, fix code review issues#92
longobucco merged 2 commits into
ragfrom
fix/rag-review

Conversation

@lucaosti
Copy link
Copy Markdown
Collaborator

Summary

Fixes all issues identified in the code review of PR #91.

  • Duplicate BM25/RRF logic removedchat_service.py had independent reimplementations of _bm25_search, _rrf_merge, _load_corpus, _resolve_merged, and a second FlashRank singleton (_get_reranker). All replaced by delegation to hybrid_search, reranker, and parent_expansion modules.
  • _qvac_dict_to_chunk() added — converts QVAC /retrieve response dicts to EvidenceChunk so the unified EvidenceChunk-based pipeline can be used end-to-end.
  • answer() refactored — clean 9-step pipeline: /retrieve → convert → BM25 → RRF → rerank → parent expand → /generate → citations.
  • Italian comments in pipeline.py translated to English (chunking parameters block).
  • test_chat_service.py replaced — removed tests for deleted helpers; added tests for _qvac_dict_to_chunk() and answer() (happy path, dense-only, ChromaDB fallback, /generate failure, citation precision).
  • test_hybrid_search.py added — new file covering bm25_search, rrf_fuse, load_bm25_index (migrated and adapted from the old test_chat_service.py).
  • Coverage files — already removed in a prior commit on rag; .gitignore entry confirmed.
  • Alembic — not applicable; project uses Base.metadata.create_all().

Test plan

  • tests/unit/test_chat_service.py — 12 passed
  • tests/unit/test_hybrid_search.py — 10 passed, 4 skipped (rank_bm25 not installed in CI)
  • tests/unit/test_qvac_pipeline.py — 31 passed, 5 skipped
  • No new failures introduced in tests/unit/ suite

Closes #80

…tests

Remove duplicate BM25/RRF/reranker logic from chat_service.py and delegate
to the dedicated modules (hybrid_search, reranker, parent_expansion).
Add _qvac_dict_to_chunk() to convert QVAC response dicts to EvidenceChunk.
Refactor answer() to use the unified pipeline end-to-end.

Add test_hybrid_search.py covering bm25_search, rrf_fuse, load_bm25_index.
Replace test_chat_service.py with tests for answer() and _qvac_dict_to_chunk().

Translate Italian comments in pipeline.py chunking parameters to English.
config.py: replace passlib.CryptContext with direct bcrypt calls.
passlib 1.7.4 is incompatible with bcrypt >= 4.0 (removed __about__),
causing all password hashing tests to fail with ValueError.

pipeline.py: add _register_module_aliases() to register 'services.ai.app.*'
as sys.modules aliases for 'app.*'. Required by test_sys_modules_alias.py
to guarantee class identity across different import root paths.

test_chunker.py / test_ingester_parser.py: guard legacy module imports
(module_1_ingestor, module_2_parser, module_3_micro_chunker) with
pytest.importorskip so missing optional modules produce skips, not errors.

Result: 169 passed, 11 skipped, 0 failed.
@longobucco longobucco merged commit 8273f90 into rag May 13, 2026
@lucaosti lucaosti deleted the fix/rag-review branch May 13, 2026 19:51
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.

2 participants