Migrate RAG retrieval to OpenRAG, add runtime health checks and env-based config#906
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the RAG system from a Qdrant-specific implementation to an OpenRAG-based backend. Key changes include the introduction of a new openrag_backend.py module for integration, a health check system for dependencies and configuration, and the refactoring of query tools to use the new backend. Feedback identifies a logic inconsistency where the health check requires an environment variable that the backend provides a default for. Additionally, suggestions were made to refine error handling in the retriever search wrapper to avoid masking bugs and to improve the merging logic for metadata filters to ensure clinical concepts are correctly combined.
| "langchain_ollama_installed": find_spec("langchain_ollama") is not None, | ||
| "langchain_core_installed": find_spec("langchain_core") is not None, | ||
| "dotenv_installed": find_spec("dotenv") is not None, | ||
| "openrag_endpoint_set": bool(os.getenv("OPENRAG_ENDPOINT")), |
There was a problem hiding this comment.
There is a logic inconsistency between the health check and the backend implementation. The health check marks the system as not ready if OPENRAG_ENDPOINT is missing from the environment, but openrag_backend.py (line 97) provides a default value of http://localhost:8000. If the default is intended to be supported, the health check should be updated to allow it. If the environment variable is strictly required for production safety, the default in the backend should be removed to avoid misleading behavior.
| try: | ||
| return retriever.search(**params) | ||
| except TypeError: | ||
| continue |
There was a problem hiding this comment.
Catching TypeError broadly here is risky as it can mask internal bugs within the retriever.search implementation (e.g., a TypeError raised deep inside the library logic) by incorrectly treating it as a signature mismatch. It is safer to check if the error specifically refers to unexpected keyword arguments.
| try: | |
| return retriever.search(**params) | |
| except TypeError: | |
| continue | |
| try: | |
| return retriever.search(**params) | |
| except TypeError as exc: | |
| if "unexpected keyword argument" in str(exc): | |
| continue | |
| raise |
| for key, value in omop_filters.items(): | ||
| llm_filters.setdefault(key, value) |
There was a problem hiding this comment.
Using setdefault here prevents the merging of clinical concepts when both the LLM and the OMOP detection logic identify relevant metadata. Since these fields (like omop_condition_concept_ids) are lists of identifiers, they should be combined and de-duplicated to ensure maximum retrieval recall, rather than letting the LLM output silently override the deterministic OMOP extraction.
| for key, value in omop_filters.items(): | |
| llm_filters.setdefault(key, value) | |
| for key, value in omop_filters.items(): | |
| existing = llm_filters.get(key) or [] | |
| llm_filters[key] = sorted(set(existing) | set(value)) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2f96120d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and status["langchain_ollama_installed"] | ||
| and status["langchain_core_installed"] | ||
| and status["dotenv_installed"] | ||
| and status["openrag_endpoint_set"] |
There was a problem hiding this comment.
is_rag_runtime_ready() currently requires OPENRAG_ENDPOINT to be explicitly set, but openrag_hybrid_search() already provides a working default (http://localhost:8000). This creates a false-negative health state where the service is reported “not ready” even though retrieval can run, which can block startup/traffic in environments that rely on this readiness signal.
Useful? React with 👍 / 👎.
| return retriever.search(**params) | ||
| except TypeError: | ||
| continue |
There was a problem hiding this comment.
The compatibility loop catches every TypeError from retriever.search(**params) and retries alternate signatures, but TypeError can also come from inside the search implementation (e.g., bad filter payload/type). In that case the original failure is swallowed and replaced by an “incompatible signature” runtime error, which misdiagnoses production failures and makes debugging much harder.
Useful? React with 👍 / 👎.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "dotenv_installed", | ||
| "openrag_endpoint_set", | ||
| } | ||
| assert set(status.keys()) == expected |
| "openrag_endpoint_set", | ||
| } | ||
| assert set(status.keys()) == expected | ||
| assert all(isinstance(value, bool) for value in status.values()) |
|
|
||
|
|
||
| def test_is_rag_runtime_ready_returns_bool() -> None: | ||
| assert isinstance(is_rag_runtime_ready(), bool) |
|
|
||
| def test_rag_runtime_not_ready_without_endpoint(monkeypatch) -> None: | ||
| monkeypatch.delenv("OPENRAG_ENDPOINT", raising=False) | ||
| assert is_rag_runtime_ready() is False |
| try: | ||
| openrag_hybrid_search(query="test", k=3, filters={"doc_year": 2026}) | ||
| except RuntimeError as exc: | ||
| assert "required" in str(exc) |
| assert docs[0].page_content == "alpha" | ||
| assert docs[0].metadata == {"a": 1} | ||
| assert docs[1].page_content == "beta" | ||
| assert docs[2].page_content == "gamma" |
|
|
||
|
|
||
| def test_minimum_openrag_version_is_pinned() -> None: | ||
| assert str(MIN_OPENRAG_VERSION) == "0.4.1" |
| def test_search_openrag_supports_top_k_metadata_filters_signature() -> None: | ||
| class DummyRetriever: | ||
| def search(self, **kwargs): | ||
| assert kwargs["top_k"] == 5 |
| class DummyRetriever: | ||
| def search(self, **kwargs): | ||
| assert kwargs["top_k"] == 5 | ||
| assert kwargs["metadata_filters"] == {"doc_year": 2026} |
| k=5, | ||
| filters={"doc_year": 2026}, | ||
| ) | ||
| assert result == [{"content": "ok"}] |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 9 minor |
| Security | 14 high |
🟢 Metrics 33 complexity
Metric Results Complexity 33
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
2 participants
Motivation
Description
RAG/openrag_backend.pywith OpenRAG integration, including a pinned minimum version check (MIN_OPENRAG_VERSION), a backward-compatible_search_openragwrapper, and normalization to LangChainDocumentvia_to_langchain_documents.RAG/healthcheck.pyexposingrag_runtime_health()andis_rag_runtime_ready()to report dependency and config readiness.RAG/query.pyto useopenrag_hybrid_searchinstead of Qdrant, move Ollama model and base URL to environment-controlled defaults viaRAG_LLM_MODELandOLLAMA_BASE_URL, and keep the reranker logic intact.RAG/scripts/rag_tools.pyto use OpenRAG retrieval and the same env-driven Ollama config, and merge OMOP-inferred filters with structured-LLM filters inextract_filters.RAG/agent.pyto readRAG_LLM_MODELandOLLAMA_BASE_URLfrom the environment when constructing theChatOllamaclient.RAG/tests/test_healthcheck.pyandRAG/tests/test_openrag_backend.pycovering health checks, minimum version pinning, search signature compatibility, and normalization logic.pyproject.tomlto addopenrag>=0.4.1,<0.5.0andpackaging>=25.0to dependencies.Testing
RAG/tests/test_healthcheck.pywhich verifiesrag_runtime_health()shape andis_rag_runtime_ready()behavior; tests passed.RAG/tests/test_openrag_backend.pywhich validates version pinning, normalization (_to_langchain_documents), and_search_openragsignature compatibility; tests passed.pytestfor the modified modules and observed all new tests succeeding.Codex Task