Feat/add ruff linting - #214
Conversation
- Apply ruff code linting (PEP 8, Pyflakes, isort, etc.) - Auto-format code to 120 character line length - Sort imports with isort rules - Modernize Python code with pyupgrade rules
formatting checks on all pull requests and pushes to main/dev branches.
📝 WalkthroughWalkthroughThis PR modernizes type hints across the codebase from typing module constructs ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The large number of affected files (~95) with heterogeneous changes across models, loaders, routers, and scripts requires careful verification of type correctness throughout. While most changes follow a consistent pattern (type hint modernization), the functional enhancements (sanitize_text, transcribe helper, validation logic) and configuration additions add complexity. The scope demands attention to ensure no runtime behavioral regressions despite the primarily cosmetic nature of most edits. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
coderabbitai
Bot
added
the
chore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (21)
openrag/components/indexer/loaders/pdf_loaders/pymupdf.py (2)
14-27: Type annotations are incomplete and inconsistent with base class.Given that this PR modernizes type hints, this method signature should align with the base class and sibling implementations. Currently missing:
file_path: str | Pathsave_markdown: bool- Return type
-> Documentmetadatashould bedict | None = None(notdict = None)Proposed fix
- async def aload_document(self, file_path, metadata: dict = None, save_markdown=False): + async def aload_document(self, file_path: str | Path, metadata: dict | None = None, save_markdown: bool = False) -> Document:
34-44: Same type annotation gaps as above.This method has identical type hint inconsistencies with the base class signature.
Proposed fix
- async def aload_document(self, file_path, metadata: dict = None, save_markdown=False): + async def aload_document(self, file_path: str | Path, metadata: dict | None = None, save_markdown: bool = False) -> Document:openrag/routers/users.py (1)
164-188: Add authorization check for token regeneration endpoint.The endpoint lacks the
admin_user=Depends(require_admin)dependency that all other sensitive user operations have (list_users,create_user,get_user,delete_user). The docstring claims "Requires admin role (or user can regenerate their own token)" but the code enforces neither—any authenticated user can regenerate any other user's token. The vectordb implementation checks only that the user exists, not authorization or ownership.Either add
admin_user=Depends(require_admin)if only admins should regenerate tokens, or implement user self-service logic that validates the requesting user matches the target user_id.automatic-evaluation-pipeline/benchmark.py (3)
88-97: Missing environment variables will silently passNonetoChatOpenAI.Using
os.environ.get()without defaults meansMODEL,BASE_URL, andAPI_KEYcould beNoneif not set. Sincellm_completion_judgeandllm_precision_judgeare initialized at module level (lines 171-172), this will cause failures when the judges are invoked rather than at startup.Consider validating required environment variables early or providing sensible defaults.
Suggested improvement
+def get_required_env(key: str) -> str: + value = os.environ.get(key) + if not value: + raise ValueError(f"Required environment variable '{key}' is not set") + return value + llm_judge_settings = { - "model": os.environ.get("MODEL"), - "base_url": os.environ.get("BASE_URL"), - "api_key": os.environ.get("API_KEY"), + "model": get_required_env("MODEL"), + "base_url": get_required_env("BASE_URL"), + "api_key": get_required_env("API_KEY"), "temperature": 0.2,
201-204: Type inconsistency: returning"error"strings whereintscores are expected.The function returns
("error", "error")on exceptions, but the success path returns(int, int). This mixed-type return complicates downstream handling and could cause runtime errors if the results are used numerically before filtering.Suggested improvement using `None`
except Exception as e: logger.debug(f"Error evaluating response: {e}") - return "error", "error" + return None, NoneThen update the filter on line 279:
- valid_scores = [(comp, prec) for comp, prec in llm_judge_scores if comp != "error"] + valid_scores = [(comp, prec) for comp, prec in llm_judge_scores if comp is not None]
278-290: Data alignment bug: slicing doesn't preserve index correspondence after filtering.The filtering on line 279 removes errors from arbitrary positions, but line 280 and 288 use simple slicing (
[:len(valid_scores)]). This causes misalignment betweenvalid_scores,valid_ndcg_scores, andchunks_count.Example:
llm_judge_scores = [(5, 5), ("error", "error"), (7, 7)]nDCG_scores = [0.8, 0.6, 0.9]valid_scores = [(5, 5), (7, 7)](removes index 1)valid_ndcg_scores = nDCG_scores[:2] = [0.8, 0.6]❌ (should be[0.8, 0.9])Suggested fix using index tracking
# Filter out error responses - valid_scores = [(comp, prec) for comp, prec in llm_judge_scores if comp != "error"] - valid_ndcg_scores = nDCG_scores[: len(valid_scores)] # Match the filtered scores + valid_indices = [i for i, (comp, prec) in enumerate(llm_judge_scores) if comp != "error"] + valid_scores = [llm_judge_scores[i] for i in valid_indices] + valid_ndcg_scores = [nDCG_scores[i] for i in valid_indices] eval_results = pd.DataFrame( valid_scores, columns=["completion_evaluation", "precision_evaluation"], ) eval_results["nDCG"] = valid_ndcg_scores chunks_count = [ - len(input_reference["chunks"]) for input_reference in list_response_answer_reference[: len(valid_scores)] + len(list_response_answer_reference[i]["chunks"]) for i in valid_indices ]openrag/components/indexer/loaders/media_loader.py (1)
116-129: Ensure temp language-detection files are always removed.If
_transcribe_chunkreturns an empty string, the early return skips thefinally, leaving*_langdetect.wavon disk.🐛 Suggested fix
- text = await self._transcribe_chunk(tmp_path) - if not text: - return fallback_language # Fallback to English - try: - return langdetect.detect(text) - except Exception as e: - logger.exception("Language detection failed", error=str(e)) - return fallback_language - finally: - await asyncio.to_thread(os.remove, tmp_path) + try: + text = await self._transcribe_chunk(tmp_path) + if not text: + return fallback_language # Fallback to English + return langdetect.detect(text) + except Exception as e: + logger.exception("Language detection failed", error=str(e)) + return fallback_language + finally: + await asyncio.to_thread(os.remove, tmp_path)automatic-evaluation-pipeline/generate_questions.py (2)
127-136: Guard against clusters smaller thann_min.If a cluster has fewer items than
n_min,random.randint(n_min, min(n_max, len(chunks)))will raise aValueErrorbecause the lower bound exceeds the upper bound. Add a check to skip such clusters or clamp the bounds.🛠️ Proposed fix
- for cluster_label, chunks in clusters.items(): # cluster loop - for _ in range(n_questions_per_cluster): - n = random.randint(n_min, min(n_max, len(chunks))) - sampled_chunks = random.sample(chunks, n) - task = question_answer(chunks=sampled_chunks) - tasks.append(task) + for cluster_label, chunks in clusters.items(): # cluster loop + max_n = min(n_max, len(chunks)) + if max_n < n_min: + continue + for _ in range(n_questions_per_cluster): + n = random.randint(n_min, max_n) + sampled_chunks = random.sample(chunks, n) + task = question_answer(chunks=sampled_chunks) + tasks.append(task)
57-67: Avoid creating the semaphore at definition time.While Python 3.12+ has improved asyncio synchronization primitives to avoid strict event loop binding issues, creating
asyncio.Semaphore(10)as a default parameter is still a poor practice. Define it as an optional parameter instead, and instantiate it inside the coroutine when needed. Apply the same fix to bothsummarizeandquestion_answer:🛠️ Proposed fix
-async def summarize(chunk: str, semaphore: asyncio.Semaphore = asyncio.Semaphore(10)) -> str: +async def summarize(chunk: str, semaphore: asyncio.Semaphore | None = None) -> str: + if semaphore is None: + semaphore = asyncio.Semaphore(10) async with semaphore:-async def question_answer(chunks: list[dict], semaphore=asyncio.Semaphore(10)): +async def question_answer(chunks: list[dict], semaphore: asyncio.Semaphore | None = None): + if semaphore is None: + semaphore = asyncio.Semaphore(10) async with semaphore:openrag/components/indexer/loaders/CustomDocLoader.py (1)
35-37: Pre-existing bug: Only the last page content is retained.The loop uses assignment (
s = ...) instead of concatenation (s += ...), so each iteration overwrites the previous content. Only the last page will be included in the final document.🐛 Proposed fix
s = "" for page_num, p in enumerate(pages, start=1): - s = p.page_content.strip() + f"\n[PAGE_{page_num}]\n" + s += p.page_content.strip() + f"\n[PAGE_{page_num}]\n"automatic-evaluation-pipeline/upload_files.py (1)
10-13: Validate APP_URL/APP_PORT before building base_url.
Current code raises a KeyError (APP_URL) or builds...:None(APP_PORT) without a clear message.Proposed fix
-num_port = os.environ.get("APP_PORT") -app_url = os.environ["APP_URL"] -base_url = f"http://{app_url}:{num_port}" # the base url of your running app for instance: 'http://localhost:8080' +num_port = os.environ.get("APP_PORT") +app_url = os.environ.get("APP_URL") +if not app_url: + raise RuntimeError("APP_URL is required (e.g., localhost)") +if not num_port: + raise RuntimeError("APP_PORT is required (e.g., 8080)") +base_url = f"http://{app_url}:{num_port}" # the base url of your running app for instance: 'http://localhost:8080'pyproject.toml (1)
50-60: Remove Ruff from runtime dependencies.
ruff>=0.14.1is listed in[project].dependencies(line 50) and duplicated in[dependency-groups].lint(line 60). As a linting tool, it should only be in the lint group to avoid installing it in production environments.Proposed change
dependencies = [ "fast-langdetect>=1.0.0", - "ruff>=0.14.1", "librosa>=0.11.0", ]openrag/components/indexer/chunker/utils.py (1)
194-210: Unconditional dereference of optionallength_functionparameter.The
length_functionparameter is typed asCallable[[str], int] | Nonewith defaultNone, but is called unconditionally on lines 209-210 (and again on line 221) without aNonecheck. While all current call sites provide this parameter, the type signature and default suggest the function should handle theNonecase.Either make
length_functionrequired by removing the| Noneunion and default value, or add a fallback implementation (e.g.,len) whenNoneis provided.openrag/components/indexer/chunker/chunker.py (1)
141-162: Guard againstllm_config=NonebeforeChatOpenAIandChunkContextualizerinitialization.Line 153 (
ChatOpenAI(**llm_config)) and line 161 (ChunkContextualizer(llm_config)) will raiseTypeErrorif callers instantiateBaseChunkerdirectly with the defaultllm_config=None. WhileChunkerFactory.create_chunker()always providesconfig.vlm, the signature allowsNone, yet the implementation doesn't guard against it.🛠️ Proposed fix to handle None safely
- self.llm = ChatOpenAI(**llm_config) + llm_config = dict(llm_config or {}) + self.llm = ChatOpenAI(**llm_config)Apply the same guard before line 161 when passing to
ChunkContextualizer.openrag/api.py (1)
217-226: Prevent duplicate mounting of openai_router.The router is currently mounted twice when both
WITH_OPENAI_APIandWITH_CHAINLIT_UIare enabled (the default configuration). This duplicates routes and OpenAPI schema entries.Suggested fix
-if WITH_OPENAI_API: - # Mount the openai router - app.include_router(openai_router, prefix="/v1", tags=[Tags.OPENAI]) - -if WITH_CHAINLIT_UI: +if WITH_OPENAI_API or WITH_CHAINLIT_UI: + app.include_router(openai_router, prefix="/v1", tags=[Tags.OPENAI]) + +if WITH_CHAINLIT_UI: # Mount the default front from chainlit.utils import mount_chainlitopenrag/components/indexer/vectordb/vectordb.py (1)
998-1002: Incorrect type annotation forCONNECTORSdictionary.The type hint
dict[BaseVectorDB]is syntactically incorrect. A dictionary requires two type parameters (key and value types). This should bedict[str, type[BaseVectorDB]]to properly annotate a mapping from string names to VectorDB classes.Proposed fix
class ConnectorFactory: - CONNECTORS: dict[BaseVectorDB] = { + CONNECTORS: dict[str, type[BaseVectorDB]] = { "milvus": MilvusDB, # "qdrant": QdrantDB, }tests/test_vectordb.py (1)
4-24: The_build_expr_template_and_params()static method does not exist in the MilvusDB class and must be implemented.These tests call
MilvusDB._build_expr_template_and_params(partition, filter)on lines 7 and 20, but this method is not defined inopenrag/components/indexer/vectordb/vectordb.py. The tests will fail with anAttributeErrorwhen run. Implement this static method in the MilvusDB class to return a templated expression and parameter dictionary as expected by the assertions.openrag/components/indexer/loaders/txt_loader.py (1)
51-54: Incorrect docstring: Class is MarkdownLoader, not TextLoader.The docstring incorrectly states "Loader for plain text files (.txt)" but this is the
MarkdownLoaderclass which handles Markdown files.📝 Suggested fix
class MarkdownLoader(BaseLoader): """ - Loader for plain text files (.txt). + Loader for Markdown files (.md). """openrag/components/indexer/indexer.py (3)
57-66: Mutable default argumentmetadata: dict = {}is dangerous.Using a mutable default argument (empty dict) is a well-known Python pitfall. The same dict instance is reused across all calls, which can cause unexpected mutations to persist. The utility function in
openrag/components/indexer/utils/files.pycorrectly usesmetadata: dict | None = Nonewithmetadata = metadata or {}inside.Additionally,
task_id: str = Noneshould bestr | None = Noneto match the modernized type annotations.Suggested fix
`@ray.method`(concurrency_group="serialize") async def serialize_file( self, path: str, - metadata: dict = {}, - task_id: str = None, + metadata: dict | None = None, + task_id: str | None = None, ): # Serialize + metadata = metadata or {} doc = await serialize_file(task_id, path, metadata=metadata) return doc
68-92: PotentialAttributeErrorwhenuserisNone.The parameter
user: dict | None = NoneallowsNone, but line 91 callsuser.get("id")without a null check. IfuserisNone, this will raise anAttributeError.Suggested fix
await task_state_manager.set_details.remote( task_id, file_id=metadata.get("file_id"), partition=partition, metadata=user_metadata, - user_id=user.get("id"), + user_id=user.get("id") if user else None, )
191-203: Copy-paste error in log message.The error message on line 202 references
update_file_metadatabut this is thecopy_filemethod. This will be misleading when debugging.Suggested fix
- log.error("Vector database is not enabled, but update_file_metadata was called.") + log.error("Vector database is not enabled, but copy_file was called.")
🤖 Fix all issues with AI agents
In `@automatic-evaluation-pipeline/benchmark.py`:
- Around line 219-221: The code inconsistently reads environment variables:
change the access for APP_PORT to match APP_URL (or vice versa) so both are
validated consistently; either use os.environ["APP_PORT"] and
os.environ["APP_URL"] to raise clear KeyError when missing, or introduce a small
helper (e.g., get_env_or_raise) and call it for both keys, then use the
validated values (num_port, num_host) to build openrag_api_base_url, ensuring
the error message mentions the missing variable name for easier debugging.
In `@LINTING.md`:
- Around line 86-111: The pre-commit hook currently collects staged Python files
into the unquoted FILES variable which breaks on paths with spaces; change the
collection to a bash array (e.g., use mapfile -t FILES < <(git diff --cached
--name-only --diff-filter=ACM | grep '\.py$')) and then use the array safely
everywhere by expanding as "${FILES[@]}" when invoking commands (uv run ruff
check/format, git add) and when testing emptiness use: if [ ${`#FILES`[@]} -eq 0
]; then exit 0; fi; update usages of FILES in the hook to the array form to
ensure filenames with spaces are handled correctly.
In `@openrag/api.py`:
- Around line 17-22: The module currently calls
ray.init(dashboard_host="0.0.0.0") unconditionally at import time; remove that
top-level init and instead initialize Ray only when needed by wrapping the call
with a guard such as if not ray.is_initialized(): ray.init(...), and/or move
initialization into your application startup hook (e.g., a function decorated
with app.on_event("startup") or a dedicated init_ray() function invoked from
when the server actually starts). Update references to ray.init in openrag.api
to use ray.is_initialized() before calling and ensure no unconditional
import-time side effects remain.
In `@openrag/app_front.py`:
- Around line 64-65: Remove the hard-coded fallback for
os.environ["CHAINLIT_AUTH_SECRET"] and instead fail fast when authentication is
enabled: check the auth toggle (e.g., AUTH_TOKEN or similar env/config flag) and
if it is set but CHAINLIT_AUTH_SECRET is missing/empty, raise an exception (or
call sys.exit) with a clear message; optionally allow an explicit dev override
(e.g., OPENRAG_DEV=true) to set a non-production default, but do not silently
set a known secret. Ensure you update the code that referenced
os.environ["CHAINLIT_AUTH_SECRET"] to rely on the validated environment value.
In `@openrag/components/indexer/indexer.py`:
- Around line 53-55: The task_id parameter in the chunk method has a default of
None but is annotated as str; update the type annotation to allow None (use
modern union syntax) so it reads task_id: str | None = None in the async def
chunk(self, doc: Document, file_path: str, task_id: str | None = None) signature
(the method uses chunker.split_document and is defined as chunk in indexer.py).
In `@openrag/components/indexer/loaders/pdf_loaders/openai.py`:
- Around line 18-21: The pdf_to_images function opens pdfium.PdfDocument without
closing it; modify it to use a context manager so the PdfDocument is
automatically closed (e.g., run a synchronous helper inside asyncio.to_thread
that does "with pdfium.PdfDocument(pdf_path) as pdf: return
[p.render(scale=scale).to_pil() for p in pdf]"), preserving the async signature
and returning the same list of PIL images; update references in pdf_to_images to
use that context-managed call to avoid file-handle leaks.
🧹 Nitpick comments (11)
openrag/components/ray_utils.py (1)
39-39: LGTM! Formatting change looks good.The consolidation to a single line is consistent with the Ruff formatting applied across the codebase.
As an optional nitpick: the
pendingvariable fromray.waitis unpacked but never used. Consider using_to make this explicit:- ready, pending = await asyncio.to_thread(ray.wait, [future], num_returns=1, timeout=timeout) + ready, _ = await asyncio.to_thread(ray.wait, [future], num_returns=1, timeout=timeout)automatic-evaluation-pipeline/benchmark.py (1)
54-66: Remove ChatGPT link from code comments.External ChatGPT conversation links are ephemeral and not suitable as documentation references. They may become inaccessible and don't provide reliable context for future maintainers.
Suggested change
def compute_inverted_ranks(true_chunk_id, all_retrieved_chunks): - # see link: https://chatgpt.com/share/6813f998-2e88-8002-a472-6af2e9a64b61 + # Computes reciprocal rank: 1/position if found, 0 otherwise (for MRR calculation) key = Falseopenrag/components/indexer/loaders/media_loader.py (2)
53-78: Use Optional typing forlanguageparameters.These params accept
Nonebut are annotated asstr. Align with the PR’s 3.10+ typing style for accuracy.♻️ Suggested tweak
- async def _process_chunk(self, index: int, segment: AudioSegment, wav_path: Path, language: str = None) -> str: + async def _process_chunk( + self, + index: int, + segment: AudioSegment, + wav_path: Path, + language: str | None = None, + ) -> str: @@ - async def _transcribe_chunk(self, wav_path: Path, language: str = None) -> str: + async def _transcribe_chunk(self, wav_path: Path, language: str | None = None) -> str:
194-209: Align override signature withBaseLoaderfor typing.The override drops type hints and uses
dict = None. Keeping the same typed signature improves consistency and type checking.♻️ Suggested tweak
- async def aload_document(self, file_path, metadata: dict = None, save_markdown=False): + async def aload_document( + self, + file_path: str | Path, + metadata: dict | None = None, + save_markdown: bool = False, + ):.github/workflows/lint.yml (2)
7-9: Consider re‑enabling PR linting gates.Keeping
pull_requestdisabled means lint issues land inmain/devbefore being caught. If this is temporary, please add a tracking item and re-enable before release.
28-31: Avoidcurl | shfor uv installation in CI.For supply‑chain hardening, prefer the official setup action (pinned version) or checksum verification rather than executing a remote script directly. Line 28–31.
💡 Example (verify action name/version in uv docs)
- - name: Install uv - run: | - curl -LsSf https://astral.sh/uv/install.sh | sh - echo "$HOME/.local/bin" >> $GITHUB_PATH + - name: Set up uv + uses: astral-sh/setup-uv@v3 + with: + version: "0.x.y"openrag/app_front.py (1)
69-69: Simplify the timeout construction.
httpx.Timeout(timeout=httpx.Timeout(...))is redundant and may behave unexpectedly depending on httpx parsing rules. Prefer a singleTimeoutinstance or a scalar timeout. Line 69 and Line 134.💡 Suggested simplification
- async with httpx.AsyncClient(timeout=httpx.Timeout(timeout=httpx.Timeout(4 * 60.0))) as client: + async with httpx.AsyncClient(timeout=httpx.Timeout(4 * 60.0)) as client:- async with httpx.AsyncClient(timeout=httpx.Timeout(timeout=httpx.Timeout(4 * 60.0))) as client: + async with httpx.AsyncClient(timeout=httpx.Timeout(4 * 60.0)) as client:Also applies to: 134-134
openrag/components/indexer/vectordb/vectordb.py (1)
434-442: Parameter namefiltershadows built-in.The parameter name
filtershadows Python's built-infilter()function. While this is a common pattern in database/query contexts and the type hint change itself is fine, consider renaming tofilter_exprorquery_filterin a future refactor to avoid shadowing.openrag/routers/utils.py (1)
181-193: Good refactor: Centralized file ID validation.The new
is_file_id_validhelper improves maintainability. However, consider reordering the checks invalidate_file_idto check for empty/whitespace-only strings first, as checking forbidden characters on an effectively empty ID is unnecessary work.♻️ Suggested reordering
async def validate_file_id(file_id: str): + if not file_id.strip(): + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="File ID cannot be empty.") if not is_file_id_valid(file_id): raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=f"File ID contains forbidden characters: {', '.join(FORBIDDEN_CHARS_IN_FILE_ID)}", ) - if not file_id.strip(): - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="File ID cannot be empty.") return file_idopenrag/routers/indexer.py (1)
313-313: Type annotation mismatch with runtime behavior.The parameter is annotated as
Any | Nonebutvalidate_metadatadependency returns adict. While this works, the annotation is overly permissive and could bedictto match the actual runtime type.♻️ Consider tightening the type
async def patch_file( partition: str, file_id: str = Depends(validate_file_id), - metadata: Any | None = Depends(validate_metadata), + metadata: dict = Depends(validate_metadata), indexer=Depends(get_indexer),Same applies to
copy_file_between_partitionson line 358.openrag/components/indexer/indexer.py (1)
223-241: Parameterfiltershadows built-in.The parameter name
filtershadows Python's built-infilterfunction. While this is common in search APIs, consider usingfiltersorquery_filterto avoid shadowing.This is a nitpick and can be deferred if the naming convention is intentional across the codebase.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (86)
.github/workflows/api_tests/mock_vllm.py.github/workflows/lint.ymlLINTING.mdautomatic-evaluation-pipeline/benchmark.pyautomatic-evaluation-pipeline/generate_questions.pyautomatic-evaluation-pipeline/upload_files.pyopenrag/api.pyopenrag/app_front.pyopenrag/chainlit_api.pyopenrag/components/indexer/chunker/chunker.pyopenrag/components/indexer/chunker/test_chunking.pyopenrag/components/indexer/chunker/utils.pyopenrag/components/indexer/embeddings/__init__.pyopenrag/components/indexer/embeddings/openai.pyopenrag/components/indexer/indexer.pyopenrag/components/indexer/loaders/CustomDocLoader.pyopenrag/components/indexer/loaders/__init__.pyopenrag/components/indexer/loaders/base.pyopenrag/components/indexer/loaders/doc.pyopenrag/components/indexer/loaders/docx.pyopenrag/components/indexer/loaders/eml_loader.pyopenrag/components/indexer/loaders/image.pyopenrag/components/indexer/loaders/media_loader.pyopenrag/components/indexer/loaders/pdf_loaders/docling.pyopenrag/components/indexer/loaders/pdf_loaders/docling2.pyopenrag/components/indexer/loaders/pdf_loaders/dotsocr.pyopenrag/components/indexer/loaders/pdf_loaders/marker.pyopenrag/components/indexer/loaders/pdf_loaders/openai.pyopenrag/components/indexer/loaders/pdf_loaders/pymupdf.pyopenrag/components/indexer/loaders/pptx_loader.pyopenrag/components/indexer/loaders/serializer.pyopenrag/components/indexer/loaders/test_media_loader.pyopenrag/components/indexer/loaders/txt_loader.pyopenrag/components/indexer/utils/files.pyopenrag/components/indexer/utils/test_text_sanitizer.pyopenrag/components/indexer/vectordb/utils.pyopenrag/components/indexer/vectordb/vectordb.pyopenrag/components/llm.pyopenrag/components/map_reduce.pyopenrag/components/pipeline.pyopenrag/components/prompts/prompts.pyopenrag/components/ray_utils.pyopenrag/components/reranker.pyopenrag/components/retriever.pyopenrag/components/utils.pyopenrag/config/config.pyopenrag/consts.pyopenrag/models/indexer.pyopenrag/models/openai.pyopenrag/routers/actors.pyopenrag/routers/extract.pyopenrag/routers/indexer.pyopenrag/routers/openai.pyopenrag/routers/partition.pyopenrag/routers/queue.pyopenrag/routers/search.pyopenrag/routers/tools.pyopenrag/routers/users.pyopenrag/routers/utils.pyopenrag/scripts/backup.pyopenrag/scripts/embed.pyopenrag/scripts/filter-logs.pyopenrag/scripts/migrations/alembic/env.pyopenrag/scripts/migrations/alembic/versions/4add4d260575_initial_migration.pyopenrag/scripts/migrations/alembic/versions/cd642e4502d8_create_users_memberships_tables.pyopenrag/scripts/restore.pyopenrag/utils/dependencies.pyopenrag/utils/exceptions/base.pyopenrag/utils/exceptions/embeddings.pyopenrag/utils/exceptions/vectordb.pyopenrag/utils/external_resource_errors.pyopenrag/utils/logger.pyopenrag/utils/test_external_resource_errors.pyopenrag/utils/test_logger.pypyproject.tomltests/api_tests/conftest.pytests/api_tests/test_actors.pytests/api_tests/test_extract.pytests/api_tests/test_indexer.pytests/api_tests/test_openai_compat.pytests/api_tests/test_partition.pytests/api_tests/test_queue.pytests/api_tests/test_search.pytests/api_tests/test_users.pytests/test_vectordb.pyutility/data_indexer.py
💤 Files with no reviewable changes (7)
- tests/api_tests/test_actors.py
- tests/api_tests/test_partition.py
- tests/api_tests/test_users.py
- tests/api_tests/test_queue.py
- tests/api_tests/test_extract.py
- openrag/utils/test_logger.py
- openrag/utils/test_external_resource_errors.py
🧰 Additional context used
🧬 Code graph analysis (32)
openrag/scripts/migrations/alembic/versions/cd642e4502d8_create_users_memberships_tables.py (1)
openrag/scripts/migrations/alembic/versions/4add4d260575_initial_migration.py (2)
table_exists(16-20)index_exists(23-28)
openrag/components/indexer/loaders/image.py (1)
openrag/components/indexer/loaders/base.py (1)
BaseLoader(19-151)
openrag/routers/openai.py (3)
openrag/routers/utils.py (1)
get_partition_name(261-287)openrag/components/llm.py (2)
chat_completion(45-87)completions(23-43)openrag/components/pipeline.py (2)
chat_completion(192-202)completions(180-190)
openrag/utils/dependencies.py (1)
openrag/components/indexer/indexer.py (1)
TaskStateManager(271-382)
openrag/components/indexer/loaders/pdf_loaders/pymupdf.py (2)
openrag/components/indexer/loaders/base.py (1)
aload_document(37-43)openrag/components/indexer/loaders/pdf_loaders/openai.py (1)
aload_document(42-69)
openrag/components/indexer/loaders/doc.py (1)
openrag/components/indexer/loaders/base.py (1)
aload_document(37-43)
openrag/components/indexer/loaders/media_loader.py (1)
openrag/components/indexer/loaders/base.py (1)
aload_document(37-43)
openrag/components/indexer/utils/test_text_sanitizer.py (2)
openrag/components/indexer/chunker/test_chunking.py (1)
test_multiline_spacing(178-183)openrag/components/indexer/utils/text_sanitizer.py (1)
clean_markdown_table_spacing(98-127)
tests/api_tests/test_search.py (1)
tests/api_tests/conftest.py (1)
api_client(16-19)
automatic-evaluation-pipeline/generate_questions.py (1)
openrag/components/indexer/indexer.py (1)
chunk(53-55)
openrag/routers/search.py (1)
openrag/components/indexer/indexer.py (1)
asearch(224-241)
openrag/routers/users.py (4)
openrag/components/indexer/vectordb/vectordb.py (2)
get_user(871-873)delete_user(875-882)openrag/utils/dependencies.py (1)
get_vectordb(45-47)openrag/routers/utils.py (1)
require_admin(171-178)openrag/components/indexer/vectordb/utils.py (1)
delete_user(388-395)
openrag/components/indexer/loaders/pdf_loaders/docling2.py (3)
openrag/components/indexer/loaders/base.py (1)
aload_document(37-43)openrag/components/indexer/loaders/pdf_loaders/docling.py (1)
aload_document(61-84)openrag/components/indexer/loaders/pdf_loaders/openai.py (1)
aload_document(42-69)
openrag/scripts/backup.py (3)
openrag/components/indexer/vectordb/utils.py (2)
list_partition_files(165-188)list_partitions(265-269)openrag/components/indexer/vectordb/vectordb.py (4)
list_partition_files(57-58)list_partition_files(718-735)list_partitions(45-46)list_partitions(737-742)openrag/scripts/embed.py (1)
open_output_file(246-272)
openrag/components/indexer/loaders/pdf_loaders/openai.py (1)
openrag/components/indexer/loaders/base.py (1)
aload_document(37-43)
openrag/routers/indexer.py (2)
openrag/components/indexer/indexer.py (3)
add_file(68-141)set_state(284-287)copy_file(192-221)openrag/routers/utils.py (2)
validate_metadata(196-202)require_task_owner(156-168)
openrag/components/map_reduce.py (1)
openrag/components/indexer/indexer.py (1)
chunk(53-55)
openrag/routers/queue.py (3)
openrag/routers/utils.py (1)
require_admin(171-178)openrag/utils/dependencies.py (1)
get_task_state_manager(24-25)openrag/components/indexer/indexer.py (1)
get_all_user_info(363-374)
openrag/scripts/migrations/alembic/versions/4add4d260575_initial_migration.py (1)
openrag/scripts/migrations/alembic/versions/cd642e4502d8_create_users_memberships_tables.py (2)
table_exists(16-20)index_exists(23-28)
openrag/components/indexer/loaders/base.py (1)
openrag/utils/logger.py (1)
get_logger(14-46)
openrag/components/indexer/loaders/txt_loader.py (2)
openrag/components/indexer/loaders/base.py (2)
BaseLoader(19-151)aload_document(37-43)openrag/utils/logger.py (1)
get_logger(14-46)
openrag/utils/exceptions/vectordb.py (1)
openrag/utils/exceptions/base.py (1)
VDBError(35-39)
tests/api_tests/test_openai_compat.py (1)
tests/api_tests/conftest.py (1)
api_client(16-19)
openrag/components/indexer/loaders/__init__.py (1)
openrag/components/indexer/loaders/base.py (1)
BaseLoader(19-151)
openrag/components/indexer/utils/files.py (2)
openrag/components/indexer/indexer.py (1)
serialize_file(58-66)openrag/components/ray_utils.py (1)
call_ray_actor_with_timeout(11-55)
openrag/components/indexer/chunker/chunker.py (2)
openrag/components/indexer/chunker/utils.py (3)
MDElement(20-34)split_md_elements(56-110)get_chunk_page_number(113-148)openrag/components/indexer/embeddings/base.py (1)
BaseEmbedding(5-20)
openrag/components/indexer/vectordb/utils.py (2)
openrag/components/indexer/vectordb/vectordb.py (6)
list_partition_files(57-58)list_partition_files(718-735)create_user(863-869)get_user_by_token(887-888)remove_partition_member(918-921)update_partition_member_role(902-905)openrag/routers/users.py (1)
create_user(82-98)
openrag/components/indexer/indexer.py (3)
openrag/components/indexer/chunker/chunker.py (1)
split_document(288-314)openrag/components/indexer/utils/files.py (1)
serialize_file(73-86)tests/api_tests/test_indexer.py (1)
get_task_id(43-49)
openrag/scripts/restore.py (1)
openrag/scripts/backup.py (1)
load_openrag_config(181-201)
automatic-evaluation-pipeline/benchmark.py (2)
openrag/components/indexer/vectordb/utils.py (2)
to_dict(55-58)to_dict(76-81)openrag/utils/exceptions/base.py (1)
to_dict(20-24)
openrag/scripts/embed.py (2)
openrag/scripts/restore.py (1)
read_vdb_section(122-167)openrag/scripts/backup.py (1)
open_output_file(136-162)
openrag/components/indexer/vectordb/vectordb.py (5)
openrag/components/indexer/vectordb/utils.py (6)
list_partition_files(165-188)file_exists_in_partition(288-294)remove_file_from_partition(233-250)create_user(298-325)list_partition_members(415-428)update_partition_member_role(451-458)openrag/components/indexer/embeddings/base.py (1)
BaseEmbedding(5-20)openrag/components/indexer/embeddings/__init__.py (2)
EmbeddingFactory(9-18)get_embedder(11-18)openrag/utils/exceptions/vectordb.py (3)
VDBConnectionError(4-13)UnexpectedVDBError(109-118)VDBDeleteError(37-46)openrag/routers/users.py (1)
create_user(82-98)
🪛 LanguageTool
LINTING.md
[uncategorized] ~68-~68: The official name of this software platform is spelled with a capital “H”.
Context: ...Integration A GitHub Actions workflow (.github/workflows/lint.yml) automatically runs...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: api-tests
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| num_port = os.environ.get("APP_PORT") | ||
| num_host = os.environ["APP_URL"] | ||
| openrag_api_base_url = f"http://{num_host}:{num_port}" |
There was a problem hiding this comment.
Inconsistent environment variable access patterns.
Line 219 uses .get() (returns None if unset), while line 220 uses direct access (raises KeyError if unset). If APP_PORT is not set, the URL becomes "http://host:None" which will fail silently with confusing errors.
Suggested fix for consistency
- num_port = os.environ.get("APP_PORT")
- num_host = os.environ["APP_URL"]
+ num_port = os.environ["APP_PORT"]
+ num_host = os.environ["APP_URL"]Or use a helper function with clear error messages for both.
📝 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.
| num_port = os.environ.get("APP_PORT") | |
| num_host = os.environ["APP_URL"] | |
| openrag_api_base_url = f"http://{num_host}:{num_port}" | |
| num_port = os.environ["APP_PORT"] | |
| num_host = os.environ["APP_URL"] | |
| openrag_api_base_url = f"http://{num_host}:{num_port}" |
🤖 Prompt for AI Agents
In `@automatic-evaluation-pipeline/benchmark.py` around lines 219 - 221, The code
inconsistently reads environment variables: change the access for APP_PORT to
match APP_URL (or vice versa) so both are validated consistently; either use
os.environ["APP_PORT"] and os.environ["APP_URL"] to raise clear KeyError when
missing, or introduce a small helper (e.g., get_env_or_raise) and call it for
both keys, then use the validated values (num_port, num_host) to build
openrag_api_base_url, ensuring the error message mentions the missing variable
name for easier debugging.
| ```bash | ||
| cat > .git/hooks/pre-commit << 'EOF' | ||
| #!/bin/bash | ||
|
|
||
| # Get list of staged Python files | ||
| FILES=$(git diff --cached --name-only --diff-filter=ACM | grep '\.py$') | ||
|
|
||
| if [ -z "$FILES" ]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Run ruff check (will auto-fix if possible) | ||
| echo "Running ruff check..." | ||
| uv run ruff check $FILES --fix | ||
|
|
||
| # Run ruff format | ||
| echo "Running ruff format..." | ||
| uv run ruff format $FILES | ||
|
|
||
| # Re-add the modified files | ||
| git add $FILES | ||
|
|
||
| # Run ruff check again without --fix to catch remaining errors | ||
| echo "Checking for remaining issues..." | ||
| uv run ruff check $FILES | ||
|
|
There was a problem hiding this comment.
Pre-commit snippet should handle filenames with spaces safely.
Unquoted $FILES will break on paths with spaces. Consider using a bash array via mapfile.
Suggested doc snippet update
-FILES=$(git diff --cached --name-only --diff-filter=ACM | grep '\.py$')
+mapfile -t FILES < <(git diff --cached --name-only --diff-filter=ACM | grep -E '\.py$')
-if [ -z "$FILES" ]; then
+if [ ${`#FILES`[@]} -eq 0 ]; then
exit 0
fi
-uv run ruff check $FILES --fix
+uv run ruff check --fix "${FILES[@]}"
-uv run ruff format $FILES
+uv run ruff format "${FILES[@]}"
-git add $FILES
+git add "${FILES[@]}"
-uv run ruff check $FILES
+uv run ruff check "${FILES[@]}"🤖 Prompt for AI Agents
In `@LINTING.md` around lines 86 - 111, The pre-commit hook currently collects
staged Python files into the unquoted FILES variable which breaks on paths with
spaces; change the collection to a bash array (e.g., use mapfile -t FILES <
<(git diff --cached --name-only --diff-filter=ACM | grep '\.py$')) and then use
the array safely everywhere by expanding as "${FILES[@]}" when invoking commands
(uv run ruff check/format, git add) and when testing emptiness use: if [
${`#FILES`[@]} -eq 0 ]; then exit 0; fi; update usages of FILES in the hook to the
array form to ensure filenames with spaces are handled correctly.
| ray.init(dashboard_host="0.0.0.0") | ||
|
|
||
| # Apply noqa: E402 to ignore "module level import not at top of file" cause ray.init has to be called first | ||
|
|
||
| # flake8: noqa: E402 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -25 openrag/api.py | cat -nRepository: linagora/openrag
Length of output: 1006
🏁 Script executed:
rg -n --type=py '\bray\.init\b' -C2Repository: linagora/openrag
Length of output: 396
🏁 Script executed:
fd -e py -x grep -l "uvicorn\|FastAPI" | head -10Repository: linagora/openrag
Length of output: 102
Avoid unconditional ray.init at module import time.
This causes ray.init to run on every uvicorn reload, which fails or hangs without initialization guard.
🛠️ Guard against re-initialization
-ray.init(dashboard_host="0.0.0.0")
+if not ray.is_initialized():
+ ray.init(dashboard_host="0.0.0.0", ignore_reinit_error=True)📝 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.
| ray.init(dashboard_host="0.0.0.0") | |
| # Apply noqa: E402 to ignore "module level import not at top of file" cause ray.init has to be called first | |
| # flake8: noqa: E402 | |
| if not ray.is_initialized(): | |
| ray.init(dashboard_host="0.0.0.0", ignore_reinit_error=True) | |
| # Apply noqa: E402 to ignore "module level import not at top of file" cause ray.init has to be called first | |
| # flake8: noqa: E402 | |
🤖 Prompt for AI Agents
In `@openrag/api.py` around lines 17 - 22, The module currently calls
ray.init(dashboard_host="0.0.0.0") unconditionally at import time; remove that
top-level init and instead initialize Ray only when needed by wrapping the call
with a guard such as if not ray.is_initialized(): ray.init(...), and/or move
initialization into your application startup hook (e.g., a function decorated
with app.on_event("startup") or a dedicated init_ray() function invoked from
when the server actually starts). Update references to ray.init in openrag.api
to use ray.is_initialized() before calling and ensure no unconditional
import-time side effects remain.
| os.environ["CHAINLIT_AUTH_SECRET"] = "default_secret_for_openrag_ui" # Set default value | ||
|
|
There was a problem hiding this comment.
Default auth secret is an insecure fallback.
Hard‑coding a known CHAINLIT_AUTH_SECRET undermines session integrity if it reaches production. Prefer failing fast (or gating via an explicit dev‑only flag) when AUTH_TOKEN is enabled and the secret is missing. Line 64–65.
💡 Safer default
if AUTH_TOKEN:
if not CHAINLIT_AUTH_SECRET:
- os.environ["CHAINLIT_AUTH_SECRET"] = "default_secret_for_openrag_ui" # Set default value
+ logger.error("CHAINLIT_AUTH_SECRET must be set when AUTH_TOKEN is enabled.")
+ raise RuntimeError("Missing CHAINLIT_AUTH_SECRET")🤖 Prompt for AI Agents
In `@openrag/app_front.py` around lines 64 - 65, Remove the hard-coded fallback
for os.environ["CHAINLIT_AUTH_SECRET"] and instead fail fast when authentication
is enabled: check the auth toggle (e.g., AUTH_TOKEN or similar env/config flag)
and if it is set but CHAINLIT_AUTH_SECRET is missing/empty, raise an exception
(or call sys.exit) with a clear message; optionally allow an explicit dev
override (e.g., OPENRAG_DEV=true) to set a non-production default, but do not
silently set a known secret. Ensure you update the code that referenced
os.environ["CHAINLIT_AUTH_SECRET"] to rely on the validated environment value.
| async def chunk(self, doc: Document, file_path: str, task_id: str = None) -> list[Document]: | ||
| chunks = await self.chunker.split_document(doc, task_id) | ||
| return chunks |
There was a problem hiding this comment.
Inconsistent type annotation: task_id allows None but is typed as str.
The default value is None, but the type hint is str. This should be str | None = None to be consistent with the other modernized type annotations in this file (e.g., lines 71-73).
Suggested fix
- async def chunk(self, doc: Document, file_path: str, task_id: str = None) -> list[Document]:
+ async def chunk(self, doc: Document, file_path: str, task_id: str | None = None) -> list[Document]:📝 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.
| async def chunk(self, doc: Document, file_path: str, task_id: str = None) -> list[Document]: | |
| chunks = await self.chunker.split_document(doc, task_id) | |
| return chunks | |
| async def chunk(self, doc: Document, file_path: str, task_id: str | None = None) -> list[Document]: | |
| chunks = await self.chunker.split_document(doc, task_id) | |
| return chunks |
🤖 Prompt for AI Agents
In `@openrag/components/indexer/indexer.py` around lines 53 - 55, The task_id
parameter in the chunk method has a default of None but is annotated as str;
update the type annotation to allow None (use modern union syntax) so it reads
task_id: str | None = None in the async def chunk(self, doc: Document,
file_path: str, task_id: str | None = None) signature (the method uses
chunker.split_document and is defined as chunk in indexer.py).
| async def pdf_to_images(pdf_path: str, scale: float = 1.0) -> list[Image.Image]: | ||
| pdf: pdfium.PdfDocument = await asyncio.to_thread(pdfium.PdfDocument, pdf_path) | ||
| return [p.render(scale=scale).to_pil() for p in pdf] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
pypdfium2 PdfDocument context manager close cleanup
💡 Result:
Short answer: use the PdfDocument as a context manager — it calls close() on exit and frees PDFium resources (children/pages are closed automatically). If you opened the PDF from a file-like/byte-stream, pass autoclose=True (default for some constructors) or ensure you close the stream yourself so the underlying file handle/buffer is released. Re-closing is a no-op.
Examples and notes
- With statement (recommended): with pdfium.PdfDocument("file.pdf") as pdf: ... — document is closed on exit. [1][2]
- If you supplied a byte/stream, PdfDocument can hold a data-holder and optionally autoclose the stream on finalization; use autoclose or manually close the stream before/during PdfDocument.close(). [2]
- Closing a parent document automatically closes derived children (pages, form envs). Attempting to close an already-closed object is ignored. [1][2]
- For custom file-access callbacks or buffers, keep the buffer/callback objects alive until you call close() to avoid use-after-free (see PDFium notes). [3]
Sources
- pypdfium2 Python API docs. [1]
- pypdfium2 _helpers.document source (context manager, autoclose behavior). [2]
- pypdfium2 GitHub / PDFium lifetime notes (custom file access / buffers). [3]
Use context manager for PdfDocument to prevent file-handle leaks.
PdfDocument requires explicit cleanup; use a context manager (with statement) to automatically close the document and release resources.
♻️ Suggested fix
async def pdf_to_images(pdf_path: str, scale: float = 1.0) -> list[Image.Image]:
- pdf: pdfium.PdfDocument = await asyncio.to_thread(pdfium.PdfDocument, pdf_path)
- return [p.render(scale=scale).to_pil() for p in pdf]
+ def _load_pdf():
+ with pdfium.PdfDocument(pdf_path) as pdf:
+ return [p.render(scale=scale).to_pil() for p in pdf]
+ return await asyncio.to_thread(_load_pdf)🤖 Prompt for AI Agents
In `@openrag/components/indexer/loaders/pdf_loaders/openai.py` around lines 18 -
21, The pdf_to_images function opens pdfium.PdfDocument without closing it;
modify it to use a context manager so the PdfDocument is automatically closed
(e.g., run a synchronous helper inside asyncio.to_thread that does "with
pdfium.PdfDocument(pdf_path) as pdf: return [p.render(scale=scale).to_pil() for
p in pdf]"), preserving the async signature and returning the same list of PIL
images; update references in pdf_to_images to use that context-managed call to
avoid file-handle leaks.
1 participant
🎨 Add Ruff Linting, Modernize Type Annotations, and Improve Code Quality
This PR introduces automated code quality tooling and modernizes the codebase to align with contemporary Python standards and best practices.
🚀 What's New
Automated Linting & CI Integration
.github/workflows/lint.ymlto automatically run Ruff linting and formatting checks on every push tomainanddevbranches, ensuring consistent code quality across all contributionsLINTING.mdguide covering local setup, pre-commit hooks, CI integration, and troubleshooting tipsType Annotation Modernization
Migrated type hints to modern Python 3.10+ syntax throughout the codebase:
Union[str, List[str]]withstr | list[str]Optional[List]withlist | None.github/workflows/api_tests/mock_vllm.pyfor improved readability and maintainability✅ Testing
All existing tests continue to pass. The new linting workflow validates code quality on every push.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.