feat(vllm): handle binary not found - #325
Conversation
Signed-off-by: Dorin Geman <dorin.geman@docker.com>
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 enhances the user experience when interacting with vLLM models by introducing explicit detection and handling for cases where the vLLM binary is missing. It refactors the CLI's error handling to provide more specific feedback and actionable steps to users, including a new hint for installing the vLLM runner, and ensures the backend returns an appropriate HTTP status code for this specific error condition. 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:
- In handleClientError, use errors.Is(err, vllm.StatusNotFound) instead of substring matching on err.Error() so wrapped errors are detected reliably.
- Rather than errors.Join(err, errors.New(message)), consider wrapping with fmt.Errorf("%s: %w", message, err) to produce a single chainable error and preserve unwrapping semantics.
- The v.status assignment in vLLM.Install isn’t used elsewhere—either remove it or document its purpose to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In handleClientError, use errors.Is(err, vllm.StatusNotFound) instead of substring matching on err.Error() so wrapped errors are detected reliably.
- Rather than errors.Join(err, errors.New(message)), consider wrapping with fmt.Errorf("%s: %w", message, err) to produce a single chainable error and preserve unwrapping semantics.
- The v.status assignment in vLLM.Install isn’t used elsewhere—either remove it or document its purpose to avoid confusion.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 addresses the scenario where the vLLM binary is not found, providing a more informative error message and guidance to the user. The changes involve modifying the error handling logic in several command files and adding a specific check for the vLLM binary not found status. The handleClientError function in cmd/cli/commands/utils.go is updated to include a check for the vllm.StatusNotFound error, and corresponding next steps are printed to the console. Additionally, the vllm.go file now sets the v.status to StatusNotFound.Error() when the binary is not found.
| return handleClientError(err, "Failed to list running models") | ||
| } |
There was a problem hiding this comment.
The handleNotRunningError function is being removed here. This function centralizes the handling of the notRunningErr error, which includes printing next steps to enable the Docker Model Runner. Removing it means that users will no longer see these helpful instructions when the service is not running, making it harder for them to resolve the issue. This could lead to a degraded user experience, as users might not know how to start the Docker Model Runner.
Consider keeping the handleNotRunningError function to ensure users receive guidance on how to resolve the Docker Model Runner is not running error.
| return "", handleClientError(err, "Failed to get model "+modelName) | ||
| } |
There was a problem hiding this comment.
The handleNotRunningError function is being removed here. This function centralizes the handling of the notRunningErr error, which includes printing next steps to enable the Docker Model Runner. Removing it means that users will no longer see these helpful instructions when the service is not running, making it harder for them to resolve the issue. This could lead to a degraded user experience, as users might not know how to start the Docker Model Runner.
Consider keeping the handleNotRunningError function to ensure users receive guidance on how to resolve the Docker Model Runner is not running error.
| return "", handleClientError(err, "Failed to list models") | ||
| } |
There was a problem hiding this comment.
The handleNotRunningError function is being removed here. This function centralizes the handling of the notRunningErr error, which includes printing next steps to enable the Docker Model Runner. Removing it means that users will no longer see these helpful instructions when the service is not running, making it harder for them to resolve the issue. This could lead to a degraded user experience, as users might not know how to start the Docker Model Runner.
Consider keeping the handleNotRunningError function to ensure users receive guidance on how to resolve the Docker Model Runner is not running error.
| return handleClientError(err, "Failed to list running models") | ||
| } |
There was a problem hiding this comment.
The handleNotRunningError function is being removed here. This function centralizes the handling of the notRunningErr error, which includes printing next steps to enable the Docker Model Runner. Removing it means that users will no longer see these helpful instructions when the service is not running, making it harder for them to resolve the issue. This could lead to a degraded user experience, as users might not know how to start the Docker Model Runner.
Consider keeping the handleNotRunningError function to ensure users receive guidance on how to resolve the Docker Model Runner is not running error.
| return handleClientError(err, "Failed to pull model") | ||
| } |
There was a problem hiding this comment.
The handleNotRunningError function is being removed here. This function centralizes the handling of the notRunningErr error, which includes printing next steps to enable the Docker Model Runner. Removing it means that users will no longer see these helpful instructions when the service is not running, making it harder for them to resolve the issue. This could lead to a degraded user experience, as users might not know how to start the Docker Model Runner.
Consider keeping the handleNotRunningError function to ensure users receive guidance on how to resolve the Docker Model Runner is not running error.
| return handleClientError(err, "Failed to remove model") | ||
| } |
There was a problem hiding this comment.
The handleNotRunningError function is being removed here. This function centralizes the handling of the notRunningErr error, which includes printing next steps to enable the Docker Model Runner. Removing it means that users will no longer see these helpful instructions when the service is not running, making it harder for them to resolve the issue. This could lead to a degraded user experience, as users might not know how to start the Docker Model Runner.
Consider keeping the handleNotRunningError function to ensure users receive guidance on how to resolve the Docker Model Runner is not running error.
| return handleClientError(err, "Failed to inspect model") | ||
| } |
There was a problem hiding this comment.
The handleNotRunningError function is being removed here. This function centralizes the handling of the notRunningErr error, which includes printing next steps to enable the Docker Model Runner. Removing it means that users will no longer see these helpful instructions when the service is not running, making it harder for them to resolve the issue. This could lead to a degraded user experience, as users might not know how to start the Docker Model Runner.
Consider keeping the handleNotRunningError function to ensure users receive guidance on how to resolve the Docker Model Runner is not running error.
| return handleClientError(err, "Failed to unload models") | ||
| } |
There was a problem hiding this comment.
The handleNotRunningError function is being removed here. This function centralizes the handling of the notRunningErr error, which includes printing next steps to enable the Docker Model Runner. Removing it means that users will no longer see these helpful instructions when the service is not running, making it harder for them to resolve the issue. This could lead to a degraded user experience, as users might not know how to start the Docker Model Runner.
Consider keeping the handleNotRunningError function to ensure users receive guidance on how to resolve the Docker Model Runner is not running error.
| return fmt.Errorf("%w\n%s", err, strings.TrimRight(buf.String(), "\n")) | ||
| } | ||
| return err | ||
| return errors.Join(err, errors.New(message)) |
There was a problem hiding this comment.
The order of arguments in errors.Join is important. The error being checked should come first, followed by the message. This ensures that the original error is checked before adding the new message.
| return errors.Join(err, errors.New(message)) | |
| return errors.Join(err, errors.New(message)) |
| } else if errors.Is(err, vllm.StatusNotFound) { | ||
| http.Error(w, err.Error(), http.StatusPreconditionFailed) |
There was a problem hiding this comment.
Returning the error message directly to the client might expose internal implementation details or sensitive information. It's generally better to return a generic error message and log the specific error on the server side.
| } else if errors.Is(err, vllm.StatusNotFound) { | |
| http.Error(w, err.Error(), http.StatusPreconditionFailed) | |
| } else if errors.Is(err, vllm.StatusNotFound) { | |
| http.Error(w, "vLLM binary not found", http.StatusPreconditionFailed) |
2 participants
Record a
vLLM binary not foundstatus and show a hint ondocker model run.This can happen when somebody attempts to use a model for vLLM with DMR with the llama.cpp backend installed only with Docker CE.
The next improvement is to add a
--backend vllmflag todocker model install-runner.