Fix configuration regressions and modernize container runtime defaults - #16
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughOptimizes Docker image build caching by separating dependency and source layers, adds new configuration fields for logging and rate limiting with environment variable support, introduces a non-root runtime user, and refactors app startup validation from event handler to async lifespan context manager. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's robustness and maintainability by addressing configuration regressions and modernizing container runtime practices. It ensures explicit and typed configuration usage, provides sensible defaults for various settings, and improves the overall security and efficiency of the Docker build process. The changes aim to prevent startup failures, facilitate easier bootstrapping in minimal environments, and promote safer handling of sensitive information. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several significant improvements, including modernizing the Dockerfile for better caching and security, fixing configuration-related startup issues by introducing a typed AppSettings model, and replacing a deprecated FastAPI startup event with a lifespan handler. The changes are well-executed and enhance the project's robustness and maintainability. I have a couple of suggestions to further improve container image size and configuration consistency.
| # Run the application. | ||
| CMD ["/app/.venv/bin/fastapi", "run", "src/acebet/app/main.py", "--port", "80", "--host", "0.0.0.0"] | ||
| # Copy dependency manifests first for better layer caching. | ||
| COPY pyproject.toml uv.lock README.md ./ |
There was a problem hiding this comment.
The README.md file is being copied into the container image but doesn't appear to be required for building dependencies or at runtime. To keep the container image as lean as possible, which is a containerization best practice, consider removing it from this COPY instruction.
COPY pyproject.toml uv.lock ./
There was a problem hiding this comment.
@codex check if it the readme is still being copied and needs to be fixed
There was a problem hiding this comment.
Result
I checked the Dockerfile at the referenced location, and README.md is not being copied anymore. The COPY instruction at line 12 currently includes only pyproject.toml and uv.lock, so there is nothing to fix for that specific review comment. DockerfileL11-L13
Commands I used
- ✅
sed -n '1,220p' Dockerfile - ✅
nl -ba Dockerfile | sed -n '1,80p' - ✅
curl -s https://docs.docker.com/build/cache/optimize/ | head -n 5(web check per instruction)
Repo update status
No code changes were required for this check, so I did not create a commit and did not open a PR.
| from starlette.types import Message | ||
|
|
||
| from acebet.app.config import validate_config | ||
| from acebet.app.config import settings, validate_config |
There was a problem hiding this comment.
Great work introducing the typed settings object. For consistency and to fully embrace the typed configuration pattern you've introduced, I suggest refactoring the application to exclusively use the settings object for all configuration values. Currently, some parts of the code still use module-level constants from config.py (e.g., ACCESS_TOKEN_EXPIRE_MINUTES). Using settings.acebet_access_token_expire_minutes directly would make the configuration management more unified and maintainable.
There was a problem hiding this comment.
@codex check if we need to fix the imports and make consistent importing settings and accessing the env var via the config object
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85efb3c635
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| USER app | ||
|
|
||
| # Run the application. | ||
| CMD ["/app/.venv/bin/fastapi", "run", "src/acebet/app/main.py", "--port", "80", "--host", "0.0.0.0"] |
There was a problem hiding this comment.
This change switches the container to USER app but keeps FastAPI bound to port 80, which causes startup failure on standard Linux container defaults where non-root users cannot bind ports below 1024. In those environments the process exits with a permission error before serving traffic, so the default image command is not runnable without extra runtime sysctl/capability tweaks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex check if the issue remains and fix it if necessary
There was a problem hiding this comment.
Summary
- Checked the referenced review concern and confirmed it still applied: the container ran as non-root (
USER app) while binding FastAPI to privileged port80; updated the Docker CMD to use port8000so default runtime works without extra capabilities. DockerfileL25-L28 - Committed the fix on the current branch (
5f7c5d6) and created a follow-up PR via themake_prtool.
Testing
- ✅
curl -s https://man7.org/linux/man-pages/man7/ip.7.html | rg -n "privileged ports|below 1024" -m 2 - ✅
nl -ba Dockerfile | sed -n '1,120p' - ✅
nl -ba Dockerfile | sed -n '22,32p' - ✅
git status --short
| RUN uv sync --frozen --no-cache --no-install-project | ||
|
|
||
| # Copy application source. | ||
| COPY src ./src |
There was a problem hiding this comment.
The Docker build now copies only src/, but the /predict/ handler defaults to testing=False and reads the production dataset from /app/data/atp_data_production.feather (the top-level data/ tree). Because that directory is no longer copied into the image, regular prediction requests in the container hit a file-not-found error at runtime.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex make sure the copy cotains also the data, the prediction requests must work in docker
There was a problem hiding this comment.
Summary
- Addressed the PR thread request by updating the Docker build to include runtime data needed by prediction requests:
Dockerfilenow copiesdata/into/app/data(COPY data ./data) alongsidesrc/. This preserves the current layering approach while fixing the missing dataset issue in container runs. DockerfileL17-L19 - Confirmed the app’s production prediction path expects the dataset under
/app/data/atp_data_production.feather, which is why this copy is required for Docker runtime correctness. src/acebet/app/main.pyL215-L219 - Committed the change on the current branch (
b34434a) and created a follow-up PR via themake_prtool.
Testing
- ✅
curl -s https://docs.docker.com/build/cache/optimize/ | head -n 5 - ✅
nl -ba Dockerfile | sed -n '1,120p' - ✅
nl -ba src/acebet/app/main.py | sed -n '206,230p' - ✅
git add Dockerfile && git commit -m "Include data assets in Docker image for prediction endpoint" - ✅
git status --short
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17b09896a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| model_config = SettingsConfigDict(extra="ignore") | ||
|
|
||
| acebet_env: str = Field(alias="ACEBET_ENV") | ||
| acebet_env: str = Field(default="development", alias="ACEBET_ENV") |
There was a problem hiding this comment.
Defaulting acebet_env to "development" makes a missing ACEBET_ENV look like an intentional dev deployment, so validate_secret_key_in_non_dev is bypassed and the code falls back to the hardcoded JWT secret (ACEBET_SECRET_KEY = ... "acebet-dev-insecure-secret-key"). Before this change, missing ACEBET_ENV failed fast; after it, a misconfigured production container can silently start with a predictable signing key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Reverted
ACEBET_ENVto a required setting by removing the"development"default fromAppSettings, restoring fail-fast behavior when the env var is missing. src/acebet/app/config.pyL14-L17 - Updated test bootstrap setup to explicitly define
ACEBET_ENV=testbefore app import, so tests remain stable with the stricter config requirement. tests/test_acebet.pyL6-L10 - Committed the changes on the current branch (
bfb1c47) and created a follow-up PR via themake_prtool.
Testing
- ✅
curl -s https://docs.pydantic.dev/latest/concepts/pydantic_settings/ | head -n 20 - ✅
uv run pytest -q - ✅
uv run ruff check src/acebet/app/config.py tests/test_acebet.py
| RUN uv sync --frozen --no-cache --no-install-project | ||
|
|
||
| # Copy application source. | ||
| COPY src ./src |
There was a problem hiding this comment.
This Dockerfile now only copies src into /app, but the default inference path (testing=false) loads /app/data/atp_data_production.feather from src/acebet/app/main.py. As a result, containerized /predict requests in default mode will fail with FileNotFoundError unless operators manually mount data/, which is a regression from the previous COPY . /app behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex make sure the data are also copied so that the predict works fine in docker
There was a problem hiding this comment.
Summary
- Updated the Docker build to include runtime data assets by adding
COPY data ./dataalongsidesrc, so prediction requests in Docker can access/app/data/*as expected. DockerfileL17-L19 - Kept the existing layer-caching structure intact (manifests first, dependency install, then app assets), only expanding the runtime copy step to include
data/as requested in the review thread. DockerfileL11-L22 - Committed the change on the current branch (
9dabf11) and created a follow-up PR via themake_prtool.
Testing
- ✅
curl -s https://docs.docker.com/reference/dockerfile/ | head -n 20 - ✅
nl -ba Dockerfile | sed -n '1,80p' - ✅
git status --short && git rev-parse HEAD
1 participant
Motivation
Description
AppSettingsfields for environment, logging, and rate-limits with defaults and aredacted()helper for safe structured logging insrc/acebet/app/config.py.settingsobject into the application entrypoint and switch tosettings.acebet_*attributes for rate limits and logging insrc/acebet/app/main.py.@app.on_event("startup")with a FastAPIlifespanhandler that runs configuration validation before serving requests.jwt.encode/jwt.decodecalls) insrc/acebet/app/dependencies/auth.py.Dockerfilelayering and runtime defaults by copying dependency manifests first, pre-installing deps withuv sync --no-install-project, copying source afterwards, and running the container as a non-root user, plus Python runtime env flags.Testing
uv run pytest -qand all tests passed (4 passed).uv run ruff check src/acebet/app/config.py src/acebet/app/main.py src/acebet/app/dependencies/auth.pyand linting checks passed for the changed files.docker build -t acebet:test .but thedockerCLI is not available in the execution environment so the image build could not be validated here.Codex Task
Summary by CodeRabbit
Release Notes
Chores
New Features