refactor(backends): standardize status reporting across all backends#702
refactor(backends): standardize status reporting across all backends#702doringeman merged 1 commit intodocker:mainfrom
Conversation
Summary of ChangesHello @doringeman, 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 improves the user experience of the 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
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
backendStatusTable, legacy values like"not running"will now be parsed asStatusErrorand shown as errors in the table; consider explicitly filtering or mapping these known legacy strings instead of treating them as generic errors. - The status parsing in
ParseStatusis tightly coupled to the four known prefixes and the exact"<prefix>: "format; if any backend accidentally deviates (e.g., extra spaces or different capitalization), it will be classified asStatusError, so it may be worth either normalizing input or adding a more defensive fallback for partially matching known statuses.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `backendStatusTable`, legacy values like `"not running"` will now be parsed as `StatusError` and shown as errors in the table; consider explicitly filtering or mapping these known legacy strings instead of treating them as generic errors.
- The status parsing in `ParseStatus` is tightly coupled to the four known prefixes and the exact `"<prefix>: "` format; if any backend accidentally deviates (e.g., extra spaces or different capitalization), it will be classified as `StatusError`, so it may be worth either normalizing input or adding a more defensive fallback for partially matching known statuses.
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/status.go:84-85` </location>
<code_context>
+ }
+
+ backends := make([]backendInfo, 0, len(backendStatus))
+ for name, statusText := range backendStatus {
+ statusType, details := inference.ParseStatus(statusText)
+
+ // Assign sort order: Running < Error < Not Installed < Installing
</code_context>
<issue_to_address>
**issue:** Parsing status text assumes all backends now emit the new standardized format; legacy or custom values will be surfaced as generic errors.
With `ParseStatus`, any backend still returning legacy or custom status strings (e.g., "not running") will now be treated as `StatusError`, with the raw string shown in DETAILS. If that reclassification would be misleading for existing callers, consider either special-casing common legacy values before calling `ParseStatus`, or extending `ParseStatus` to recognize historical formats to keep behavior consistent during the migration.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| for name, statusText := range backendStatus { | ||
| statusType, details := inference.ParseStatus(statusText) |
There was a problem hiding this comment.
issue: Parsing status text assumes all backends now emit the new standardized format; legacy or custom values will be surfaced as generic errors.
With ParseStatus, any backend still returning legacy or custom status strings (e.g., "not running") will now be treated as StatusError, with the raw string shown in DETAILS. If that reclassification would be misleading for existing callers, consider either special-casing common legacy values before calling ParseStatus, or extending ParseStatus to recognize historical formats to keep behavior consistent during the migration.
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring that standardizes status reporting across all backends and improves the output of docker model status with a formatted table. The introduction of status constants and helper functions in pkg/inference/backend.go is a solid approach to ensure consistency. The changes in each backend to adopt this new standard are well-executed. I have a few minor suggestions to further improve code clarity and maintainability.
Signed-off-by: Dorin Geman <dorin.geman@docker.com>
4f81707 to
860b40d
Compare
Prettify
docker model status.E.g.,