feat: add error handling for unsupported model formats in manager#192
feat: add error handling for unsupported model formats in manager#192ilopezluna merged 2 commits intomainfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR introduces an explicit check for unsupported model formats in the Manager’s create endpoint—returning a 400 Bad Request with a warning log instead of a 500—and cleans up a duplicate diskusage import. Sequence diagram for error handling in model creation (unsupported format)sequenceDiagram
participant Client
participant Manager
participant Distribution
Client->>Manager: POST /models (create model)
Manager->>Distribution: Request model pull
Distribution-->>Manager: Return ErrUnsupportedFormat
Manager->>Manager: Log warning
Manager->>Client: 400 Bad Request (unsupported format)
Class diagram for Manager error handling updateclassDiagram
class Manager {
+handleCreateModel(w, r)
}
class Distribution {
+ErrUnsupportedFormat
}
Manager --> Distribution: uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves error handling in the model manager by adding specific handling for unsupported model formats, changing the HTTP status code from 500 Internal Server Error to 400 Bad Request when attempting to pull models in unsupported formats like safetensors.
Key Changes
- Added specific error handling for
distribution.ErrUnsupportedFormatwith appropriate 400 status code - Reordered import statements for better organization
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Summary of ChangesHello @ilopezluna, 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 refines the error handling logic within the model manager. It introduces a more precise HTTP status code for scenarios where a client attempts to pull a model in an unsupported format, transitioning from a generic server error to a specific client-side error. This change enhances the clarity and correctness of API responses, allowing clients to better understand and react to invalid requests. Highlights
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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider centralizing the mapping of distribution.ErrUnsupportedFormat to HTTP Bad Request status in a shared error-handling utility so that handlers remain concise and consistent.
- It may be better to return a more user-friendly error message instead of the raw ErrUnsupportedFormat.Error() to avoid exposing internal implementation details.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing the mapping of distribution.ErrUnsupportedFormat to HTTP Bad Request status in a shared error-handling utility so that handlers remain concise and consistent.
- It may be better to return a more user-friendly error message instead of the raw ErrUnsupportedFormat.Error() to avoid exposing internal implementation details.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 correctly improves error handling for unsupported model formats by returning a 400 Bad Request instead of a 500 Internal Server Error. The change is well-implemented and provides a much clearer error message to the end-user. I have one suggestion regarding error handling consistency that could be addressed in a follow-up to further improve code quality.
| if errors.Is(err, distribution.ErrUnsupportedFormat) { | ||
| m.log.Warnf("Unsupported model format for %q: %v", request.From, err) | ||
| http.Error(w, distribution.ErrUnsupportedFormat.Error(), http.StatusBadRequest) | ||
| return | ||
| } |
There was a problem hiding this comment.
While this new error handling for ErrUnsupportedFormat is a great addition and returns a clear message to the user, it highlights an inconsistency in how errors are reported in this function. Some errors return a generic, hardcoded string (e.g., for ErrModelNotFound), while the default case returns the full wrapped err.Error().
For better consistency and an improved user experience, consider refactoring the other error handling cases in handleCreateModel to also return more specific and helpful error messages, similar to what you've done here. This could be addressed in a follow-up PR to keep this change focused.
For example, for ErrModelNotFound:
// Before
http.Error(w, "Model not found", http.StatusNotFound)
// After (suggested)
http.Error(w, err.Error(), http.StatusNotFound)This would provide more context to the user, which is a pattern already used by other handlers in this file, such as handleDeleteModel.
Co-authored-by: Dorin-Andrei Geman <dorin.geman@docker.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| if errors.Is(err, distribution.ErrUnsupportedFormat) { | ||
| m.log.Warnf("Unsupported model format for %q: %v", request.From, err) | ||
| http.Error(w, distribution.ErrUnsupportedFormat.Error(), http.StatusUnsupportedMediaType) |
There was a problem hiding this comment.
The HTTP status code should be http.StatusBadRequest (400) to match the PR description and expected behavior, not http.StatusUnsupportedMediaType (415).
| http.Error(w, distribution.ErrUnsupportedFormat.Error(), http.StatusUnsupportedMediaType) | |
| http.Error(w, distribution.ErrUnsupportedFormat.Error(), http.StatusBadRequest) |
Before:
After:
Summary by Sourcery
Handle unsupported model format errors in the manager by returning a Bad Request status and logging a warning instead of an Internal Server Error.
Bug Fixes:
Enhancements: