Update configurability and docker documentation#245
Merged
Conversation
Contributor
WalkthroughThis pull request implements an environment-driven configuration system for the OpenML REST API alongside Docker infrastructure updates. Changes include modifying Possibly related PRs
🚥 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 |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The Dockerfile sets
OPENML_REST_API_CONFIG_FILE_PATHbut the code expectsOPENML_REST_API_CONFIG_FILE/OPENML_REST_API_CONFIG_DIRECTORY, so the container won’t actually use the intended config file unless these names are aligned. - Calling
logging.basicConfigat import time inconfig.pycan override application-level logging configuration; consider configuring the logger locally (e.g., viaNullHandleror leaving configuration to the entrypoint) instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Dockerfile sets `OPENML_REST_API_CONFIG_FILE_PATH` but the code expects `OPENML_REST_API_CONFIG_FILE` / `OPENML_REST_API_CONFIG_DIRECTORY`, so the container won’t actually use the intended config file unless these names are aligned.
- Calling `logging.basicConfig` at import time in `config.py` can override application-level logging configuration; consider configuring the logger locally (e.g., via `NullHandler` or leaving configuration to the entrypoint) instead.
## Individual Comments
### Comment 1
<location> `src/config.py:8-11` </location>
<code_context>
from dotenv import load_dotenv
+logging.basicConfig(level=logging.INFO)
+logger = logging.getLogger(__name__)
+
TomlTable = dict[str, typing.Any]
</code_context>
<issue_to_address>
**suggestion:** Avoid calling basicConfig at import time in a library module
Calling `logging.basicConfig` in a library module configures the root logger globally and can override logging settings of applications that import this code. Instead, just define a module logger with `logging.getLogger(__name__)` here and move any logging configuration to the application entrypoint (e.g. `main.py`).
```suggestion
from dotenv import load_dotenv
logger = logging.getLogger(__name__)
```
</issue_to_address>
### Comment 2
<location> `docker/python/Dockerfile:21` </location>
<code_context>
+RUN mkdir /config
+COPY ./src/config.toml /config/config.toml
+ENV OPENML_REST_API_CONFIG_FILE_PATH=/config/config.toml
+COPY ./docker/python/README.md /README.md
</code_context>
<issue_to_address>
**issue (bug_risk):** Config env var name does not match what the application reads
In `src/config.py`, the app reads `OPENML_REST_API_CONFIG_FILE` (`CONFIG_FILE_ENV`), but the Dockerfile sets `OPENML_REST_API_CONFIG_FILE_PATH`. This env var will never be used, so the container will always fall back to the default config path. Please align the names (e.g., use `OPENML_REST_API_CONFIG_FILE` here or update the Python constant) so `/config/config.toml` is actually loaded.
</issue_to_address>
### Comment 3
<location> `docker/python/Dockerfile:13-17` </location>
<code_context>
RUN python -m pip install uv
-WORKDIR /python-api
+WORKDIR /app
COPY pyproject.toml .
RUN uv pip install -e ".[dev]"
-COPY . /python-api
+COPY . /app
+RUN mkdir /config
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Workdir and docker-compose bind mount now point to different paths
The image now installs and runs code from `/app`, but `docker-compose.yaml` still bind-mounts the source into `/python-api`. With this mismatch, changes on the host won’t affect the code actually executed in the container. Either update the bind mount to target `/app` for live development, or remove the bind mount if you intend to run only the baked image contents.
Suggested implementation:
```
WORKDIR /python-api
```
```
COPY . /python-api
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker/python/Dockerfile (1)
26-26:⚠️ Potential issue | 🟠 MajorRemove
--reloadfrom the defaultCMDof a published image.
--reloadenables uvicorn's file-system watcher and auto-restart on code changes — a development convenience that adds overhead and is inappropriate as the default for a production-published Docker image. It should be omitted fromCMD; consumers who want it can override it at runtime.🐛 Proposed fix
-CMD ["--host", "0.0.0.0", "--reload"] +CMD ["--host", "0.0.0.0"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/python/Dockerfile` at line 26, The Dockerfile's default CMD includes the development-only flag "--reload" which should be removed from the published image; update the CMD declaration that currently contains ["--host", "0.0.0.0", "--reload"] to omit "--reload" so the default command only sets runtime options appropriate for production (e.g., keep "--host" but remove "--reload"), allowing developers to re-enable reload via an explicit runtime override if needed.
🧹 Nitpick comments (2)
.github/workflows/docker-publish.yml (1)
52-59: Consider skipping the description update for pre-releases.The "latest" tag is already excluded for pre-releases (line 38), but this step has no matching
if:guard and will update the Docker Hub description on every release, including pre-releases. If the README should only reflect stable releases, add:💡 Proposed guard
- name: Docker Hub Description uses: peter-evans/dockerhub-description@v5 + if: ${{ !github.event.release.prerelease }} with: username: ${{ secrets.DOCKERHUB_USERNAME }} password: ${{ secrets.DOCKERHUB_PASSWORD }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docker-publish.yml around lines 52 - 59, The Docker Hub Description step (uses: peter-evans/dockerhub-description@v5) runs for every release including pre-releases; add an if guard to skip this step for prereleases by requiring the release to be non-prerelease (e.g., if: github.event.release.prerelease == false) so the README/description is only updated for stable releases and matches the existing "latest" tag exclusion logic.src/config.py (1)
10-11:logging.basicConfigat module level may silently override application logging config.
basicConfigconfigures the root logger only if it has no handlers yet. If this module is imported before the application sets up its own logging, it will lock in INFO-level root logging and potentially suppress any subsequentbasicConfigcall in the app. Consider either removing this and letting the application own the root logger, or narrowing to justgetLogger(__name__)without abasicConfigcall.💡 Proposed change
-logging.basicConfig(level=logging.INFO) logger = logging.getLogger(__name__)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.py` around lines 10 - 11, The module-level call to logging.basicConfig is unsafe because it can preempt application logging; remove the logging.basicConfig(level=logging.INFO) call and only create the module logger via logger = logging.getLogger(__name__) (or, if you must set defaults, guard basicConfig with a check for existing root handlers using logging.getLogger().handlers) so the application remains responsible for global logging configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/python/Dockerfile`:
- Line 21: Dockerfile currently exports the wrong env var name
(OPENML_REST_API_CONFIG_FILE_PATH) while src/config.py expects CONFIG_FILE_ENV
(OPENML_REST_API_CONFIG_FILE), causing the config path to be ignored; update the
docker/python/Dockerfile to set ENV
OPENML_REST_API_CONFIG_FILE=/config/config.toml (or alternatively change
CONFIG_FILE_ENV in src/config.py to match the Dockerfile and README) so the
environment variable name matches the constant CONFIG_FILE_ENV used by
src/config.py and the README.
In `@docker/python/README.md`:
- Line 7: Fix the typo in the README sentence by changing "inteded" to
"intended" so the line reads "The image is not intended to be used as a
standalone image."; update the docker/python/README.md content accordingly to
correct the spelling.
---
Outside diff comments:
In `@docker/python/Dockerfile`:
- Line 26: The Dockerfile's default CMD includes the development-only flag
"--reload" which should be removed from the published image; update the CMD
declaration that currently contains ["--host", "0.0.0.0", "--reload"] to omit
"--reload" so the default command only sets runtime options appropriate for
production (e.g., keep "--host" but remove "--reload"), allowing developers to
re-enable reload via an explicit runtime override if needed.
---
Nitpick comments:
In @.github/workflows/docker-publish.yml:
- Around line 52-59: The Docker Hub Description step (uses:
peter-evans/dockerhub-description@v5) runs for every release including
pre-releases; add an if guard to skip this step for prereleases by requiring the
release to be non-prerelease (e.g., if: github.event.release.prerelease ==
false) so the README/description is only updated for stable releases and matches
the existing "latest" tag exclusion logic.
In `@src/config.py`:
- Around line 10-11: The module-level call to logging.basicConfig is unsafe
because it can preempt application logging; remove the
logging.basicConfig(level=logging.INFO) call and only create the module logger
via logger = logging.getLogger(__name__) (or, if you must set defaults, guard
basicConfig with a check for existing root handlers using
logging.getLogger().handlers) so the application remains responsible for global
logging configuration.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #245 +/- ##
=======================================
Coverage ? 54.36%
=======================================
Files ? 32
Lines ? 1135
Branches ? 101
=======================================
Hits ? 617
Misses ? 516
Partials ? 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Updates the configuration:
/configdirectory by defaultI'm aware the introduced logging situation is not ideal, but that's deferred to a follow up PR.