Refactor/hydra to pydantic - #295
Merged
Merged
Conversation
…ings
Replace Hydra YAML-based configuration with typed Pydantic models.
All config sections are now defined in config/models.py with defaults
and env var loading. Existing code using config.X["key"] or
config.X.get("key") is converted to typed attribute access (config.X.key).
- Add config/models.py, config/mixins.py, config/settings.py
- Remove .hydra_config/ YAML files and hydra-core dependency
- Update Dockerfiles, docker-compose, pytest.ini, cluster.yaml
…cess - Remove shadowed model_config in MimetypesConfig - Use type(self).model_fields instead of self.model_fields to avoid Pydantic v2.11+ deprecation warnings (6 occurrences)
Defaults now live in conf/config.yaml (auditable, diffable) instead of being baked into Python code. The loader reads YAML, merges env var overrides, then validates with Pydantic. - Add conf/config.yaml as single source of truth for defaults - Add config/loader.py with YAML + env merge + Pydantic validation - Strip from_env() methods from models (pure validation schemas now) - Remove _env* helpers from mixins (loader handles env vars) - Update Dockerfiles to copy conf/ directory - Add explicit pyyaml dependency
Tests were passing plain dicts where the Pydantic migration now expects LLMConfig, VLMConfig, and LoaderConfig instances (.model_dump() calls).
- Remove unused llm_params from Settings (inheritance handles shared params) - Restore YAML anchor for DRY llm/vlm defaults, strip anchor keys in loader - Add SEMAPHORE env var as shorthand for both LLM_SEMAPHORE and VLM_SEMAPHORE - Add frozen=True to all config models (prevents accidental mutation) - Add repr=False on all secret fields (api_key, password, api_token) - Add env var name to coercion error messages for easier debugging - Remove stale Hydra references in test skip messages
- Add VDB_PORT env var mapping (fix pre-existing VDB_iPORT typo, keep backward compat) - Use attribute access instead of bracket notation in backup/restore scripts - Use self.image_captioning instead of self.config["loader"]["image_captioning"] in OpenAI loader
Merge config.py, settings.py, and mixins.py into models.py and __init__.py. No behavioral changes.
Pre-existing typo across config, loader, and whisper worker (4 files).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved Hydra-based configs and loader; added centralized YAML at Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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🧪 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 |
coderabbitai
Bot
added
breaking-change
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openrag/components/indexer/embeddings/__init__.py (1)
11-16: 🛠️ Refactor suggestion | 🟠 MajorRaise an
OpenRAGError-derived exception for unsupported providers.This is a configuration error on a core factory path. A raw
ValueErrorhere breaks the repo’s exception contract and makes startup failures harder to normalize; please raiseEmbeddingErroror a dedicatedOpenRAGErrorsubclass instead.As per coding guidelines, "All custom exceptions must inherit from
OpenRAGErrorfromopenrag/utils/exceptions/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/indexer/embeddings/__init__.py` around lines 11 - 16, Replace the raw ValueError in get_embedder with an OpenRAGError-derived exception: import the project exception (e.g., OpenRAGError or the existing EmbeddingError) from openrag.utils.exceptions and raise that instead of ValueError when EMBEDDER_MAPPING.get(provider) is missing; if EmbeddingError does not exist yet, add a small subclass EmbeddingError(OpenRAGError) in the exceptions module and raise EmbeddingError(f"Unsupported embedding provider: {provider}") from get_embedder to preserve the repo's exception contract.openrag/components/indexer/loaders/__init__.py (1)
40-46:⚠️ Potential issue | 🟠 MajorFail fast on unknown loader classes in config.
continueleaves the process running with a partial registry, so a typo inconf/config.yamlonly shows up later as missing support for that extension. This should abort startup with anOpenRAGError-derived config exception instead of silently dropping the mapping.As per coding guidelines, "All custom exceptions must inherit from
OpenRAGErrorfromopenrag/utils/exceptions/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/indexer/loaders/__init__.py` around lines 40 - 46, The loop that builds file loader mappings should fail fast when a configured class name is not found: in the block where you currently check "if cls is None" (iterating file_loaders from config.loader.file_loaders.model_dump() and using discovered), replace the logger.error + continue with raising a configuration exception that inherits from OpenRAGError (import OpenRAGError from openrag.utils.exceptions) and include a clear message containing cls_name and ext so startup aborts on typos instead of leaving a partial registry.
🧹 Nitpick comments (4)
openrag/components/retriever.py (1)
323-333: Typo in variable name:retreiverConfig→retrieverConfigThe variable name has a typo ("retreiver" instead of "retriever"). This appears twice at lines 324 and 326.
✏️ Suggested fix
`@classmethod` def create_retriever(cls, config) -> ABCRetriever: - retreiverConfig = config.retriever.model_dump() + retrieverConfig = config.retriever.model_dump() - retriever_type = retreiverConfig.pop("type") + retriever_type = retrieverConfig.pop("type") retriever_cls = RetrieverFactory.RETRIEVERS.get(retriever_type, None) if retriever_cls is None: raise ValueError(f"Unknown retriever type: {retriever_type}") - retreiverConfig["llm"] = ChatOpenAI(**config.llm.model_dump()) - return retriever_cls(**retreiverConfig) + retrieverConfig["llm"] = ChatOpenAI(**config.llm.model_dump()) + return retriever_cls(**retrieverConfig)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/retriever.py` around lines 323 - 333, In create_retriever (class method) rename the misspelled local variable retreiverConfig to retrieverConfig everywhere it’s used: the assignment from config.retriever.model_dump(), the pop("type") call, and when building the "llm" entry before passing kwargs to retriever_cls; ensure all references (including the later retreiverConfig["llm"] and return retriever_cls(**retreiverConfig)) are updated to retrieverConfig so the correct variable is passed into retriever_cls.openrag/config/loader.py (2)
193-204:ValueErrordoesn't inherit fromOpenRAGError.Per coding guidelines, custom exceptions should inherit from
OpenRAGError. Consider either wrapping this in a config-specific exception or using the existing exception hierarchy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/config/loader.py` around lines 193 - 204, The _coerce function currently raises a plain ValueError on parse failures; change this to raise an OpenRAGError (or a config-specific subclass of OpenRAGError) so the exception fits the project's hierarchy. Update the except block in _coerce to construct and raise the chosen OpenRAGError subclass with the same message (including env_var, expected type and value), and ensure any imports or exception class definitions are added/adjusted so _coerce references the proper OpenRAGError-derived type.
164-170: Consider logging when YAML config file is missing.Silent fallback to empty dict may obscure misconfiguration issues in production. A debug/warning log would help operators diagnose missing config scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/config/loader.py` around lines 164 - 170, The _load_yaml function silently returns an empty dict when the provided Path does not exist; modify it to emit a log message (at debug or warning level) when path.exists() is False so missing config files are visible in logs. Locate the _load_yaml function and use the module logger (or create one if absent) to log the missing path before returning {} and keep behavior of yaml.safe_load and returning data or {} unchanged.openrag/config/models.py (1)
171-178:PathsConfigoverrides inheritedmodel_config— consider merging instead.This works but creates a maintenance burden: if
ConfigMixin.model_configadds new settings, they won't apply toPathsConfig. Consider using a merged config pattern.♻️ Alternative pattern using ChainMap or explicit merge
class PathsConfig(ConfigMixin): prompts_dir: Path = Path("../prompts/example1") data_dir: Path = Path("../data") db_dir: Path = Path("/app/db") log_dir: Path = Path("/app/logs") - model_config = {"frozen": True, "arbitrary_types_allowed": True} + model_config = {**ConfigMixin.model_config, "arbitrary_types_allowed": True}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/config/models.py` around lines 171 - 178, PathsConfig currently replaces ConfigMixin.model_config outright which prevents future parent config changes from applying; update PathsConfig to merge its overrides into the inherited model_config instead of replacing it — e.g., retrieve the parent model_config from ConfigMixin (or via getattr(self.__class__.__bases__[0], "model_config")) and combine it with the overrides ("frozen": True, "arbitrary_types_allowed": True) so PathsConfig merges with the base config rather than overriding it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@conf/config.yaml`:
- Around line 61-66: The rdb block currently hardcodes a real-looking password
value under the key password (in the rdb configuration), violating the "NEVER be
set here" policy; change the password value to an empty string or an explicit
placeholder (e.g., "" or "<SET_VIA_ENV>") so no real secret appears in the
config, and ensure any runtime code that reads rdb.password falls back to
environment variables or a secret manager when empty/placeholder.
In `@Dockerfile.ray`:
- Line 53: The Dockerfile contains a stray RUN apt install -y instruction that
has no packages and will fail; remove this line or replace it with the intended
package installation (e.g., change the RUN apt install -y entry to include the
specific packages you need or delete the RUN apt install -y line entirely so the
Docker build does not run a no-op/erroneous apt install command).
In `@openrag/components/indexer/loaders/eml_loader.py`:
- Line 191: Replace direct config access with the instance flag on the loader:
in EmlLoader change checks that use self.config.loader.image_captioning to use
self.image_captioning (the attribute initialized in BaseLoader.__init__) before
performing image captioning; update every occurrence (e.g., the conditional
guarding image captioning logic in the methods referencing image captioning) so
the loader follows the same pattern as docx/pptx/marker/docling and respects the
instance-level toggle.
In `@openrag/components/indexer/loaders/pdf_loaders/docling2.py`:
- Line 112: Replace the direct config check
"self.config.loader.image_captioning" with the instance attribute
"self.image_captioning" (provided by BaseLoader __init__) to follow loader
conventions; locate the conditional in docling2.py (the "if
self.config.loader.image_captioning:" check) and change it to use
"self.image_captioning" before performing image captioning operations so the
loader respects the common BaseLoader flag.
In `@openrag/components/indexer/vectordb/vectordb.py`:
- Around line 1211-1214: Replace the raw ValueError raised in the connector
resolution block with a VDBError (or create a new class deriving from
OpenRAGError if VDBError does not yet exist) so unsupported connector errors
follow the repo's error taxonomy; specifically update the code that looks up
ConnectorFactory.CONNECTORS.get(name) (variable name) to raise
VDBError(f"VECTORDB '{name}' is not supported.") and ensure VDBError inherits
from OpenRAGError in openrag/utils/exceptions (or import the existing VDBError)
so initialization failures are handled consistently.
In `@openrag/config/__init__.py`:
- Around line 22-34: The load_config function currently ignores the config_path
when overrides is None and always returns the cached singleton from
get_settings; update load_config to treat a non-None config_path as a cache
bypass: if config_path is provided (or overrides), call the loader function
(from .loader import load_config as _load) with conf_dir=config_path and any
overrides and return its result instead of get_settings(), ensuring callers who
pass config_path actually load from that path; keep the existing behavior of
returning get_settings() only when both config_path and overrides are None and
update the docstring to reflect that non-None config_path will bypass the cache.
In `@openrag/config/loader.py`:
- Around line 236-241: The loader currently resolves only ("prompts_dir",
"data_dir", "log_dir") but omits "db_dir", so relative DB_DIR values aren't
converted to absolute paths; update the resolution logic in
openrag/config/loader.py by including "db_dir" in the keys iterated (or
separately resolve paths["db_dir"]) so that paths = data.get("paths", {}) will
have paths["db_dir"] = str(Path(paths["db_dir"]).resolve()) when present,
ensuring consistent absolute path handling for DB_DIR alongside prompts_dir,
data_dir, and log_dir.
- Line 76: The env var tuple ("IMAGE_CAPTIONING_URL",
"loader.image_captioning_url", bool) is mistyped; align the types between this
tuple and the LoaderConfig model by changing the third element from bool to str
and updating LoaderConfig (models.py) to use a str for
loader.image_captioning_url, or if the intent is a feature flag instead, rename
the env var and model field to IMAGE_CAPTIONING_URL_ENABLED /
loader.image_captioning_url_enabled and keep the type bool; make the names and
types consistent across loader.py and LoaderConfig.
- Around line 214-220: The code directly converts the SEMAPHORE env var using
int(semaphore) which will raise an unclear ValueError on bad input; replace the
raw int() coercion with the project's _coerce helper (or wrap the conversion to
raise a contextual error) when computing sem_value so failures include the env
var name and expected type; update the block that reads semaphore, sem_value,
sem = data.setdefault("semaphore", {}), sem.setdefault("llm_semaphore",
sem_value) and sem.setdefault("vlm_semaphore", sem_value) to obtain sem_value
via _coerce("SEMAPHORE", int, semaphore) (or equivalent) so invalid values
produce clear, contextual error messages.
In `@openrag/scripts/backup.py`:
- Line 201: The function currently returns Pydantic model objects (config.rdb,
config.vectordb) but downstream code expects dict-like access (e.g.,
rdb['host'], vdb['port']), causing TypeError; fix by returning plain dicts
instead of models here: replace the returned values with their serialized dict
form (use model_dump() on config.rdb and config.vectordb) so callers can
continue using bracket indexing, or alternatively update all downstream usages
to attribute access (e.g., rdb.host, vectordb.port) — pick one approach and
apply it consistently (refer to symbols config.rdb, config.vectordb and the
downstream accesses that use rdb['...'] / vdb['...']).
In `@openrag/scripts/restore.py`:
- Line 228: The function currently returns Pydantic model instances (config.rdb,
config.vectordb) but downstream code expects plain dicts (uses rdb['host'],
vdb['collection_name']); change the return to return the serialized dicts by
calling model_dump() on each (e.g., return config.rdb.model_dump(),
config.vectordb.model_dump()) so callers receive dictionaries compatible with
the dict-style access used in the rest of the code (same fix as applied in
backup.py).
---
Outside diff comments:
In `@openrag/components/indexer/embeddings/__init__.py`:
- Around line 11-16: Replace the raw ValueError in get_embedder with an
OpenRAGError-derived exception: import the project exception (e.g., OpenRAGError
or the existing EmbeddingError) from openrag.utils.exceptions and raise that
instead of ValueError when EMBEDDER_MAPPING.get(provider) is missing; if
EmbeddingError does not exist yet, add a small subclass
EmbeddingError(OpenRAGError) in the exceptions module and raise
EmbeddingError(f"Unsupported embedding provider: {provider}") from get_embedder
to preserve the repo's exception contract.
In `@openrag/components/indexer/loaders/__init__.py`:
- Around line 40-46: The loop that builds file loader mappings should fail fast
when a configured class name is not found: in the block where you currently
check "if cls is None" (iterating file_loaders from
config.loader.file_loaders.model_dump() and using discovered), replace the
logger.error + continue with raising a configuration exception that inherits
from OpenRAGError (import OpenRAGError from openrag.utils.exceptions) and
include a clear message containing cls_name and ext so startup aborts on typos
instead of leaving a partial registry.
---
Nitpick comments:
In `@openrag/components/retriever.py`:
- Around line 323-333: In create_retriever (class method) rename the misspelled
local variable retreiverConfig to retrieverConfig everywhere it’s used: the
assignment from config.retriever.model_dump(), the pop("type") call, and when
building the "llm" entry before passing kwargs to retriever_cls; ensure all
references (including the later retreiverConfig["llm"] and return
retriever_cls(**retreiverConfig)) are updated to retrieverConfig so the correct
variable is passed into retriever_cls.
In `@openrag/config/loader.py`:
- Around line 193-204: The _coerce function currently raises a plain ValueError
on parse failures; change this to raise an OpenRAGError (or a config-specific
subclass of OpenRAGError) so the exception fits the project's hierarchy. Update
the except block in _coerce to construct and raise the chosen OpenRAGError
subclass with the same message (including env_var, expected type and value), and
ensure any imports or exception class definitions are added/adjusted so _coerce
references the proper OpenRAGError-derived type.
- Around line 164-170: The _load_yaml function silently returns an empty dict
when the provided Path does not exist; modify it to emit a log message (at debug
or warning level) when path.exists() is False so missing config files are
visible in logs. Locate the _load_yaml function and use the module logger (or
create one if absent) to log the missing path before returning {} and keep
behavior of yaml.safe_load and returning data or {} unchanged.
In `@openrag/config/models.py`:
- Around line 171-178: PathsConfig currently replaces ConfigMixin.model_config
outright which prevents future parent config changes from applying; update
PathsConfig to merge its overrides into the inherited model_config instead of
replacing it — e.g., retrieve the parent model_config from ConfigMixin (or via
getattr(self.__class__.__bases__[0], "model_config")) and combine it with the
overrides ("frozen": True, "arbitrary_types_allowed": True) so PathsConfig
merges with the base config rather than overriding it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 163c5344-9858-4a22-b2ca-e0425f89f9bf
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (60)
.hydra_config/chunker/base.yaml.hydra_config/chunker/recursive_splitter.yaml.hydra_config/config.yaml.hydra_config/rag/ChatBotRag.yaml.hydra_config/rag/SimpleRag.yaml.hydra_config/rag/base.yaml.hydra_config/retriever/base.yaml.hydra_config/retriever/hyde.yaml.hydra_config/retriever/multiQuery.yaml.hydra_config/retriever/single.yaml.hydra_config/websearch/base.yaml.hydra_config/websearch/staan.yamlDockerfileDockerfile.raycluster.yamlconf/config.yamldocker-compose.yamldocs/assets/compose_linux_gpu.yamlopenrag/components/indexer/chunker/chunker.pyopenrag/components/indexer/embeddings/__init__.pyopenrag/components/indexer/embeddings/openai.pyopenrag/components/indexer/indexer.pyopenrag/components/indexer/loaders/__init__.pyopenrag/components/indexer/loaders/audio/local_whisper.pyopenrag/components/indexer/loaders/base.pyopenrag/components/indexer/loaders/eml_loader.pyopenrag/components/indexer/loaders/pdf_loaders/docling2.pyopenrag/components/indexer/loaders/pdf_loaders/marker.pyopenrag/components/indexer/loaders/pdf_loaders/openai.pyopenrag/components/indexer/loaders/serializer.pyopenrag/components/indexer/loaders/test_doc_loader.pyopenrag/components/indexer/utils/files.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/reranker.pyopenrag/components/retriever.pyopenrag/components/test_llm.pyopenrag/components/utils.pyopenrag/components/websearch/__init__.pyopenrag/config/__init__.pyopenrag/config/config.pyopenrag/config/loader.pyopenrag/config/models.pyopenrag/models/openai.pyopenrag/routers/indexer.pyopenrag/routers/openai.pyopenrag/routers/search.pyopenrag/routers/utils.pyopenrag/routers/workspaces.pyopenrag/scripts/backup.pyopenrag/scripts/restore.pyopenrag/tests/test_relationships_integration.pyopenrag/utils/dependencies.pypyproject.tomlpytest.iniquick_start/docker-compose.yaml
💤 Files with no reviewable changes (18)
- .hydra_config/retriever/hyde.yaml
- .hydra_config/rag/SimpleRag.yaml
- .hydra_config/rag/ChatBotRag.yaml
- .hydra_config/retriever/multiQuery.yaml
- .hydra_config/retriever/single.yaml
- .hydra_config/websearch/base.yaml
- .hydra_config/config.yaml
- .hydra_config/rag/base.yaml
- cluster.yaml
- .hydra_config/websearch/staan.yaml
- docker-compose.yaml
- docs/assets/compose_linux_gpu.yaml
- pytest.ini
- quick_start/docker-compose.yaml
- .hydra_config/chunker/recursive_splitter.yaml
- .hydra_config/chunker/base.yaml
- .hydra_config/retriever/base.yaml
- openrag/config/config.py
- Use self.image_captioning in eml_loader and docling2 (BaseLoader pattern) - Use attribute access instead of bracket notation in backup/restore scripts - Remove hardcoded rdb password default, use empty string like other secrets - Include db_dir in path resolution (was missing) - Use _coerce() for SEMAPHORE env var (proper error on invalid value) - Bypass cache when config_path is provided in load_config() - Remove no-op apt install -y from Dockerfile.ray - Fix pre-existing ruff formatting in eml_loader.py and prompts.py
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/config/loader.py`:
- Around line 236-240: Move the path normalization out of _apply_env_overrides()
and into load_config() after overrides and _deep_merge() have been applied: stop
resolving paths in _apply_env_overrides() and instead, in load_config(), take
the merged data's "paths" dict and for each key
("prompts_dir","data_dir","db_dir","log_dir") normalize with
Path(paths[key]).resolve() relative to conf_dir (i.e., join conf_dir with the
value when it is a relative path) so overrides are honored and resolution is
based on the configuration directory; apply the same relocation/fix for the
identical normalization block currently present around lines 277-279.
- Around line 41-42: The env-override tuple order causes the legacy key
("VDB_iPORT", "vectordb.port", int) to be applied after the canonical
("VDB_PORT", "vectordb.port", int") in _apply_env_overrides(), so swap their
order in the overrides list so the legacy tuple is listed first and the
canonical ("VDB_PORT", "vectordb.port", int) appears after it; this ensures
_apply_env_overrides() applies the canonical VDB_PORT last and therefore wins
when both env vars are present.
- Around line 193-203: In _coerce, stop silently treating unknown boolean
strings as False: when target_type is bool, only accept explicit true tokens
("true","1","yes") and explicit false tokens ("false","0","no")
case-insensitively; if the value does not match any of these tokens, raise
ValueError naming the env_var and that a boolean was expected (use
target_type.__name__ in the message) so typos like "treu" fail fast; update the
bool-branch in _coerce accordingly.
In `@openrag/config/models.py`:
- Around line 316-318: Add input validation in the config model for docling
settings: ensure docling_pool_size and docling_max_tasks_per_worker are positive
integers (>0) and docling_num_gpus is non-negative (>=0). Implement this as
Pydantic validators (or a root_validator) on the model containing
docling_num_gpus, docling_pool_size, and docling_max_tasks_per_worker so invalid
values raise a ValueError with clear messages referencing the field names; this
prevents negative/zero pool or task values and negative GPU counts from being
accepted at startup.
- Around line 288-296: to_dict currently only serializes declared model_fields
and thus drops any extra MIME mappings provided via YAML; update to_dict (in the
same class where model_config is defined) to also include instance.extra
mappings from self.model_extra (pydantic v2) by merging them into the result
after populating declared fields from type(self).model_fields, ensuring declared
fields keep priority and only add entries from self.model_extra for keys not
already present, and convert values to str to match the return type dict[str,
str].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22398ff6-0c5e-4bfe-9638-a5fbd354729d
📒 Files selected for processing (10)
Dockerfile.rayconf/config.yamlopenrag/components/indexer/loaders/eml_loader.pyopenrag/components/indexer/loaders/pdf_loaders/docling2.pyopenrag/components/prompts/prompts.pyopenrag/config/__init__.pyopenrag/config/loader.pyopenrag/config/models.pyopenrag/scripts/backup.pyopenrag/scripts/restore.py
✅ Files skipped from review due to trivial changes (3)
- openrag/scripts/backup.py
- openrag/scripts/restore.py
- conf/config.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- Dockerfile.ray
- openrag/components/prompts/prompts.py
- openrag/config/init.py
MimetypesConfig.to_dict() only iterated model_fields, silently dropping any custom MIME types added via config.yaml (stored in __pydantic_extra__). Also replace relative imports in config package with absolute imports per project convention.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openrag/config/__init__.py (1)
11-11: Use absolute imports withopenrag.prefix as per coding guidelines.The imports use
config.modelsandconfig.loaderinstead of the fully qualifiedopenrag.config.modelsandopenrag.config.loader. While these work whenopenrag/is inPYTHONPATH, they don't follow the project's coding guideline for absolute imports.♻️ Suggested fix
-from config.models import Settings +from openrag.config.models import Settings `@lru_cache` def get_settings() -> Settings: """Cached singleton — one Settings instance per process.""" - from config.loader import load_config as _load + from openrag.config.loader import load_config as _load return _load() def load_config(config_path=None, overrides=None) -> Settings: ... if overrides or config_path: - from config.loader import load_config as _load + from openrag.config.loader import load_config as _load return _load(conf_dir=config_path, overrides=overrides) return get_settings()As per coding guidelines: "Use absolute imports from the
openrag/directory (which is the Python path root)".Also applies to: 17-17, 31-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/config/__init__.py` at line 11, Replace relative/short module imports in this package init with fully qualified absolute imports using the openrag. prefix: update any occurrences of config.models and config.loader to openrag.config.models and openrag.config.loader (the imports in __init__.py and the similar occurrences referenced at lines 17 and 31). Ensure the import names (Settings, load_settings, or whatever symbols are currently imported) remain unchanged so existing references keep working.openrag/config/loader.py (1)
3-11: Fix import block sorting per Ruff I001.Static analysis indicates the import block is unsorted/unformatted.
♻️ Suggested fix
from __future__ import annotations import os from pathlib import Path from typing import Any import yaml -from config.models import Settings +from openrag.config.models import SettingsThis also addresses the coding guideline for absolute imports from the
openrag/directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/config/loader.py` around lines 3 - 11, The import block in loader.py is unsorted; reorder imports into the standard groups (future, standard library, third-party, local application) and sort them alphabetically within each group (e.g., keep "from __future__ import annotations" first, then standard libs like os and pathlib.Path, then typing and third-party yaml, then local "from config.models import Settings"); you can run isort/ruff --fix or manually reorder the lines to follow Ruff I001 and the project's absolute-import guideline so that symbols like Path, Any, yaml, and Settings are imported in the correct grouped order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/config/loader.py`:
- Around line 160-162: The _AUDIO_EXTENSIONS tuple is missing "wav", causing
AUDIOLOADER to skip .wav files in the extension loop; add "wav" to the
_AUDIO_EXTENSIONS tuple and remove the redundant direct assignment to
loader.file_loaders.wav (the one that sets it from AUDIOLOADER) so the loop that
maps AUDIOLOADER to FileLoadersConfig handles wav consistently with other audio
extensions; reference symbols: _AUDIO_EXTENSIONS, AUDIOLOADER,
loader.file_loaders.wav, FileLoadersConfig.
---
Nitpick comments:
In `@openrag/config/__init__.py`:
- Line 11: Replace relative/short module imports in this package init with fully
qualified absolute imports using the openrag. prefix: update any occurrences of
config.models and config.loader to openrag.config.models and
openrag.config.loader (the imports in __init__.py and the similar occurrences
referenced at lines 17 and 31). Ensure the import names (Settings,
load_settings, or whatever symbols are currently imported) remain unchanged so
existing references keep working.
In `@openrag/config/loader.py`:
- Around line 3-11: The import block in loader.py is unsorted; reorder imports
into the standard groups (future, standard library, third-party, local
application) and sort them alphabetically within each group (e.g., keep "from
__future__ import annotations" first, then standard libs like os and
pathlib.Path, then typing and third-party yaml, then local "from config.models
import Settings"); you can run isort/ruff --fix or manually reorder the lines to
follow Ruff I001 and the project's absolute-import guideline so that symbols
like Path, Any, yaml, and Settings are imported in the correct grouped order.
🪄 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
Run ID: 10e509af-5524-4774-8429-82fdde018ede
📒 Files selected for processing (3)
openrag/config/__init__.pyopenrag/config/loader.pyopenrag/config/models.py
- Swap VDB_iPORT/VDB_PORT order so canonical name takes precedence - Reject invalid boolean env var values instead of silently coercing to False - Move path resolution after overrides merge so programmatic overrides are honored - Log warning when config YAML file is missing - Merge PathsConfig.model_config with parent instead of overriding - Add Pydantic validators (ge=) for docling pool settings - Fix retreiverConfig typo in RetrieverFactory.create_retriever
The project's ruff config has known-first-party = ["openrag"], so `from config.models` is classified as third-party. Relative imports within the config package are the correct pattern here.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openrag/config/loader.py (2)
14-14: Use Loguru instead of standard logging.The codebase uses Loguru for structured logging. Replace the standard
loggingimport with the project's logger utility.🛠️ Suggested fix
-import logging +from utils.logger import get_logger import os from pathlib import Path from typing import Any import yaml from .models import Settings -logger = logging.getLogger(__name__) +logger = get_logger()As per coding guidelines: "Use Loguru with structured logging via
get_logger()fromutils.logger".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/config/loader.py` at line 14, Replace the standard logging usage in loader.py: remove the logging import and the line creating logger = logging.getLogger(__name__), import get_logger from utils.logger, and instantiate logger = get_logger(__name__) so the module uses the project's Loguru-based logger; ensure any existing calls use the same logger variable name and update imports accordingly.
172-173: Consider specifying encoding for portability.Explicit
encoding="utf-8"ensures consistent behavior across platforms with different default encodings.🔧 Suggested fix
- with open(path) as f: + with open(path, encoding="utf-8") as f: data = yaml.safe_load(f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/config/loader.py` around lines 172 - 173, The file opens YAML with plain open(path) which relies on system default encoding; update the open call to explicitly specify encoding="utf-8" (e.g., change the with open(path) as f: to with open(path, encoding="utf-8") as f:) so that yaml.safe_load(f) reads files consistently across platforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openrag/config/loader.py`:
- Line 14: Replace the standard logging usage in loader.py: remove the logging
import and the line creating logger = logging.getLogger(__name__), import
get_logger from utils.logger, and instantiate logger = get_logger(__name__) so
the module uses the project's Loguru-based logger; ensure any existing calls use
the same logger variable name and update imports accordingly.
- Around line 172-173: The file opens YAML with plain open(path) which relies on
system default encoding; update the open call to explicitly specify
encoding="utf-8" (e.g., change the with open(path) as f: to with open(path,
encoding="utf-8") as f:) so that yaml.safe_load(f) reads files consistently
across platforms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4df8370b-f444-488b-a644-1367e05b54a9
📒 Files selected for processing (4)
openrag/components/retriever.pyopenrag/config/__init__.pyopenrag/config/loader.pyopenrag/config/models.py
🚧 Files skipped from review as they are similar to previous changes (2)
- openrag/config/init.py
- openrag/config/models.py
…bSearchConfig - Replace flat RetrieverConfig with _BaseRetrieverConfig + Single/MultiQuery/Hyde variants - Replace flat WebSearchConfig with _BaseWebSearchConfig + StaanWebSearchConfig - Pydantic discriminates on `type` / `provider` fields at parse time - Type-specific fields (k_queries, combine, lang, base_url) are now scoped to their class
Ahmath-Gadji
force-pushed
the
refactor/hydra-to-pydantic
branch
from
April 1, 2026 13:54
f30c985 to
b75089b
Compare
Ahmath-Gadji
approved these changes
Apr 1, 2026
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — I’ve added a few extra commits that goes with the branch.
This was referenced May 29, 2026
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
3 participants
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.
Summary
conf/config.yamlas single source of truth with readable defaults, easy to review in code reviews.envonlyhydra-coreandomegaconfdependenciesrepr()output (api_key, password, api_token)VDB_iPORTtypo (addVDB_PORT, keep backward compat)whisper_concurencytypo across 4 filesConfig package structure
conf/config.yaml # YAML defaults (no secrets)
openrag/config/
├── init.py # Public API: load_config(), Settings, get_settings()
├── loader.py # YAML parsing, env var overrides, type coercion
└── models.py # ConfigMixin + all Pydantic models + Settings
Test plan
ruff check+ruff format)Summary by CodeRabbit
Configuration
Chores