cli: prompt and install missing backend during model run#738
cli: prompt and install missing backend during model run#738iamanishx wants to merge 3 commits intodocker:mainfrom
Conversation
Signed-off-by: Manish Biswal <manishbiswal754@gmail.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
PromptInstallBackend, consider reading fromcmd.InOrStdin()instead ofos.Stdinso the prompt behaves correctly in non-interactive contexts and is easier to test. - In
EnsureBackendAvailable, comparingerr.Error()to the literal string"backend installation cancelled"is brittle; introduce a sentinel error (e.g.,var ErrBackendInstallCancelled = errors.New(...)) and compare against that instead. - In
GetRequiredBackendFromModelInfo, defaulting tollamacppwhen the config type assertion fails or the format is unknown may hide misconfigurations; consider returning an explicit error in those cases so callers can decide how to handle unsupported or unexpected formats.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PromptInstallBackend`, consider reading from `cmd.InOrStdin()` instead of `os.Stdin` so the prompt behaves correctly in non-interactive contexts and is easier to test.
- In `EnsureBackendAvailable`, comparing `err.Error()` to the literal string `"backend installation cancelled"` is brittle; introduce a sentinel error (e.g., `var ErrBackendInstallCancelled = errors.New(...)`) and compare against that instead.
- In `GetRequiredBackendFromModelInfo`, defaulting to `llamacpp` when the config type assertion fails or the format is unknown may hide misconfigurations; consider returning an explicit error in those cases so callers can decide how to handle unsupported or unexpected formats.
## Individual Comments
### Comment 1
<location path="cmd/cli/commands/utils.go" line_range="306" />
<code_context>
+ return strings.HasPrefix(state, "installed") || strings.HasPrefix(state, "running"), nil
+}
+
+func PromptInstallBackend(backend string, cmd *cobra.Command) (bool, error) {
+ fmt.Fprintf(cmd.OutOrStdout(), "Backend %q is not installed. Download and install it now? [Y/n]: ", backend)
+
+ reader := bufio.NewReader(os.Stdin)
+ input, err := reader.ReadString('\n')
+ if err != nil {
</code_context>
<issue_to_address>
**suggestion:** Use Cobra’s input stream instead of os.Stdin to respect command I/O redirection
PromptInstallBackend reads directly from os.Stdin. In Cobra commands, prefer cmd.InOrStdin() so redirected input, tests, and non-interactive use work correctly. Please use cmd.InOrStdin() instead of os.Stdin here.
```suggestion
reader := bufio.NewReader(cmd.InOrStdin())
```
</issue_to_address>
### Comment 2
<location path="cmd/cli/commands/utils.go" line_range="344" />
<code_context>
+
+ if !confirm {
+ cmd.Printf("Run 'docker model install-runner --backend %s' to install it manually.\n", backend)
+ return fmt.Errorf("backend installation cancelled")
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid comparing error strings; use a sentinel error value instead
The caller currently checks this error via `err.Error() == "backend installation cancelled"`, which is brittle because any change to the message will break the comparison. Define a package-level sentinel (e.g. `var ErrBackendInstallCancelled = errors.New("backend installation cancelled")`) and have the caller use `errors.Is` instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| func PromptInstallBackend(backend string, cmd *cobra.Command) (bool, error) { | ||
| fmt.Fprintf(cmd.OutOrStdout(), "Backend %q is not installed. Download and install it now? [Y/n]: ", backend) | ||
|
|
||
| reader := bufio.NewReader(os.Stdin) |
There was a problem hiding this comment.
suggestion: Use Cobra’s input stream instead of os.Stdin to respect command I/O redirection
PromptInstallBackend reads directly from os.Stdin. In Cobra commands, prefer cmd.InOrStdin() so redirected input, tests, and non-interactive use work correctly. Please use cmd.InOrStdin() instead of os.Stdin here.
| reader := bufio.NewReader(os.Stdin) | |
| reader := bufio.NewReader(cmd.InOrStdin()) |
|
|
||
| if !confirm { | ||
| cmd.Printf("Run 'docker model install-runner --backend %s' to install it manually.\n", backend) | ||
| return fmt.Errorf("backend installation cancelled") |
There was a problem hiding this comment.
issue (bug_risk): Avoid comparing error strings; use a sentinel error value instead
The caller currently checks this error via err.Error() == "backend installation cancelled", which is brittle because any change to the message will break the comparison. Define a package-level sentinel (e.g. var ErrBackendInstallCancelled = errors.New("backend installation cancelled")) and have the caller use errors.Is instead.
![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 enhances the model run command to automatically detect and offer to install missing backends. The implementation correctly inspects local and remote model metadata to determine the required backend. The core logic for checking, prompting, and installing backends is well-encapsulated in new utility functions.
My review focuses on improving error handling robustness, reducing coupling between CLI commands, and ensuring code consistency. I've identified a few areas where the code can be made more maintainable and resilient to future changes.
| if err.Error() == "backend installation cancelled" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Comparing error strings with err.Error() == "..." is brittle and considered a poor practice. If the error message changes in the future, this check will silently fail. A more robust approach is to use a sentinel error.
Please define a public sentinel error in the commands package (e.g., in utils.go):
// In cmd/cli/commands/utils.go
var ErrBackendInstallationCancelled = errors.New("backend installation cancelled")Then, update EnsureBackendAvailable to return this error:
// In cmd/cli/commands/utils.go, line 344
return ErrBackendInstallationCancelledFinally, you can use errors.Is for a type-safe check here.
| if err.Error() == "backend installation cancelled" { | |
| return nil | |
| } | |
| if errors.Is(err, ErrBackendInstallationCancelled) { | |
| return nil | |
| } |
| func InstallBackend(backend string, cmd *cobra.Command) error { | ||
| installCmd := newInstallRunner() | ||
| installCmd.SetArgs([]string{"--backend", backend}) | ||
|
|
||
| if err := installCmd.Execute(); err != nil { | ||
| return fmt.Errorf("failed to install backend %s: %w", backend, err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The InstallBackend function executes the install-runner command programmatically. This creates a tight coupling between the run and install-runner commands, making the code harder to maintain and test. For instance, install-runner may print to standard output, which might not be desirable in all contexts where InstallBackend is called.
A better design would be to extract the core installation logic from the install-runner command's RunE function (likely from runInstallOrStart in install-runner.go) into a separate, exported function. This new function could then be called directly by both InstallBackend and the install-runner command, removing the coupling and improving modularity.
| case types.FormatGGUF: | ||
| return llamacpp.Name, nil | ||
| case types.FormatDiffusers: | ||
| return "diffusers", nil |
There was a problem hiding this comment.
The diffusers backend name is hardcoded as a string literal. This is inconsistent with how other backends like vllm and llamacpp are referenced using their respective Name constants (vllm.Name, llamacpp.Name). Using constants improves maintainability and reduces the risk of typos.
Please use the diffusers.Name constant instead. You will need to add the following import to utils.go:
import "github.com/docker/model-runner/pkg/inference/backends/diffusers"| return "diffusers", nil | |
| return diffusers.Name, nil |
doringeman
left a comment
doringeman
left a comment
There was a problem hiding this comment.
Please see func (c *Client) InstallBackend, I believe that can be used to simplify the backend installation and the checks. 🙌
Will check this out thanks |
2 participants
Summary
model runbefore pulling the model by inspecting local metadata first and falling back to remote metadatainstall-runner --backend <name>automaticallyrunning llama.cpp ...are treated as installed whilenot ...anderror ...states are treated as missingWhy
Users running safetensors models had to run a separate backend install command manually. This change keeps users in the
model runflow while still asking for explicit consent before downloading backend components.Validation
go build ./cmd/cli/...go test ./cmd/cli/commands/... -run "Install|Backend|Run"./model-cli.exe --debug run ai/functiongemma-vllm:270M \"hello\"Backend \"vllm\" is not installed. Download and install it now? [Y/n]: