Harden runtime configuration, improve prediction safety, and align packaging/Docker best practices - #15
Conversation
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 operational robustness and maintainability by refining its configuration management, improving the reliability of its prediction services, and aligning its deployment artifacts with modern best practices. The changes ensure a more stable startup, predictable API behavior, and efficient Docker image builds, contributing to a more resilient and developer-friendly system. 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
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
|
📝 WalkthroughWalkthroughThe changes add rate limiting and logging configuration fields to the application settings, enhance Docker runtime optimizations, replace debug flags with log level configuration, improve type safety with explicit annotations, add input validation guards, and include module-level documentation across the codebase. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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 a solid set of improvements, hardening the application's runtime configuration, improving prediction safety, and aligning packaging and Docker practices with modern standards. The changes are well-executed and clearly beneficial. I have one suggestion regarding the Dockerfile to further optimize the build process by improving layer caching.
| COPY . /app | ||
|
|
||
| # Install the application dependencies. | ||
| WORKDIR /app | ||
| RUN uv sync --frozen --no-cache | ||
| # Install runtime dependencies only. | ||
| RUN uv sync --frozen --no-cache --no-dev |
There was a problem hiding this comment.
To better leverage Docker's layer caching, it's a good practice to copy only the dependency manifests (pyproject.toml, uv.lock), install dependencies, and then copy the rest of the application source code. This separation ensures that the dependency layer is only rebuilt when dependencies change, not on every code change, leading to faster builds.
COPY pyproject.toml uv.lock ./
# Install runtime dependencies only.
RUN uv sync --frozen --no-cache --no-dev
COPY . .
![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: b55a0bcf43
ℹ️ 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 environment variable silently opt into development behavior, which bypasses validate_secret_key_in_non_dev and allows the app to run with the known fallback JWT secret (ACEBET_SECRET_KEY in config.py). In any production/staging deployment where ACEBET_ENV is accidentally omitted, tokens can be forged with that default key instead of failing fast at startup as before.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/acebet/app/dependencies/predict_winner.py (1)
132-133:⚠️ Potential issue | 🟡 MinorValueError from
predict()not translated to HTTP error in endpoint.When
query_datareturns an empty DataFrame,predict()raises aValueError. However, inmain.py,make_prediction()is called without catching this error, so it will propagate as a 500 Internal Server Error rather than a more appropriate 404 or 400 response.Consider handling this in the
/predict/endpoint to return a user-friendly HTTP error.🛡️ Suggested handling in main.py
In
src/acebet/app/main.py, wrap the call to handle the ValueError:try: prob, class_, player_1 = make_prediction( data_file=data_file, model_path=model_path, p1_name=p1_name, p2_name=p2_name, date=date, ) except ValueError as exc: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail=str(exc), ) from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/acebet/app/dependencies/predict_winner.py` around lines 132 - 133, The endpoint calling make_prediction should catch the ValueError raised when query_data returns an empty DataFrame (which bubbles up from predict) and translate it into an HTTP error; update the /predict handler in main.py where make_prediction(...) is invoked to wrap the call in a try/except ValueError block and raise fastapi.HTTPException (e.g., status.HTTP_404_NOT_FOUND or status.HTTP_400_BAD_REQUEST) with the ValueError message as detail, preserving the original exception via "from exc".
🧹 Nitpick comments (4)
src/acebet/app/main.py (2)
228-228: Unnecessarydel current_user- Python handles garbage collection automatically.The
del current_userstatement is not needed. Thecurrent_uservariable goes out of scope when the function returns, and Python's garbage collector handles cleanup. This pattern may suggest an intent to prevent accidental usage, but it's unconventional and adds noise.If the intent is to signal that
current_useris only used for endpoint protection, consider renaming to_current_userto indicate it's intentionally unused.✨ Suggested alternative
`@app.post`("/predict/", response_model=PredictionResponse) def predict_match_outcome( - request: PredictionRequest, current_user: UserInDB = Depends(get_current_user) + request: PredictionRequest, _current_user: UserInDB = Depends(get_current_user) ) -> PredictionResponse:Then remove line 228:
- del current_user🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/acebet/app/main.py` at line 228, Remove the unnecessary del current_user statement: either delete the line "del current_user" (preferred) or, if you want to indicate the variable is intentionally unused, rename the variable to _current_user where it's defined (e.g., in the endpoint function's parameter or local binding) so you avoid the explicit deletion while conveying intent; update any references accordingly (look for current_user in the endpoint function surrounding the del to locate the change).
50-53: Redundant validation at startup - configuration already validated at module load.The
settings = AppSettings()at module level inconfig.py(line 77) already validates configuration when the module is imported. Callingvalidate_config()again at startup creates a secondAppSettings()instance that re-reads environment variables and re-runs validators. This is redundant and could cause confusion if environment variables change between import time and startup (unlikely but possible in edge cases).Consider whether this double validation is intentional. If the goal is to surface errors clearly at startup, the existing import-time validation already achieves this. If you want explicit startup validation, consider making
settingslazy-loaded instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/acebet/app/main.py` around lines 50 - 53, The startup hook validate_startup_config() is causing a second AppSettings() instantiation via validate_config(), so either remove the `@app.on_event`("startup") validate_startup_config() function entirely to rely on the module-level settings in config.py, or modify validate_config() to reuse the existing singleton settings object (e.g., import and use config.settings) instead of creating a new AppSettings(); target symbols: validate_startup_config, validate_config, AppSettings, and settings in config.py.src/acebet/app/config.py (2)
84-89:validate_config()creates a duplicate settings instance.This function instantiates a new
AppSettings()every time it's called, which re-reads environment variables. Sincesettings = AppSettings()is already created at module level (line 77) and validation happens there, this function may return success even if the actualsettingssingleton was created with different values (edge case).If the intent is to validate at startup, consider reusing the existing singleton or making this function return the validated instance.
✨ Suggested improvement
def validate_config() -> None: """Validate startup configuration and raise clear runtime errors when invalid.""" - try: - AppSettings() - except ValidationError as exc: - raise RuntimeError(f"Invalid application configuration: {exc}") from exc + # Settings already validated at module load; this ensures errors surface clearly. + if settings is None: + raise RuntimeError("Configuration failed to load.")Or simply document that this function is a no-op safety check since validation happens at import time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/acebet/app/config.py` around lines 84 - 89, The function validate_config currently instantiates a fresh AppSettings(), which re-reads env and can diverge from the module-level singleton settings; change validate_config to reuse and return the existing module-level settings variable instead of constructing a new AppSettings() (i.e., reference settings, optionally wrapping access in a try/except for ValidationError to raise a RuntimeError), or make it explicitly a no-op that returns settings so callers get the validated singleton.
21-26: Add validation for rate limit format strings.The
default_rate_limitandlogin_rate_limitfields accept arbitrary strings without validation. slowapi expects specific formats like"60/minute","5/second", or multiple limits like"1/second;5/minute;20/hour". Invalid formats will cause runtime errors when the limiter is instantiated.Add a field validator to ensure the rate limit strings match the expected pattern.
✨ Suggested validator
import re `@field_validator`("default_rate_limit", "login_rate_limit") `@classmethod` def validate_rate_limit(cls, value: str) -> str: """Ensure rate limit matches slowapi expected format.""" pattern = r"^\d+/(second|minute|hour|day)(?:;\d+/(second|minute|hour|day))*$" if not re.match(pattern, value.strip()): raise ValueError( f"Rate limit '{value}' must match format like '60/minute' or '1/second;5/minute;20/hour'." ) return value.strip()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/acebet/app/config.py` around lines 21 - 26, The rate limit fields default_rate_limit and login_rate_limit currently accept any string and need validation to prevent runtime errors; add a class field validator (e.g., def validate_rate_limit) using pydantic's `@field_validator` for "default_rate_limit" and "login_rate_limit", import re, apply a regex like ^\d+/(second|minute|hour|day)(?:;\d+/(second|minute|hour|day))*$ to validate and strip the value, raise ValueError with a clear message on mismatch, and return the cleaned string so the limiter receives only properly formatted rate strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 29-30: Update the comment above the ACEBET_LOGIN_RATE_LIMIT
variable to reflect that this value configures the rate limit for login attempts
or the login endpoint (not a generic /limit demo); mention the expected format
(e.g., "10/minute") and scope (login attempts/session) so env users clearly
understand ACEBET_LOGIN_RATE_LIMIT's intent and usage.
---
Outside diff comments:
In `@src/acebet/app/dependencies/predict_winner.py`:
- Around line 132-133: The endpoint calling make_prediction should catch the
ValueError raised when query_data returns an empty DataFrame (which bubbles up
from predict) and translate it into an HTTP error; update the /predict handler
in main.py where make_prediction(...) is invoked to wrap the call in a
try/except ValueError block and raise fastapi.HTTPException (e.g.,
status.HTTP_404_NOT_FOUND or status.HTTP_400_BAD_REQUEST) with the ValueError
message as detail, preserving the original exception via "from exc".
---
Nitpick comments:
In `@src/acebet/app/config.py`:
- Around line 84-89: The function validate_config currently instantiates a fresh
AppSettings(), which re-reads env and can diverge from the module-level
singleton settings; change validate_config to reuse and return the existing
module-level settings variable instead of constructing a new AppSettings()
(i.e., reference settings, optionally wrapping access in a try/except for
ValidationError to raise a RuntimeError), or make it explicitly a no-op that
returns settings so callers get the validated singleton.
- Around line 21-26: The rate limit fields default_rate_limit and
login_rate_limit currently accept any string and need validation to prevent
runtime errors; add a class field validator (e.g., def validate_rate_limit)
using pydantic's `@field_validator` for "default_rate_limit" and
"login_rate_limit", import re, apply a regex like
^\d+/(second|minute|hour|day)(?:;\d+/(second|minute|hour|day))*$ to validate and
strip the value, raise ValueError with a clear message on mismatch, and return
the cleaned string so the limiter receives only properly formatted rate strings.
In `@src/acebet/app/main.py`:
- Line 228: Remove the unnecessary del current_user statement: either delete the
line "del current_user" (preferred) or, if you want to indicate the variable is
intentionally unused, rename the variable to _current_user where it's defined
(e.g., in the endpoint function's parameter or local binding) so you avoid the
explicit deletion while conveying intent; update any references accordingly
(look for current_user in the endpoint function surrounding the del to locate
the change).
- Around line 50-53: The startup hook validate_startup_config() is causing a
second AppSettings() instantiation via validate_config(), so either remove the
`@app.on_event`("startup") validate_startup_config() function entirely to rely on
the module-level settings in config.py, or modify validate_config() to reuse the
existing singleton settings object (e.g., import and use config.settings)
instead of creating a new AppSettings(); target symbols:
validate_startup_config, validate_config, AppSettings, and settings in
config.py.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.env.exampleDockerfiledocs/deployment/docker-and-github-actions.mdpyproject.tomlsrc/acebet/app/__init__.pysrc/acebet/app/config.pysrc/acebet/app/dependencies/auth.pysrc/acebet/app/dependencies/predict_winner.pysrc/acebet/app/main.pysrc/acebet/data/__init__.pysrc/acebet/dataprep/__init__.pysrc/acebet/train/__init__.py
| # Rate limit for the /limit endpoint demo. | ||
| ACEBET_LOGIN_RATE_LIMIT=10/minute |
There was a problem hiding this comment.
Clarify ACEBET_LOGIN_RATE_LIMIT comment to match variable intent.
The current wording mentions a /limit demo endpoint, which conflicts with the LOGIN-scoped variable name and can confuse runtime configuration.
💡 Proposed doc fix
-# Rate limit for the /limit endpoint demo.
+# Rate limit for authentication/login endpoint requests.
ACEBET_LOGIN_RATE_LIMIT=10/minute📝 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.
| # Rate limit for the /limit endpoint demo. | |
| ACEBET_LOGIN_RATE_LIMIT=10/minute | |
| # Rate limit for authentication/login endpoint requests. | |
| ACEBET_LOGIN_RATE_LIMIT=10/minute |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 29 - 30, Update the comment above the
ACEBET_LOGIN_RATE_LIMIT variable to reflect that this value configures the rate
limit for login attempts or the login endpoint (not a generic /limit demo);
mention the expected format (e.g., "10/minute") and scope (login
attempts/session) so env users clearly understand ACEBET_LOGIN_RATE_LIMIT's
intent and usage.
1 participant
Motivation
Description
AppSettingswith defaults and validators:ACEBET_ENV,ACEBET_LOG_LEVEL,ACEBET_LOG_FILE,ACEBET_DEFAULT_RATE_LIMIT, andACEBET_LOGIN_RATE_LIMIT, plus aredacted()view for safe startup logging insrc/acebet/app/config.py.ACEBET_ACCESS_TOKEN_EXPIRE_MINUTESandACEBET_LOG_LEVEL, and requireACEBET_SECRET_KEYin non-dev environments; kept a dev-default secret fallback for local use.settingsinsrc/acebet/app/main.py, tightened middleware typing toCallable[[Request], Awaitable[Response]], and usedsettingsvalues for limiter and logging configuration.src/acebet/app/dependencies/predict_winner.pyby raising on empty query results and keeping function return typing; normalizedprobandclass_insrc/acebet/app/main.pyto scalarfloat/intbefore serializing thePredictionResponseand removed unusedcurrent_userusage.pyproject.tomlfromdata*toacebet*so the package is discovered correctly.Dockerfileto setPYTHONDONTWRITEBYTECODE/PYTHONUNBUFFERED, useuv sync --no-devfor runtime-only installs, and explicitlyEXPOSE 80for the service port..env.examplenow documents new env vars, deployment docs updated to referenceACEBET_LOG_LEVEL, and package__init__.pydocstrings were added to satisfy linters.ruff --fixstyle cleanups and wrapped long lines to satisfy project lint rules.Testing
uv run ruff check src testswhich completed withAll checks passed!.uv run pytest -qwhich succeeded (4 passed) and emitted only upstream deprecation/warning messages (FastAPIon_eventdeprecation and LightGBM/scikit-learn tag warnings).Codex Task
Summary by CodeRabbit
New Features
Bug Fixes
Chores
ACEBET_DEBUGtoACEBET_LOG_LEVEL