Use Pydantic classes for configuration settings#325
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughThis PR refactors the configuration system from TOML-loading helper functions with module-level eager initialization to Pydantic-based models with lazy initialization and caching. The change introduces frozen Pydantic models ( Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (89.91%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #325 +/- ##
==========================================
- Coverage 93.87% 93.60% -0.27%
==========================================
Files 69 69
Lines 3248 3269 +21
Branches 227 231 +4
==========================================
+ Hits 3049 3060 +11
- Misses 139 147 +8
- Partials 60 62 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The
get_configfunction is both cached and backed by a mutable global_config; this means callingload_set_configurationafterget_confighas been used will not update the cached result—either drop the@functools.cacheor refactor so configuration updates are reflected correctly. - In
database/setup.py::_create_engine, the new URL.create call no longer includes the database port, so non-default ports inDatabaseConfigurationwill be ignored; passport=db_config.portto preserve existing behavior. - The type hints for
_load_database_configuration(dict[str, dict[str, str]]) do not match actual usage where values likeportare integers; consider updating these annotations to reflect the real types to avoid confusion and improve tooling support.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `get_config` function is both cached and backed by a mutable global `_config`; this means calling `load_set_configuration` after `get_config` has been used will not update the cached result—either drop the `@functools.cache` or refactor so configuration updates are reflected correctly.
- In `database/setup.py::_create_engine`, the new URL.create call no longer includes the database port, so non-default ports in `DatabaseConfiguration` will be ignored; pass `port=db_config.port` to preserve existing behavior.
- The type hints for `_load_database_configuration` (`dict[str, dict[str, str]]`) do not match actual usage where values like `port` are integers; consider updating these annotations to reflect the real types to avoid confusion and improve tooling support.
## Individual Comments
### Comment 1
<location path="src/database/setup.py" line_range="10-11" />
<code_context>
- echo = db_config.pop("echo", False)
-
- db_url = URL.create(**db_config)
+def _create_engine(db_config: DatabaseConfiguration) -> AsyncEngine:
+ db_url = URL.create(
+ drivername=db_config.drivername,
+ username=db_config.username,
</code_context>
<issue_to_address>
**issue (bug_risk):** Database port from configuration is ignored when constructing the SQLAlchemy URL.
In `_create_engine`, `URL.create` now omits `db_config.port`, so non-default ports from TOML/env are ignored and the driver’s default is used instead. Please pass `port=db_config.port` into `URL.create(...)` to maintain previous behavior and avoid connection failures when the DB is not on the default port.
</issue_to_address>
### Comment 2
<location path="src/routers/openml/tasks.py" line_range="168-169" />
<code_context>
# we don't need to be careful to avoid losing typedness
template = template.replace("[TASK:id]", str(task.task_id))
- server_url = config.load_routing_configuration()["server_url"]
+ url = get_config().routing.server_url
+ server_url = f"{url.scheme}://{url.host}:{url.port}/"
return template.replace("[CONSTANT:base_url]", server_url)
</code_context>
<issue_to_address>
**issue (bug_risk):** Reconstructing `server_url` from components can produce invalid URLs and drops any path prefix.
The previous code used the configured `server_url` string as-is, but the new code rebuilds it as `f"{url.scheme}://{url.host}:{url.port}/"` from an `AnyUrl`, which changes behavior:
* If no port is configured, `url.port` is `None`, giving URLs like `http://example.com:None/`.
* Any path component (e.g. a reverse-proxy prefix) is dropped.
Instead, consider either using the string form directly (e.g. `server_url = str(url).rstrip('/') + '/'`) or reconstructing while handling default ports and preserving `url.path`.
</issue_to_address>
### Comment 3
<location path="src/routers/mldcat_ap/dataset.py" line_range="34-36" />
<code_context>
router = APIRouter(prefix="/mldcat_ap", tags=["MLDCAT-AP"])
-_configuration = config.load_configuration()
+_routing_configuration = config.get_config().routing
_server_url = (
- f"{_configuration['arff_base_url']}{_configuration['fastapi']['root_path']}{router.prefix}"
+ f"{_routing_configuration.server_url}{_routing_configuration.root_path}{router.prefix}"
)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Concatenating `server_url`, `root_path`, and `router.prefix` may produce awkward or duplicated slashes.
This construction can produce URLs with double slashes or depend on `router.prefix` always starting with `/` (e.g. `"http://host/"` + `"/api"` → `"http://host//api"`).
Consider normalizing these segments—e.g., explicitly stripping/adding slashes or using a joining helper (`urllib.parse.urljoin`, `posixpath.join`-style)—so there is exactly one `/` between components for any configuration.
</issue_to_address>
### Comment 4
<location path="tests/config_test.py" line_range="7" />
<code_context>
- expected = {"other": {1: 1}}
- output = _apply_defaults_to_siblings(input_)
- assert output == expected
+def test_load_configuration_adds_environment_variables() -> None:
+ _db_alias = "openml"
</code_context>
<issue_to_address>
**suggestion (testing):** Extend this test to also cover password overrides and multiple database aliases.
Since this helper also injects passwords from env vars and supports arbitrary aliases, please (1) assert both the default and env override for the password (e.g. `OPENML_DATABASES_OPENML_PASSWORD`), and (2) extend `_fake_config` with a second alias (e.g. `expdb`) and verify its `EXPDB` env vars are applied. This will meaningfully increase coverage and protect against regressions as more databases are added.
Suggested implementation:
```python
from config import _load_database_configuration
def test_load_configuration_adds_environment_variables(monkeypatch) -> None:
# Arrange: base configuration for two database aliases
fake_config = {
"databases": {
"openml": {
"password": "openml_default_password",
},
"expdb": {
"password": "expdb_default_password",
},
}
}
# Environment variable overrides for both aliases
monkeypatch.setenv("OPENML_DATABASES_OPENML_PASSWORD", "openml_env_password")
monkeypatch.setenv("OPENML_DATABASES_EXPDB_PASSWORD", "expdb_env_password")
# Act
config = _load_database_configuration(fake_config)
# Assert: defaults are defined and env vars override them
assert config["databases"]["openml"]["password"] == "openml_env_password"
assert config["databases"]["expdb"]["password"] == "expdb_env_password"
```
1. If this test file already defines a `_fake_config` helper/fixture, update it to include both `openml` and `expdb` aliases with default `password` values, and use that helper here instead of the inline `fake_config` dict (e.g. call `_load_database_configuration(_fake_config())`).
2. Adjust the structure of `fake_config` and the assertion paths (`config["databases"]["…"]["password"]`) if `_load_database_configuration` expects/returns a slightly different shape (for example, a top-level `DATABASES` key or alias names as upper-case keys).
3. Confirm the exact env var naming convention used by `_load_database_configuration`. If it differs (e.g. `OPENML_DATABASES_OPENML_PASS` or `OPENML_DATABASES_OPENML__PASSWORD`), update the `setenv` calls accordingly to match the implementation.
</issue_to_address>
### Comment 5
<location path="src/config.py" line_range="28" />
<code_context>
-_config_file = _config_file.expanduser().absolute()
-_dotenv_file = Path(os.getenv(DOTENV_FILE_ENV, _config_directory / ".env"))
-_dotenv_file = _dotenv_file.expanduser().absolute()
+_config: Configuration | None = None
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the configuration module by using a single cache without globals, inlining the two known DB configs, and clarifying `get_config`/`parse_configuration`/`load_set_configuration` roles.
You can simplify this module significantly by removing the double-caching pattern and over‑generic DB loader, without changing behavior.
### 1. Remove double-caching: pick one mechanism
Right now `get_config` is both cached and backed by a mutable global `_config`, which makes ordering subtle:
```python
_config: Configuration | None = None
@functools.cache
def get_config() -> Configuration:
if _config is None:
load_set_configuration()
return cast("Configuration", _config)
```
This is effectively two caches. You can keep the laziness and typing but drop the global, and cache on the arguments to the parser instead.
**Suggestion: cache the parser and make `get_config` a thin wrapper, no globals.**
```python
# Remove the module-level mutable state
# _config: Configuration | None = None
@functools.cache
def _parse_and_cache(
dotenv_file: Path | None,
configuration_file: Path | None,
) -> Configuration:
return parse_configuration(dotenv_file=dotenv_file, configuration_file=configuration_file)
def get_config(
dotenv_file: Path | None = None,
configuration_file: Path | None = None,
) -> Configuration:
"""Primary entrypoint: returns a cached Configuration for the given paths."""
return _parse_and_cache(dotenv_file, configuration_file)
```
Then, if you still want a “set global config” style entrypoint, make it a thin adapter over `get_config` instead of mutating `_config`:
```python
def load_set_configuration(
dotenv_file: Path | None = None,
configuration_file: Path | None = None,
) -> None:
"""Eagerly load (and cache) the configuration for use via get_config()."""
# Optionally clear cache if you want to support reloading:
# _parse_and_cache.cache_clear()
get_config(dotenv_file=dotenv_file, configuration_file=configuration_file)
```
This removes the ordering dependency and keeps a single, obvious cache.
### 2. Avoid over-generic DB loader for two fixed DBs
`_load_database_configuration` builds a generic `dict[str, DatabaseConfiguration]`, but you immediately destructure only two known keys. That indirection can be removed by using a small helper for the env override and constructing the two configurations directly.
Current:
```python
def _load_database_configuration(
configurations: dict[str, dict[str, str]],
) -> dict[str, DatabaseConfiguration]:
database_configurations = {}
for db_alias, db_configuration in configurations.items():
credentials = {
"username": os.environ.get(
f"OPENML_DATABASES_{db_alias.upper()}_USERNAME",
"root",
),
"password": os.environ.get(
f"OPENML_DATABASES_{db_alias.upper()}_PASSWORD",
"ok",
),
}
database_configurations[db_alias] = DatabaseConfiguration(**db_configuration, **credentials)
return database_configurations
# ...
db_configurations = _load_database_configuration(config["databases"])
return Configuration(
# ...
openml_database=db_configurations["openml"],
expdb_database=db_configurations["expdb"],
# ...
)
```
**Suggestion: keep the env override logic, but inline the two known DBs.**
```python
def _db_env_credentials(alias: str) -> dict[str, str]:
upper = alias.upper()
return {
"username": os.environ.get(f"OPENML_DATABASES_{upper}_USERNAME", "root"),
"password": os.environ.get(f"OPENML_DATABASES_{upper}_PASSWORD", "ok"),
}
def parse_configuration(
dotenv_file: Path | None = None,
configuration_file: Path | None = None,
) -> Configuration:
# ... same directory / dotenv / file resolution ...
config = tomllib.loads(configuration_file.read_text())
db_section = config["databases"]
openml_db = DatabaseConfiguration(
**db_section["openml"],
**_db_env_credentials("openml"),
)
expdb_db = DatabaseConfiguration(
**db_section["expdb"],
**_db_env_credentials("expdb"),
)
return Configuration(
routing=RoutingConfiguration(**config["routing"]),
logging=[
LoggingConfiguration(**sink_configuration)
for sink_configuration in config["logging"].values()
],
openml_database=openml_db,
expdb_database=expdb_db,
development=DevelopmentConfiguration(**config["development"]),
)
```
This keeps all existing behavior (including env overrides) but removes the generic mapping that isn’t used as such.
### 3. Clarify the public API surface
With the changes above, the responsibilities become much clearer:
- `get_config(...)` – primary, cached entrypoint.
- `parse_configuration(...)` – pure, one-shot parser with no caching.
- `load_set_configuration(...)` – optional eager warm‑up wrapper over `get_config`.
The code changes in 1 and 2 already move you toward this: `get_config` is the single way to “get the current config,” and `parse_configuration` is a simple, test-friendly pure function.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/database/users.py (1)
15-17: 💤 Low value
api_key_patternis computed once at import time — note the ordering requirement.
get_config()is called at module scope here. Ifusers.pyis imported beforeload_set_configuration()is called with a specific config file, the default config is loaded (and, with the@functools.cacheissue noted inconfig.py, permanently cached). This makesallow_test_api_keysand the resultingAPIKeyconstraint effectively immutable for the process lifetime once any import occurs.This is fine for production (config is set first in
main.py), but testing requires that the test config be loaded viaload_set_configuration()before this module is imported, or thatget_config.cache_clear()is called between test cases. Ensuring this ordering is documented, or deferring the pattern evaluation (e.g. aget_api_key_pattern()function), would reduce the risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/database/users.py` around lines 15 - 17, The module-level api_key_pattern is computed once using get_config() at import time which makes allow_test_api_keys effectively immutable; change this to a runtime lookup by replacing the module-level api_key_pattern with a function (e.g., get_api_key_pattern()) that calls get_config().development.allow_test_api_keys and returns the appropriate regex, and update any usage sites (where api_key_pattern was referenced) to call get_api_key_pattern() so the pattern reflects the current configuration without relying on import ordering or cache clearing.src/config.py (1)
167-167: ⚡ Quick win
read_text()uses the system's default encoding — specify"utf-8"explicitly.TOML requires UTF-8. On Windows,
read_text()may use a non-UTF-8 locale encoding. Prefer the standard pattern fortomllib:♻️ Proposed fix
-config = tomllib.loads(configuration_file.read_text()) +with open(configuration_file, "rb") as f: + config = tomllib.load(f)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.py` at line 167, The call to tomllib.loads currently uses configuration_file.read_text() without an explicit encoding; change the read to use UTF-8 to comply with TOML and avoid locale issues—replace configuration_file.read_text() with configuration_file.read_text(encoding="utf-8") (the line assigning config using tomllib.loads should consume that UTF-8 text).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.py`:
- Around line 28-36: The get_config function is incorrectly decorated with
`@functools.cache` which can return a stale Configuration when
load_set_configuration() is called later; remove the `@functools.cache` decorator
so get_config always reads the _config global (and keep returning
cast("Configuration", _config)), and also remove the functools import if it's
unused elsewhere; ensure get_config, load_set_configuration, and the _config
global remain as the single-source cache.
In `@src/config.toml`:
- Around line 9-11: There is a duplicated comment block saying "Any number of
logging.NAME configurations can be added. NAME is for reference only, it has no
meaning otherwise."; remove the redundant copy and keep a single instance of
that comment to avoid repetition, ensuring the retained comment remains adjacent
to the logging configuration examples or header (look for the exact text "Any
number of logging.NAME configurations can be added" in the file to locate both
duplicates).
In `@src/database/setup.py`:
- Around line 10-22: The URL created in _create_engine ignores
DatabaseConfiguration.port so non-default ports are dropped; update the
URL.create call inside _create_engine to pass the port (e.g.,
port=db_config.port) so the constructed URL reflects the configured port before
calling create_async_engine; ensure you reference DatabaseConfiguration.port
when adding the parameter to URL.create.
In `@src/routers/mldcat_ap/dataset.py`:
- Around line 34-37: The module-level evaluation of _routing_configuration =
config.get_config().routing and the derived _server_url freezes the server URL
at import time (before load_set_configuration() and create_api(...) can load a
test config); change this to compute the URL lazily by removing the module-level
_server_url/_routing_configuration initialization and adding a small accessor
function (e.g., _get_server_url()) that calls config.get_config().routing and
returns f"{routing.server_url}{routing.root_path}{router.prefix}", then update
all handlers that reference _server_url to call _get_server_url() instead so the
correct config is used after load_set_configuration().
In `@src/routers/openml/tasks.py`:
- Around line 168-170: The code reconstructs the server URL manually from
get_config().routing.server_url using url.scheme, url.host and url.port which
causes redundant default ports (e.g., :443); change it to use
str(get_config().routing.server_url) (or call str(url) on the existing
routing.server_url) and replace "[CONSTANT:base_url]" with that canonical string
so the returned URL omits redundant default ports; update the block around
get_config(), routing.server_url and the template.replace call to use str(url)
instead of f"{url.scheme}://{url.host}:{url.port}/".
---
Nitpick comments:
In `@src/config.py`:
- Line 167: The call to tomllib.loads currently uses
configuration_file.read_text() without an explicit encoding; change the read to
use UTF-8 to comply with TOML and avoid locale issues—replace
configuration_file.read_text() with
configuration_file.read_text(encoding="utf-8") (the line assigning config using
tomllib.loads should consume that UTF-8 text).
In `@src/database/users.py`:
- Around line 15-17: The module-level api_key_pattern is computed once using
get_config() at import time which makes allow_test_api_keys effectively
immutable; change this to a runtime lookup by replacing the module-level
api_key_pattern with a function (e.g., get_api_key_pattern()) that calls
get_config().development.allow_test_api_keys and returns the appropriate regex,
and update any usage sites (where api_key_pattern was referenced) to call
get_api_key_pattern() so the pattern reflects the current configuration without
relying on import ordering or cache clearing.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6208e319-d9d0-4b1b-a890-a32c704536fa
📒 Files selected for processing (11)
src/config.pysrc/config.tomlsrc/core/formatting.pysrc/core/logging.pysrc/database/setup.pysrc/database/users.pysrc/main.pysrc/routers/mldcat_ap/dataset.pysrc/routers/openml/tasks.pytests/config.test.tomltests/config_test.py
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.
No description provided.