feat: add retrieval-augmented generation over Quran and Hadith sources - #38
feat: add retrieval-augmented generation over Quran and Hadith sources#38GEEKYFOCUS wants to merge 3 commits into
Conversation
Implements RAG (Retrieval-Augmented Generation) for the /chat endpoint.
A ChromaDB persistent vector store indexes verse-level Quran chunks and
per-hadith chunks. Retrieved passages are injected into the prompt with
metadata so the model can cite real sources.
Key changes:
- New rag.py module: ChromaStore (ChromaDB embedded/persistent),
embedding via Gemini text-embedding-004, configurable top-k/min-score,
reference-passage formatting
- ChatResponse.sources: optional list of {text, reference, score}
containing the passages actually retrieved
- /chat endpoint retrieves passages before generation; skips gracefully
when index is missing or RAG_ENABLED=0
- scripts/ingest_corpus.py: downloads Saheeh International Quran
(Tanzil.net) and Sahih al-Bukhari (permissively licensed GitHub
export), chunks, embeds, and stores in ChromaDB; caches raw JSONL
under data/
- 11 offline tests: chunk metadata, retrieval with fake embeddings,
score threshold, empty corpus, reference formatting
- CI updated: lint/compile rag.py, run RAG tests
- RAG_ENABLED, RAG_TOP_K, RAG_MIN_SCORE, CHROMA_PERSIST_DIR env vars
Closes Deen-Bridge#24
WalkthroughAdds configurable Quran and Hadith retrieval with Gemini embeddings and persistent ChromaDB storage, an offline ingestion script, deterministic tests, CI coverage, and documentation. The ChangesRetrieval-augmented generation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/ci.ymlTraceback (most recent call last): Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
tests/test_rag.py (2)
137-151: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that exact-match results have positive similarity.
With
min_score=0.0, this test only checks that some Quran references are returned. A zero-score or otherwise incorrect result could still satisfy it. Assert positive scores and the expected top result/count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_rag.py` around lines 137 - 151, Strengthen test_retrieve_exact_match by asserting the exact expected result count and verifying that the top result is the expected Quran document, then assert every returned result has a strictly positive similarity score. Keep the existing embedding setup and search parameters unchanged.
159-180: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a mixed-score case for threshold coverage.
The query is orthogonal to every stored embedding, so an implementation that always returns
[]passes. Query with one matching vector and one below-threshold vector, then assert that only the matching document remains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_rag.py` around lines 159 - 180, Update test_retrieve_min_score_filters to use a query embedding that matches one stored document while leaving another below the 0.5 threshold, then assert the results contain only the matching document. Keep the test setup focused on threshold filtering rather than an all-zero-result orthogonal query.rag.py (1)
53-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRecommended: use asymmetric
task_typefor Gemini embeddings.
embed_textis reused for both document ingestion (embed_batch→add_documents) and query embedding (search), butgenai.embed_contentis called withouttask_type. Gemini'stext-embedding-004produces meaningfully better retrieval quality when documents are embedded withRETRIEVAL_DOCUMENTand queries withRETRIEVAL_QUERY— omitting it silently degrades relevance ranking without any visible error.♻️ Suggested refactor — pass task_type through
-def embed_text(text: str) -> list[float]: +def embed_text(text: str, task_type: str = "RETRIEVAL_QUERY") -> list[float]: if _FAKE_EMBEDDING is not None: return _FAKE_EMBEDDING import google.generativeai as genai result = genai.embed_content( model="models/text-embedding-004", content=text, + task_type=task_type, ) return list(result["embedding"]) -def embed_batch(texts: list[str]) -> list[list[float]]: - return [embed_text(t) for t in texts] +def embed_batch(texts: list[str]) -> list[list[float]]: + return [embed_text(t, task_type="RETRIEVAL_DOCUMENT") for t in texts]Since my knowledge of the exact
google-generativeai0.8.3 SDK surface may be stale, please confirm thetask_typekwarg name/values against the version inrequirements.txt.Also applies to: 141-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rag.py` around lines 53 - 66, Update embed_text to accept a task_type parameter and pass it to genai.embed_content using the SDK-supported keyword and retrieval task values. Update document-ingestion callers such as embed_batch/add_documents to use RETRIEVAL_DOCUMENT and query callers in search to use RETRIEVAL_QUERY, while preserving the fake-embedding path and confirming compatibility with the pinned SDK version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 31: Update the flake8 command in the CI workflow to include
tests/test_rag.py alongside the existing Python targets, preserving all current
options and exclusions.
In @.gitignore:
- Around line 40-42: Replace the broad data/ entry in the RAG / ChromaDB ignore
section with a narrowly scoped pattern for generated root-level data/*.jsonl
files, and retain the chroma_data/ ignore rule.
In `@main.py`:
- Around line 162-178: Update the RAG retrieval call in the async chat handler
to offload the synchronous retrieve function to a worker thread, awaiting its
result so the event loop remains responsive. Preserve the existing RAG_ENABLED
branching, source formatting, and logging behavior.
In `@rag.py`:
- Around line 243-248: Make retrieve asynchronous and offload the blocking
get_store().search(query) operation to asyncio.to_thread, while preserving the
disabled-RAG early return and existing logging. Update the async /chat caller to
await retrieve so synchronous embedding, network, and disk work does not block
the event loop.
- Around line 53-66: Update embed_text() and the ChromaStore.search() flow to
catch embedding and collection.query failures, log them appropriately, and
return empty results instead of propagating exceptions. Ensure retrieve()
continues gracefully when either operation fails so RAG errors cannot cause
/chat to return an HTTP 500 or expose raw exception details.
- Around line 105-126: Update the collection initialization in __init__ to
create CHROMA_COLLECTION with cosine distance metadata, matching search()’s 1 -
distance score conversion. Detect any existing persisted collection using the
incompatible distance space, migrate or recreate it with cosine space while
preserving its documents and embeddings, then retain the existing collection
setup and failure behavior.
In `@README.md`:
- Around line 76-91: The README ingestion instructions do not enable retrieval
after building the ChromaDB index. Update the setup example around
`scripts/ingest_corpus.py` to explicitly set `RAG_ENABLED=1` before starting the
API, or add a clear enablement step, while preserving the existing ingestion and
`--reuse-cache` commands.
In `@scripts/ingest_corpus.py`:
- Around line 169-179: Update the id construction in ingest so Quran entries
include both their surah and ayah identifiers, producing a unique ID for every
verse; retain hadith_number for hadith entries and the index fallback for other
records. Ensure the resulting ids passed to store.add_documents are unique
within the batch.
In `@tests/test_rag.py`:
- Around line 232-236: The test_rag_config_defaults function should isolate
environment variables and module import so it validates the documented exact
default values for RAG_TOP_K, RAG_MIN_SCORE, and RAG_ENABLED rather than broad
ranges. Extend the test to exercise retrieve() with RAG_ENABLED disabled and
assert it returns an empty list, matching the disabled gate in rag.py.
---
Nitpick comments:
In `@rag.py`:
- Around line 53-66: Update embed_text to accept a task_type parameter and pass
it to genai.embed_content using the SDK-supported keyword and retrieval task
values. Update document-ingestion callers such as embed_batch/add_documents to
use RETRIEVAL_DOCUMENT and query callers in search to use RETRIEVAL_QUERY, while
preserving the fake-embedding path and confirming compatibility with the pinned
SDK version.
In `@tests/test_rag.py`:
- Around line 137-151: Strengthen test_retrieve_exact_match by asserting the
exact expected result count and verifying that the top result is the expected
Quran document, then assert every returned result has a strictly positive
similarity score. Keep the existing embedding setup and search parameters
unchanged.
- Around line 159-180: Update test_retrieve_min_score_filters to use a query
embedding that matches one stored document while leaving another below the 0.5
threshold, then assert the results contain only the matching document. Keep the
test setup focused on threshold filtering rather than an all-zero-result
orthogonal query.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f8a02f21-b30d-4554-a5e9-4cea83941824
📒 Files selected for processing (10)
.github/workflows/ci.yml.gitignoreREADME.mdmain.pyrag.pyrequirements.txtscripts/ingest_corpus.pytests/fixtures/hadith_sample.jsonltests/fixtures/quran_sample.jsonltests/test_rag.py
|
|
||
| - name: Run linting | ||
| run: flake8 main.py stellar.py safety tests/redteam study.py --max-line-length=120 --ignore=E501,W503 | ||
| run: flake8 main.py stellar.py safety tests/redteam study.py rag.py --max-line-length=120 --ignore=E501,W503 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Include tests/test_rag.py in the flake8 target.
CI executes the new test file but does not lint it, allowing style violations in changed Python code to merge undetected.
Proposed fix
- run: flake8 main.py stellar.py safety tests/redteam study.py rag.py --max-line-length=120 --ignore=E501,W503
+ run: flake8 main.py stellar.py safety tests/redteam tests/test_rag.py study.py rag.py --max-line-length=120 --ignore=E501,W503As per path instructions, this FastAPI service's CI enforces flake8, so style violations should fail the build.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: flake8 main.py stellar.py safety tests/redteam study.py rag.py --max-line-length=120 --ignore=E501,W503 | |
| run: flake8 main.py stellar.py safety tests/redteam tests/test_rag.py study.py rag.py --max-line-length=120 --ignore=E501,W503 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 31, Update the flake8 command in the CI
workflow to include tests/test_rag.py alongside the existing Python targets,
preserving all current options and exclusions.
Source: Path instructions
| # RAG / ChromaDB | ||
| data/ | ||
| chroma_data/ No newline at end of file |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow the data/ ignore pattern.
Ignoring every data/ directory can silently hide unrelated source-controlled inputs. Prefer the exact generated corpus paths, such as root-level data/*.jsonl, while retaining chroma_data/.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.gitignore around lines 40 - 42, Replace the broad data/ entry in the RAG /
ChromaDB ignore section with a narrowly scoped pattern for generated root-level
data/*.jsonl files, and retain the chroma_data/ ignore rule.
| rag_sources: list[SourceDocument] = [] | ||
|
|
||
| try: | ||
| logger.info(f"Received chat request: {request.prompt[:100]}...") | ||
|
|
||
| chat_id = request.chat_id or str(uuid.uuid4()) | ||
|
|
||
| # --- RAG retrieval (before generate) --- | ||
| rag_passages = "" | ||
| if RAG_ENABLED: | ||
| rag_sources = retrieve(request.prompt) | ||
| if rag_sources: | ||
| rag_passages = format_reference_passages(rag_sources) | ||
| logger.info("RAG: %d source(s) retrieved", len(rag_sources)) | ||
| else: | ||
| logger.info("RAG: no sources retrieved") | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Blocking retrieval call inside the async /chat handler.
retrieve(request.prompt) is a plain synchronous call that internally does a network round-trip (Gemini embeddings) and a Chroma query. Since chat() is async def, this blocks the event loop for every RAG-enabled request, stalling other concurrent requests on the same worker. See linked comment in rag.py for the root cause and suggested async-friendly fix; this call site needs an await/offload once that's in place (e.g. rag_sources = await asyncio.to_thread(retrieve, request.prompt)).
As per path instructions, "network calls should be awaited or offloaded" for a FastAPI async endpoint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.py` around lines 162 - 178, Update the RAG retrieval call in the async
chat handler to offload the synchronous retrieve function to a worker thread,
awaiting its result so the event loop remains responsive. Preserve the existing
RAG_ENABLED branching, source formatting, and logging behavior.
Source: Path instructions
| def embed_text(text: str) -> list[float]: | ||
| """Embed a single text string using Gemini text-embedding-004. | ||
|
|
||
| When _FAKE_EMBEDDING is set (tests), return that instead. | ||
| """ | ||
| if _FAKE_EMBEDDING is not None: | ||
| return _FAKE_EMBEDDING | ||
| import google.generativeai as genai | ||
|
|
||
| result = genai.embed_content( | ||
| model="models/text-embedding-004", | ||
| content=text, | ||
| ) | ||
| return list(result["embedding"]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
search()/embed_text() have no error handling — a single failed embedding call takes down /chat.
ChromaStore.__init__ deliberately catches init failures and degrades gracefully (matching the PR's "preserve chat behavior when retrieval is unavailable" goal), but search() and embed_text() don't mirror that. If genai.embed_content throws (timeout, quota, transient network error) or collection.query throws, the exception propagates all the way up through retrieve() into main.py's /chat try block, which is caught by the outer generic handler and turned into an HTTP 500 for the entire chat request — even though the PR objective is that RAG failures should never break general chat.
🛡️ Proposed fix — degrade to empty results on failure
def search(
self,
query: str,
top_k: int = RAG_TOP_K,
min_score: float = RAG_MIN_SCORE,
) -> list[SourceDocument]:
if not self._ready:
logger.warning("Chroma not available — returning empty results")
return []
- query_emb = embed_text(query)
- results = self._collection.query(
- query_embeddings=[query_emb],
- n_results=top_k,
- include=["documents", "metadatas", "distances"],
- )
+ try:
+ query_emb = embed_text(query)
+ results = self._collection.query(
+ query_embeddings=[query_emb],
+ n_results=top_k,
+ include=["documents", "metadatas", "distances"],
+ )
+ except Exception as exc:
+ logger.warning("RAG search failed: %s — returning empty results", exc)
+ return []As per path instructions ("bare excepts that leak stack traces to clients"): without this guard, an embedding/query failure surfaces as a raw str(e) in the /chat HTTPException detail in main.py.
Also applies to: 150-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rag.py` around lines 53 - 66, Update embed_text() and the
ChromaStore.search() flow to catch embedding and collection.query failures, log
them appropriately, and return empty results instead of propagating exceptions.
Ensure retrieve() continues gracefully when either operation fails so RAG errors
cannot cause /chat to return an HTTP 500 or expose raw exception details.
Source: Path instructions
| def __init__(self, persist_dir: str = CHROMA_PERSIST_DIR) -> None: | ||
| self._persist_dir = persist_dir | ||
| self._collection: Any = None | ||
| try: | ||
| import chromadb | ||
| from chromadb.config import Settings | ||
|
|
||
| self._client = chromadb.PersistentClient( | ||
| path=persist_dir, | ||
| settings=Settings(anonymized_telemetry=False), | ||
| ) | ||
| self._collection = self._client.get_or_create_collection( | ||
| name=CHROMA_COLLECTION, | ||
| ) | ||
| logger.info( | ||
| "ChromaDB ready: persist_dir=%s count=%d", | ||
| persist_dir, | ||
| self._collection.count(), | ||
| ) | ||
| except Exception as exc: | ||
| logger.warning("ChromaDB init failed: %s — RAG disabled", exc) | ||
| self._collection = None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first
git ls-files | rg '^(rag\.py|main\.py|requirements\.txt)$'
printf '\n--- rag.py outline ---\n'
ast-grep outline rag.py --view expanded || true
printf '\n--- main.py outline ---\n'
ast-grep outline main.py --view expanded || true
printf '\n--- rag.py relevant lines ---\n'
sed -n '1,280p' rag.py | cat -n | sed -n '1,280p'
printf '\n--- main.py relevant lines ---\n'
sed -n '1,260p' main.py | cat -n | sed -n '1,260p'
printf '\n--- requirements.txt ---\n'
sed -n '1,220p' requirements.txt | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 22818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any explicit Chroma space configuration elsewhere in the repo.
rg -n "hnsw:space|configuration=\{|get_or_create_collection|PersistentClient|collection\.query|score = 1 - distance|distance" .Repository: Deen-Bridge/dnb-ai
Length of output: 686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If available, inspect ChromaDB version pinning and any docs/comments around score semantics.
python3 - <<'PY'
from pathlib import Path
for path in [Path("requirements.txt"), Path("rag.py"), Path("main.py")]:
if path.exists():
print(f"\n=== {path} ===")
for i, line in enumerate(path.read_text().splitlines(), 1):
if "chroma" in line.lower() or "score" in line.lower() or "distance" in line.lower() or "query(" in line.lower():
print(f"{i}: {line}")
PYRepository: Deen-Bridge/dnb-ai
Length of output: 2029
🌐 Web query:
ChromaDB default collection distance metric hnsw:space default l2 query distance semantics 1 - distance cosine
💡 Result:
In ChromaDB, the default distance metric for HNSW collections is L2 (squared Euclidean distance) [1][2][3]. When configuring a collection, you can specify the distance metric using the hnsw:space parameter in the collection configuration [1][2]. The supported values for this parameter are: l2: Squared L2 (Euclidean) distance [2][3]. This is the default setting [1][2]. cosine: Cosine distance [1][2]. ChromaDB implements this as 1 - cosine_similarity [1][4][5]. Consequently, a distance of 0.0 indicates identical vectors (perfect similarity), while values closer to 2.0 indicate greater dissimilarity [5]. ip: Inner product [1][2]. Important technical considerations include: Collection Configuration: The distance metric (hnsw:space) must be defined at the time of collection creation [1]. It cannot be changed for an existing collection; you must clone the collection or create a new one to use a different metric [1][6]. Normalization: Because L2 and inner product distance metrics are sensitive to the magnitude of vectors, it is often recommended to normalize your embeddings if you want to use them for cosine-like comparisons or to ensure consistency across different models [3][7][8]. Distance vs. Similarity: ChromaDB returns distances in its query results [5][9]. When using the cosine distance metric, you can derive the similarity score using the formula: similarity = 1 - distance [5]. Lower distances always indicate higher similarity [5][9].
Citations:
- 1: https://cookbook.chromadb.dev/core/collections/
- 2: https://docs.trychroma.com/docs/collections/configure
- 3: https://cookbook.chromadb.dev/faq/
- 4: https://stackoverflow.com/questions/77794024/searching-existing-chromadb-database-using-cosine-similarity
- 5: https://topictrick.com/blog/chromadb-tutorial-beginner-guide
- 6: Chroma VectorBase Use "L2" as Similarity Measure Rather than Cosine langchain-ai/langchain#21599
- 7: [Bug]: Cosine distance greater than 1 in query distances chroma-core/chroma#1350
- 8: https://medium.com/@razikus/chromadb-defaults-to-l2-distance-why-that-might-not-be-the-best-choice-ac3d47461245
- 9: https://docs.trychroma.com/cloud/search-api/overview
Set Chroma’s distance space to match the score conversion. get_or_create_collection() leaves this collection on Chroma’s default L2 distance, but search() interprets results as cosine with 1 - distance. That can clamp valid hits to 0.0, break RAG_MIN_SCORE, and surface misleading scores to /chat clients. Create the collection with cosine space, and recreate/migrate any existing persisted collection if it was already initialized.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 124-124: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rag.py` around lines 105 - 126, Update the collection initialization in
__init__ to create CHROMA_COLLECTION with cosine distance metadata, matching
search()’s 1 - distance score conversion. Detect any existing persisted
collection using the incompatible distance space, migrate or recreate it with
cosine space while preserving its documents and embeddings, then retain the
existing collection setup and failure behavior.
Source: Path instructions
| def retrieve(query: str) -> list[SourceDocument]: | ||
| """Convenience: search the singleton store.""" | ||
| if not RAG_ENABLED: | ||
| logger.info("RAG disabled — skipping retrieval") | ||
| return [] | ||
| return get_store().search(query) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
retrieve() performs blocking network + disk I/O and is called synchronously from the async /chat endpoint.
retrieve() → search() → embed_text() makes a synchronous HTTP call to Gemini and a synchronous Chroma disk query. main.py's chat() is async def and awaits other work (safety pipeline, model generation), but calls retrieve() directly with no await/offload — this blocks the event loop for the full round-trip of every RAG-enabled chat request, stalling all other concurrent requests on the same worker.
Consider making this async internally so callers can await it without changing call sites elsewhere:
import asyncio
async def retrieve(query: str) -> list[SourceDocument]:
if not RAG_ENABLED:
logger.info("RAG disabled — skipping retrieval")
return []
return await asyncio.to_thread(get_store().search, query)As per path instructions, "Flag blocking calls inside async endpoints (network calls should be awaited or offloaded)" — see linked consolidated comment for the main.py call site.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rag.py` around lines 243 - 248, Make retrieve asynchronous and offload the
blocking get_store().search(query) operation to asyncio.to_thread, while
preserving the disabled-RAG early return and existing logging. Update the async
/chat caller to await retrieve so synchronous embedding, network, and disk work
does not block the event loop.
Source: Path instructions
| To build the ChromaDB index, run the ingestion script after setup: | ||
|
|
||
| ```bash | ||
| python scripts/ingest_corpus.py | ||
| ``` | ||
|
|
||
| This downloads the Saheeh International Quran translation (Tanzil.net) and | ||
| Sahih al-Bukhari hadith (permissively licensed GitHub export), chunks them | ||
| (one ayah per chunk, one hadith per chunk), embeds them via Gemini | ||
| `text-embedding-004`, and stores the vectors in the directory configured by | ||
| `CHROMA_PERSIST_DIR` (default `chroma_data/`). The raw corpus JSONL is | ||
| cached under `data/`. | ||
|
|
||
| Use `--reuse-cache` to skip re-downloading: | ||
| ```bash | ||
| python scripts/ingest_corpus.py --reuse-cache |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document how to enable RAG after ingestion.
The documented flow builds the index and starts the API, but RAG_ENABLED defaults to 0. Users following these steps will not receive retrieval or sources unless they independently discover that RAG_ENABLED=1 is required. Add that setting to the setup example or explicitly show the enablement step.
This follows the documented default in this README and the retrieval gate supplied from rag.py.
Also applies to: 100-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 76 - 91, The README ingestion instructions do not
enable retrieval after building the ChromaDB index. Update the setup example
around `scripts/ingest_corpus.py` to explicitly set `RAG_ENABLED=1` before
starting the API, or add a clear enablement step, while preserving the existing
ingestion and `--reuse-cache` commands.
| def ingest(entries: list[dict], store: ChromaStore) -> None: | ||
| texts = [e["text"] for e in entries] | ||
| metadatas = [ | ||
| {k: v for k, v in e.items() if k != "text"} for e in entries | ||
| ] | ||
| ids = [ | ||
| f"{e['source']}-{e.get('surah', e.get('hadith_number', idx))}" | ||
| for idx, e in enumerate(entries) | ||
| ] | ||
| store.add_documents(texts, metadatas, ids) | ||
| logger.info("Ingested %d documents into Chroma", len(texts)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Critical: Quran ingestion IDs collide by surah, discarding almost the entire corpus.
ids is built as f"{e['source']}-{e.get('surah', e.get('hadith_number', idx))}". Every Quran entry has surah set (see download_quran, line 107), so all ayat in the same surah produce the identical id (e.g. every ayah of Al-Baqarah becomes "quran-2"). collection.add() requires unique ids within a batch — this will either raise on the duplicate ids or silently overwrite, leaving at most one ayah per surah (≤114 documents) instead of the full ~6,236-verse corpus.
🐛 Proposed fix — include the ayah/hadith_number in the id
ids = [
- f"{e['source']}-{e.get('surah', e.get('hadith_number', idx))}"
+ f"{e['source']}-{e.get('surah', '')}-{e.get('ayah', e.get('hadith_number', idx))}"
for idx, e in enumerate(entries)
]Do you want me to open a follow-up issue and generate a regression test asserting unique ids per surah/ayah pair?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def ingest(entries: list[dict], store: ChromaStore) -> None: | |
| texts = [e["text"] for e in entries] | |
| metadatas = [ | |
| {k: v for k, v in e.items() if k != "text"} for e in entries | |
| ] | |
| ids = [ | |
| f"{e['source']}-{e.get('surah', e.get('hadith_number', idx))}" | |
| for idx, e in enumerate(entries) | |
| ] | |
| store.add_documents(texts, metadatas, ids) | |
| logger.info("Ingested %d documents into Chroma", len(texts)) | |
| def ingest(entries: list[dict], store: ChromaStore) -> None: | |
| texts = [e["text"] for e in entries] | |
| metadatas = [ | |
| {k: v for k, v in e.items() if k != "text"} for e in entries | |
| ] | |
| ids = [ | |
| f"{e['source']}-{e.get('surah', '')}-{e.get('ayah', e.get('hadith_number', idx))}" | |
| for idx, e in enumerate(entries) | |
| ] | |
| store.add_documents(texts, metadatas, ids) | |
| logger.info("Ingested %d documents into Chroma", len(texts)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/ingest_corpus.py` around lines 169 - 179, Update the id construction
in ingest so Quran entries include both their surah and ayah identifiers,
producing a unique ID for every verse; retain hadith_number for hadith entries
and the index fallback for other records. Ensure the resulting ids passed to
store.add_documents are unique within the batch.
| def test_rag_config_defaults(): | ||
| # These are read from env at module import time. | ||
| assert RAG_TOP_K >= 1 | ||
| assert RAG_MIN_SCORE >= 0.0 | ||
| assert isinstance(RAG_ENABLED, bool) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the documented defaults and disabled-retrieval behavior.
These checks allow incorrect values such as RAG_TOP_K=10 or RAG_MIN_SCORE=0.5, and they never verify that retrieve() returns [] when RAG_ENABLED is false. Isolate the environment/import and assert the exact defaults, plus the disabled gate described in rag.py.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_rag.py` around lines 232 - 236, The test_rag_config_defaults
function should isolate environment variables and module import so it validates
the documented exact default values for RAG_TOP_K, RAG_MIN_SCORE, and
RAG_ENABLED rather than broad ranges. Extend the test to exercise retrieve()
with RAG_ENABLED disabled and assert it returns an empty list, matching the
disabled gate in rag.py.
|
@GEEKYFOCUS pls fix conflict |
# Conflicts: # .github/workflows/ci.yml # README.md # main.py # requirements.txt
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
main.py (3)
191-194: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude
madhhabin the semantic-cache key.
madhhabnow changes the generated system prompt, but cache eligibility and lookup remain based only on the prompt embedding. The same question can therefore return a response generated for a different madhhab. Include the normalized madhhab in the cache identity, or disable semantic caching when it is set.Also applies to: 235-241
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` around lines 191 - 194, Update the semantic-cache eligibility and lookup flow around normalize_madhhab, classify_fiqh, and FiqhInfo so the normalized madhhab contributes to the cache identity whenever it is set. Ensure cache reads and writes use the same madhhab-aware key, or disable semantic caching for requests with a specified madhhab.
102-102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
ChatRequest.madhhabat the model boundary.
normalize_madhhab()accepts aliases and silently maps unknown strings toNone, so typos or unsupported values are just dropped. A Pydantic validator here would make the request contract explicit and avoid surprising fiqh behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` at line 102, Update the ChatRequest model’s madhhab field validation to reject unsupported or misspelled values at request parsing time, while accepting the canonical madhhab names and aliases handled by normalize_madhhab(). Preserve None as the unset value, and ensure invalid non-null strings raise the model’s standard validation error instead of being silently converted to None.Source: Path instructions
115-116: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftWire RAG into
/chat.rag_sourcesis initialized but never populated,format_reference_passages(rag_sources)never reachesfull_prompt, andChatResponsestill has no field for returning sources. That leaves RAG effectively disabled even whenRAG_ENABLEDis on. Populate the retrieval results, inject the formatted passages, and return them on both the cache-hit and normal paths; if retrieval stays synchronous, move it off the async request path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` around lines 115 - 116, Update the /chat handler and its ChatResponse construction to execute retrieval when RAG_ENABLED, populate rag_sources, inject format_reference_passages(rag_sources) into full_prompt, and expose the sources through a ChatResponse field. Return the same populated sources on both cache-hit and normal response paths, and run synchronous retrieval outside the async request path if no async implementation exists.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@main.py`:
- Around line 191-194: Update the semantic-cache eligibility and lookup flow
around normalize_madhhab, classify_fiqh, and FiqhInfo so the normalized madhhab
contributes to the cache identity whenever it is set. Ensure cache reads and
writes use the same madhhab-aware key, or disable semantic caching for requests
with a specified madhhab.
- Line 102: Update the ChatRequest model’s madhhab field validation to reject
unsupported or misspelled values at request parsing time, while accepting the
canonical madhhab names and aliases handled by normalize_madhhab(). Preserve
None as the unset value, and ensure invalid non-null strings raise the model’s
standard validation error instead of being silently converted to None.
- Around line 115-116: Update the /chat handler and its ChatResponse
construction to execute retrieval when RAG_ENABLED, populate rag_sources, inject
format_reference_passages(rag_sources) into full_prompt, and expose the sources
through a ChatResponse field. Return the same populated sources on both
cache-hit and normal response paths, and run synchronous retrieval outside the
async request path if no async implementation exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f6a41256-4b62-4c7b-8f0b-e59dd392dde3
📒 Files selected for processing (3)
.github/workflows/ci.ymlREADME.mdmain.py
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
3 participants
Overview
This PR adds retrieval-augmented generation (RAG) to the
/chatendpoint. Quran and hadith passages are embedded into a ChromaDB persistent vector store, retrieved by cosine similarity on the user query, and injected into the model prompt with metadata — so answers can cite real sources instead of relying on the model's parametric memory.Related Issue
Closes #24
Verification Results
flake8 main.py rag.py tests/test_rag.py scripts/ingest_corpus.py— lint ✅ passedpython -m compileall main.py rag.py scripts/ingest_corpus.py— syntax ✅ passedpytest tests/test_rag.py -v— 11/11 ✅ passedChanges
⚙️ RAG Module
rag.py— Core RAG module:ChromaStore: ChromaDB in embedded/persistent mode (no separate server needed)VectorStoreinterface for future swap to qdrant/pgvectortext-embedding-004behind a testable seamRAG_TOP_K(default 5) andRAG_MIN_SCORE(default 0.0)format_reference_passages()formats retrieved passages for prompt injectionSourceDocumentPydantic model (text, reference, score)📥 Ingestion Script
scripts/ingest_corpus.py— Offline script (not at request time):data/;--reuse-cacheto skip re-download🌐 API Integration
main.py—/chatendpoint:RAG_ENABLED=1ChatResponse.sourcesoptional field with retrieved passagesNonewhen absent)🧪 Tests
tests/test_rag.py— 11 offline tests with fake embeddings:SourceDocumentmodel creationtests/fixtures/quran_sample.jsonl— 10 ayattests/fixtures/hadith_sample.jsonl— 5 hadith🔧 Configuration
RAG_ENABLED0RAG_TOP_K5RAG_MIN_SCORE0.0CHROMA_PERSIST_DIRchroma_data.github/workflows/ci.yml— Lint/compilerag.py, run RAG tests.gitignore— Ignoredata/andchroma_data/requirements.txt— Addedchromadb>=0.5.0README.md— Documented env vars, ingestion instructions, RAG featureAcceptance Criteria
/chatanswers includesourceslist with passages; out-of-corpus questions work with empty sourcesRAG_TOP_KandRAG_MIN_SCOREenv-configurable and documentedSummary by CodeRabbit