cli: prompt and install missing backend during model run#738
cli: prompt and install missing backend during model run#738iamanishx wants to merge 6 commits intodocker:mainfrom
Conversation
Signed-off-by: Manish Biswal <manishbiswal754@gmail.com>
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.
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.
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 |
|
@doringeman i have made the changes please have a look |
|
Fragile error string comparison, inconsistent constant usage (diffusers), missing import for diffusers package, no tests for new functions, unused GetRequiredBackend function, merge conflicts |
Signed-off-by: Manish Biswal <manishbiswal754@gmail.com>
| return nil | ||
| } | ||
|
|
||
| func GetRequiredBackendFromModelInfo(modelInfo *dmrm.Model) (string, error) { |
There was a problem hiding this comment.
Please check and use func (s *Scheduler) selectBackendForModel.
The server could expose a simple endpoint like:
GET /models/{name}/backend → {"backend": "vllm", "installed": true}
There was a problem hiding this comment.
The backend selection logic in GetRequiredBackendFromModelInfo duplicates what the server already does in Scheduler.selectBackendForModel, and the two already diverge (the server has a platform-aware fallback chain: vLLM → MLX → SGLang, while the CLI always returns vllm for safetensors). Rather than maintaining this mapping in two places, consider calling the server to determine the required backend.
|
And sorry for the delay! 🙌 |
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]: