Open
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR refactors the reranker system to support multiple providers (Infinity and OpenAI-compatible) using a factory pattern. It reorganizes reranker code into a modular structure with provider-specific implementations, updates Hydra configurations to be composable, and adjusts the pipeline to instantiate rerankers dynamically based on configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Ahmath-Gadji
added
breaking-change
BREAKING CHANGE: docker-compose.yaml now includes
`extern/reranker/${RERANKER_PROVIDER:-infinity}.yaml` instead of
`extern/infinity.yaml`. Set RERANKER_PROVIDER=infinity or leave unset
to preserve existing behavior.
Add OpenAI-compatible reranker provider selectable via RERANKER_PROVIDER
(values: `infinity`, `openai`). New env vars: RERANKER_API_KEY, RERANKER_SEMAPHORE.
Ahmath-Gadji
force-pushed
the
feat/add_openai_reranking
branch
from
b9a9bbd to
c4c3346
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
openrag/components/reranker/openai.py (1)
48-55: Prefer bareraiseto preserve the original traceback.Using
raise ecan subtly alter the traceback. Use bareraiseinstead.- raise e + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/reranker/openai.py` around lines 48 - 55, The except block in the reranker method logs the exception but re-raises using "raise e", which can alter the traceback; update the except handler that references logger, self.model_name and documents (the block that logs "Reranking failed") to re-raise the caught exception with a bare "raise" instead of "raise e" so the original traceback is preserved.openrag/components/reranker/infinity.py (1)
45-52: Prefer bareraiseto preserve the original traceback.- raise e + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/reranker/infinity.py` around lines 45 - 52, In the except Exception as e block that logs reranking failures (the block referencing logger.error with model_name=self.model_name and documents_count=len(documents)), replace the current "raise e" with a bare "raise" so the original traceback is preserved; keep the logger.error call and exception variable for logging, but re-raise using "raise" instead of "raise e".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extern/reranker/openai.yaml`:
- Around line 17-20: The service listens on container port 8000 but the Docker
port mapping uses ${RERANKER_PORT:-8003}, causing mismatch when RERANKER_PORT is
overridden; update the OpenAI vLLM service command blocks in openai.yaml to
include a --port flag using the same variable (e.g., add --port
${RERANKER_PORT:-8003}) in both the main reranker command and the reranker-cpu
command so the container binds to the same overridable port used in the mapping
and Hydra URLs, ensuring port alignment across RERANKER_PORT, the command lines,
and the port mapping.
In `@openrag/components/reranker/infinity.py`:
- Line 7: Change the relative-style import to an absolute import from the
openrag package: replace the current import of get_logger in infinity.py with an
absolute import that references openrag.utils.logger (e.g., import get_logger
from openrag.utils.logger) so the symbol get_logger is imported via the
project's top-level package name per coding guidelines.
In `@openrag/components/reranker/openai.py`:
- Line 5: Replace the relative import of the logger used in openai.py: instead
of importing get_logger from a local/relative utils module, change it to use the
project-root absolute package import so get_logger is imported from the
top-level utils.logger package (i.e., the absolute openrag package path) to
comply with the coding guideline; update the import statement that currently
references utils.logger to the absolute package import for get_logger.
- Around line 27-38: The Async HTTP call to self.rerank_url using
httpx.AsyncClient.post has no timeout and can hang; update the reranker to set a
request timeout (either by adding a configurable attribute like self.timeout on
the reranker class and passing timeout=self.timeout to client.post, or by
constructing httpx.AsyncClient(timeout=...) / using httpx.Timeout) so the post
call to self.rerank_url will fail fast on slow/unresponsive services; ensure the
timeout value is used in the call site that invokes httpx.AsyncClient().post and
consider catching httpx.TimeoutException where appropriate.
In `@openrag/components/reranker/test_rrf_reranking.py`:
- Line 5: The test file uses a relative import for BaseReranker; change the
relative import to an absolute one so it imports BaseReranker from the package
root (use the full module path, e.g. import BaseReranker from
openrag.components.reranker.base) to comply with project import guidelines and
avoid relative import issues.
---
Nitpick comments:
In `@openrag/components/reranker/infinity.py`:
- Around line 45-52: In the except Exception as e block that logs reranking
failures (the block referencing logger.error with model_name=self.model_name and
documents_count=len(documents)), replace the current "raise e" with a bare
"raise" so the original traceback is preserved; keep the logger.error call and
exception variable for logging, but re-raise using "raise" instead of "raise e".
In `@openrag/components/reranker/openai.py`:
- Around line 48-55: The except block in the reranker method logs the exception
but re-raises using "raise e", which can alter the traceback; update the except
handler that references logger, self.model_name and documents (the block that
logs "Reranking failed") to re-raise the caught exception with a bare "raise"
instead of "raise e" so the original traceback is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30e4cae1-728d-4d4c-a208-50bebaee0198
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.hydra_config/config.yaml.hydra_config/reranker/base.yaml.hydra_config/reranker/infinity.yaml.hydra_config/reranker/openai.yamldocker-compose.yamldocs/content/docs/documentation/env_vars.mdextern/reranker/infinity.yamlextern/reranker/openai.yamlopenrag/app_front.pyopenrag/components/pipeline.pyopenrag/components/reranker.pyopenrag/components/reranker/__init__.pyopenrag/components/reranker/base.pyopenrag/components/reranker/infinity.pyopenrag/components/reranker/openai.pyopenrag/components/reranker/test_rrf_reranking.py
💤 Files with no reviewable changes (1)
- openrag/components/reranker.py
Comment on lines
+17
to
+20
| command: > | ||
| --model ${RERANKER_MODEL:-BAAI/bge-reranker-v2-m3} | ||
| --trust-remote-code | ||
| --gpu_memory_utilization 0.3 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect RERANKER_PORT usage and vLLM listen-port flags"
rg -n 'RERANKER_PORT|--port|ports:' extern/reranker/openai.yaml .hydra_config/reranker/openai.yamlRepository: linagora/openrag
Length of output: 343
🏁 Script executed:
cat -n extern/reranker/openai.yamlRepository: linagora/openrag
Length of output: 1962
🏁 Script executed:
# Check vLLM documentation or code comments in the repo
rg -i 'port|listen' extern/reranker/ --context 3Repository: linagora/openrag
Length of output: 1628
🏁 Script executed:
# Check how RERANKER_PORT is used in configs and environment
rg -n 'RERANKER_PORT' --context 2Repository: linagora/openrag
Length of output: 3180
RERANKER_PORT override breaks service connectivity when overridden.
The openai.yaml vLLM service lacks a --port flag in its commands (lines 17-20 and 56-59), so it always listens on port 8000 internally. However, the port mapping on line 28 (${RERANKER_PORT:-8003}:8000) changes when RERANKER_PORT is set to a non-default value. When overridden, the port mapping remaps the container's port 8000 to the host, but the Hydra configuration constructs URLs using the overridden port number, causing a mismatch. Compare this to the infinity.yaml reranker variant, which correctly passes --port ${RERANKER_PORT:-7997} to the service.
Add the --port flag to both reranker command definitions and update the port mapping to maintain port alignment across the stack.
Proposed fix
command: >
--model ${RERANKER_MODEL:-BAAI/bge-reranker-v2-m3}
--trust-remote-code
--gpu_memory_utilization 0.3
+ --port ${RERANKER_PORT:-8000}
ports:
- - ${RERANKER_PORT:-8003}:8000
+ - ${RERANKER_PORT:-8003}:${RERANKER_PORT:-8000}Apply the same change to the reranker-cpu command block (lines 56-59).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extern/reranker/openai.yaml` around lines 17 - 20, The service listens on
container port 8000 but the Docker port mapping uses ${RERANKER_PORT:-8003},
causing mismatch when RERANKER_PORT is overridden; update the OpenAI vLLM
service command blocks in openai.yaml to include a --port flag using the same
variable (e.g., add --port ${RERANKER_PORT:-8003}) in both the main reranker
command and the reranker-cpu command so the container binds to the same
overridable port used in the mapping and Hydra URLs, ensuring port alignment
across RERANKER_PORT, the command lines, and the port mapping.
| from infinity_client.api.default import rerank | ||
| from infinity_client.models import RerankInput, ReRankResult | ||
| from langchain_core.documents.base import Document | ||
| from utils.logger import get_logger |
There was a problem hiding this comment.
Use absolute import from openrag/ directory.
Per coding guidelines, imports should use absolute paths from the openrag/ directory.
-from utils.logger import get_logger
+from openrag.utils.logger import get_logger📝 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.
Suggested change
| from utils.logger import get_logger | |
| from openrag.utils.logger import get_logger |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/reranker/infinity.py` at line 7, Change the relative-style
import to an absolute import from the openrag package: replace the current
import of get_logger in infinity.py with an absolute import that references
openrag.utils.logger (e.g., import get_logger from openrag.utils.logger) so the
symbol get_logger is imported via the project's top-level package name per
coding guidelines.
|
|
||
| import httpx | ||
| from langchain_core.documents.base import Document | ||
| from utils.logger import get_logger |
There was a problem hiding this comment.
Use absolute import from openrag/ directory.
Per coding guidelines, imports should use absolute paths from the openrag/ directory.
-from utils.logger import get_logger
+from openrag.utils.logger import get_logger📝 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.
Suggested change
| from utils.logger import get_logger | |
| from openrag.utils.logger import get_logger |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/reranker/openai.py` at line 5, Replace the relative import
of the logger used in openai.py: instead of importing get_logger from a
local/relative utils module, change it to use the project-root absolute package
import so get_logger is imported from the top-level utils.logger package (i.e.,
the absolute openrag package path) to comply with the coding guideline; update
the import statement that currently references utils.logger to the absolute
package import for get_logger.
Comment on lines
+27
to
+38
| async with httpx.AsyncClient() as client: | ||
| response = await client.post( | ||
| self.rerank_url, | ||
| headers={"Authorization": f"Bearer {self.api_key}"}, | ||
| json={ | ||
| "model": self.model_name, | ||
| "query": query, | ||
| "documents": [doc.page_content for doc in documents], | ||
| "top_n": top_k, | ||
| }, | ||
| ) | ||
| response.raise_for_status() |
There was a problem hiding this comment.
Add timeout to HTTP request to prevent indefinite hangs.
The httpx.AsyncClient.post() call has no timeout configured. If the reranker service is slow or unresponsive, this will block indefinitely, potentially exhausting the semaphore.
🛠️ Proposed fix: add timeout parameter
async with httpx.AsyncClient() as client:
response = await client.post(
self.rerank_url,
headers={"Authorization": f"Bearer {self.api_key}"},
json={
"model": self.model_name,
"query": query,
"documents": [doc.page_content for doc in documents],
"top_n": top_k,
},
+ timeout=60.0, # Consider making this configurable
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/reranker/openai.py` around lines 27 - 38, The Async HTTP
call to self.rerank_url using httpx.AsyncClient.post has no timeout and can
hang; update the reranker to set a request timeout (either by adding a
configurable attribute like self.timeout on the reranker class and passing
timeout=self.timeout to client.post, or by constructing
httpx.AsyncClient(timeout=...) / using httpx.Timeout) so the post call to
self.rerank_url will fail fast on slow/unresponsive services; ensure the timeout
value is used in the call site that invokes httpx.AsyncClient().post and
consider catching httpx.TimeoutException where appropriate.
| from components.reranker import BaseReranker | ||
| from langchain_core.documents.base import Document | ||
|
|
||
| from .base import BaseReranker |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify relative imports in the touched test file"
rg -nP '^from \.' openrag/components/reranker/test_rrf_reranking.pyRepository: linagora/openrag
Length of output: 141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if base.py exists and contains BaseReranker ==="
if [ -f "openrag/components/reranker/base.py" ]; then
echo "✓ base.py found"
rg -A 2 "class BaseReranker" openrag/components/reranker/base.py | head -5
else
echo "✗ base.py not found"
fi
echo ""
echo "=== Verify the proposed absolute import would work ==="
python3 << 'EOF'
import sys
sys.path.insert(0, '.')
try:
from openrag.components.reranker.base import BaseReranker
print(f"✓ Absolute import works, BaseReranker is: {BaseReranker}")
except Exception as e:
print(f"✗ Absolute import failed: {e}")
EOFRepository: linagora/openrag
Length of output: 469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check all imports in test_rrf_reranking.py ==="
rg -n "^from |^import " openrag/components/reranker/test_rrf_reranking.pyRepository: linagora/openrag
Length of output: 196
Use absolute import for BaseReranker.
Line 5 uses a relative import; this must be absolute from the openrag/ root per coding guidelines.
🔧 Proposed fix
-from .base import BaseReranker
+from openrag.components.reranker.base import BaseReranker🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/reranker/test_rrf_reranking.py` at line 5, The test file
uses a relative import for BaseReranker; change the relative import to an
absolute one so it imports BaseReranker from the package root (use the full
module path, e.g. import BaseReranker from openrag.components.reranker.base) to
comply with project import guidelines and avoid relative import issues.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor reranker into a multi-provider architecture
Replaces the single-file reranker with a factory pattern supporting multiple backends (Infinity and OpenAI-compatible endpoints). The provider is selected at runtime via configuration.
Changes:
BaseReranker,InfinityReranker, andOpenAIRerankerclasses underopenrag/components/reranker/.hydra_config/reranker/)docker-compose.yamlfor dynamic provider selection; addedextern/reranker/openai.yamlwith GPU/CPU supportSummary by CodeRabbit
Release Notes
New Features
Chores