Added error handling #24
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
App startup & globals model-lab/app/main.py |
Fixed model path typo (onxx_models → onnx_models); added startup checks that raise FileNotFoundError if model file missing and RuntimeError if CORS origins are empty; preloads both model options into a global emotion_models dict mapping option → Model instance. |
Predict endpoint model-lab/app/main.py |
/predict no longer creates models per-request; validates model_option and returns HTTP 400 for invalid options; uses emotion_models[option]; wraps inference in try/except and returns HTTP 500 with descriptive error on failure; retains image validation and returns emotion + probabilities on success. |
Sequence Diagram(s)
sequenceDiagram
participant App as FastAPI App
participant FS as File System
participant Config as Config
rect rgb(230,245,255)
note over App: Application startup
App->>FS: Check `onnx_models/...` files for each option
alt Missing any model file
FS-->>App: Not found
App->>App: Raise FileNotFoundError
else All present
FS-->>App: Exists
App->>App: Load models -> global emotion_models{option: Model}
end
App->>Config: Validate CORS origins
alt No origins
Config-->>App: Empty
App->>App: Raise RuntimeError
else Origins present
Config-->>App: Origins list
end
end
sequenceDiagram
participant Client
participant Endpoint as /predict
participant Models as global emotion_models
participant Inference as Inference Engine
Client->>Endpoint: POST /predict (image, optional model_option)
rect rgb(255,250,230)
note over Endpoint: Validate inputs & model_option
Endpoint->>Models: Lookup emotion_models[option]
alt option invalid
Models-->>Endpoint: KeyError / not found
Endpoint->>Client: HTTP 400 (invalid model_option)
else option valid
Endpoint->>Inference: Run inference with selected Model
alt Inference raises error
Inference-->>Endpoint: Exception
Endpoint->>Client: HTTP 500 + descriptive error
else Success
Inference-->>Endpoint: emotion + probabilities
Endpoint-->>Client: HTTP 200 + results
end
end
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20–30 minutes
- Areas to focus:
- Correctness of model path and existence checks for all options.
- Behavior and message clarity of raised FileNotFoundError and RuntimeError.
- Thread-safety and lifecycle of
emotion_models(concurrent requests). - Validation and error responses for invalid
model_option. - Exception handling in inference to avoid leaking internal traces.
Poem
🐰 I hopped through files to find the right way,
Loaded models once at the break of day.
Origins checked, no more per-call race,
Predictions faster, steady in place.
A carrot cheer for code that now stays.
Pre-merge checks and finishing touches
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
|
| Title check | ❓ Inconclusive | The title 'Added error handling' is partially related to the changeset; it refers to a real aspect of the changes (error handling was indeed added), but it misses the main improvements including model preloading optimization, startup validation, and line-ending normalization that are equally significant to the changeset. | Consider a more comprehensive title that captures the primary optimization and validation changes, such as 'Optimize model loading and add comprehensive error handling' or 'Add startup validation and error handling for model predictions'. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
model-lab/app/main.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
model-lab/app/main.py (1)
model-lab/app/model.py (2)
Model(11-84)predict(58-84)
🪛 Ruff (0.14.5)
model-lab/app/main.py
18-18: Avoid specifying long messages outside the exception class
(TRY003)
23-23: Avoid specifying long messages outside the exception class
(TRY003)
39-39: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (6)
model-lab/app/main.py (6)
14-18: LGTM! Model path validation is correct.The startup validation correctly checks for the ONNX model file and raises
FileNotFoundErrorwith a clear message if missing. This ensures the application fails fast on startup rather than during the first request.The static analysis hint (TRY003) suggests using custom exception classes for long messages, but this is an optional style preference—using standard exceptions with descriptive messages is perfectly acceptable for startup validation.
20-23: LGTM! CORS validation handles edge cases correctly.The validation properly checks for empty origins by handling the edge case where
"".split(",")returns[""]. RaisingRuntimeErrorfor missing configuration at startup is the correct approach.
32-36: Excellent fix for the race condition issue!Pre-loading both model options at startup is the correct solution. This approach:
- Eliminates race conditions from concurrent requests mutating shared
model_option- Avoids expensive per-request model instantiation
- Ensures both models are ready before handling requests
The memory trade-off of loading both models upfront is justified for correctness and performance.
39-39: Static analysis false positive for FastAPI.The Ruff hint (B008) about
File()in default arguments is a false positive. In FastAPI,File(),Form(), and similar functions are dependency markers for FastAPI's dependency injection system—they're not evaluated at function definition time like normal Python defaults. This pattern is correct and idiomatic for FastAPI.Based on static analysis hints.
49-51: LGTM! Proper validation before model selection.The validation correctly checks if
model_optionexists in the preloaded models dictionary and returns HTTP 400 (client error) with a clear message for invalid options. This prevents KeyError and provides helpful feedback to API consumers.
53-57: LGTM! Inference logic is correct and safe.The inference correctly:
- Selects the appropriate preloaded model without mutating shared state
- Wraps the prediction in try/except for safety
- Chains exceptions with
from eto preserve the original stack trace- Returns HTTP 500 with a descriptive error message on failure
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
model-lab/app/main.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
model-lab/app/main.py (1)
model-lab/app/model.py (2)
Model(11-84)predict(58-84)
🪛 Ruff (0.14.4)
model-lab/app/main.py
45-45: Do not catch blind exception: Exception
(BLE001)
46-46: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (1)
model-lab/app/main.py (1)
37-40: Good error handling for image validation.The try/except block correctly catches
OSErrorandValueError, which are the appropriate exceptions raised by PIL when opening invalid or corrupted images. Exception chaining withfrom erris properly implemented.
model-lab/app/main.py
Outdated
| if not ALLOWED_ORIGINS: | ||
| raise HTTPException(status_code=500, detail="No allowed origins configured") |
There was a problem hiding this comment.
HTTPException cannot be raised at module level, and validation logic may not work as intended.
Two issues:
- Raising
HTTPExceptionat module level will crash the application on startup rather than return an HTTP 500 response. UseRuntimeErrorinstead. - The validation logic may not work correctly because
"".split(",")returns[""], not an empty list. Check for empty strings after splitting.
Apply this diff:
-if not ALLOWED_ORIGINS:
- raise HTTPException(status_code=500, detail="No allowed origins configured")
+if not ALLOWED_ORIGINS or ALLOWED_ORIGINS == [""]:
+ raise RuntimeError("No allowed origins configured in CORS_ALLOWED_ORIGINS")📝 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.
| if not ALLOWED_ORIGINS: | |
| raise HTTPException(status_code=500, detail="No allowed origins configured") | |
| if not ALLOWED_ORIGINS or ALLOWED_ORIGINS == [""]: | |
| raise RuntimeError("No allowed origins configured in CORS_ALLOWED_ORIGINS") |
🤖 Prompt for AI Agents
In model-lab/app/main.py around lines 21 to 22, replace the module-level raise
of HTTPException with a RuntimeError so the app fails at startup without trying
to use FastAPI-specific exceptions, and change the ALLOWED_ORIGINS validation to
split the string and filter out empty/whitespace-only entries (e.g., split on
',' then strip and keep only truthy values); if the filtered list is empty,
raise RuntimeError with a clear message.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
model-lab/app/main.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
model-lab/app/main.py (1)
model-lab/app/model.py (2)
Model(11-84)predict(58-84)
🪛 Ruff (0.14.5)
model-lab/app/main.py
18-18: Avoid specifying long messages outside the exception class
(TRY003)
23-23: Avoid specifying long messages outside the exception class
(TRY003)
36-36: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (2)
model-lab/app/main.py (2)
14-18: LGTM! Model path validation is correct.The directory name has been fixed and the startup validation properly uses
FileNotFoundErrorinstead ofHTTPException, which is the correct approach for module-level checks.
20-23: LGTM! CORS validation is correct.The validation properly handles the edge case where an empty string splits to
[""], and correctly usesRuntimeErrorfor startup validation instead ofHTTPException.
1 participant
📌 Pull Request: Add Error Handling to FastAPI App
🔧 Summary
This PR introduces robust error handling to the FastAPI application and enforces consistent LF line endings across the repository.
✅ Changes Implemented
content_typecheck).Image.open()in try/except..gitattributesto enforce LF endings.📂 Files Updated
app/main.py(FastAPI error handling logic).gitattributes(line ending enforcement)🧪 Testing
/predictendpoint rejects non‑image files.🚀 Impact
Summary by CodeRabbit
Improvements
Bug Fixes