Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions cmd/cli/commands/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -809,11 +809,34 @@ func newRunCmd() *cobra.Command {
return nil
}

_, err := desktopClient.Inspect(model, false)
if err != nil {
if !errors.Is(err, desktop.ErrNotFound) {
return handleClientError(err, "Failed to inspect model")
modelInfo, err := desktopClient.Inspect(model, false)
modelFoundLocally := err == nil
if err != nil && !errors.Is(err, desktop.ErrNotFound) {
return handleClientError(err, "Failed to inspect model")
}

if !modelFoundLocally {
remoteInfo, remoteErr := desktopClient.Inspect(model, true)
if remoteErr == nil {
modelInfo = remoteInfo
}
}

backend := ""
if modelInfo.ID != "" {
backend, _ = GetRequiredBackendFromModelInfo(&modelInfo)
}

if backend != "" {
if err := EnsureBackendAvailable(backend, cmd); err != nil {
if err.Error() == "backend installation cancelled" {
return nil
}
Comment on lines +832 to +834
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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 ErrBackendInstallationCancelled

Finally, you can use errors.Is for a type-safe check here.

Suggested change
if err.Error() == "backend installation cancelled" {
return nil
}
if errors.Is(err, ErrBackendInstallationCancelled) {
return nil
}

return err
}
}

if !modelFoundLocally {
cmd.Println("Unable to find model '" + model + "' locally. Pulling from the server.")
if err := pullModel(cmd, desktopClient, model); err != nil {
return err
Expand Down
116 changes: 116 additions & 0 deletions cmd/cli/commands/utils.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package commands

import (
"bufio"
"bytes"
"encoding/json"
"errors"
"fmt"
"io"
Expand All @@ -12,7 +14,10 @@ import (
"github.com/docker/model-runner/cmd/cli/desktop"
"github.com/docker/model-runner/cmd/cli/pkg/standalone"
"github.com/docker/model-runner/pkg/distribution/oci/reference"
"github.com/docker/model-runner/pkg/distribution/types"
"github.com/docker/model-runner/pkg/inference/backends/llamacpp"
"github.com/docker/model-runner/pkg/inference/backends/vllm"
dmrm "github.com/docker/model-runner/pkg/inference/models"
"github.com/moby/term"
"github.com/olekukonko/tablewriter"
"github.com/olekukonko/tablewriter/renderer"
Expand Down Expand Up @@ -270,3 +275,114 @@ func newTable(w io.Writer) *tablewriter.Table {
}),
)
}

func CheckBackendInstalled(backend string) (bool, error) {
status := desktopClient.Status()
if status.Error != nil {
return false, fmt.Errorf("failed to get backend status: %w", status.Error)
}

var backendStatus map[string]string
if err := json.Unmarshal(status.Status, &backendStatus); err != nil {
return false, fmt.Errorf("failed to parse backend status: %w", err)
}

backendState, exists := backendStatus[backend]
if !exists {
return false, nil
}

state := strings.TrimSpace(strings.ToLower(backendState))
if strings.HasPrefix(state, "not ") || strings.HasPrefix(state, "error") {
return false, nil
}

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
reader := bufio.NewReader(os.Stdin)
reader := bufio.NewReader(cmd.InOrStdin())

input, err := reader.ReadString('\n')
if err != nil {
return false, fmt.Errorf("failed to read input: %w", err)
}

input = strings.TrimSpace(strings.ToLower(input))
return input == "" || input == "y" || input == "yes", 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
}
Comment on lines +316 to +325
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.


func EnsureBackendAvailable(backend string, cmd *cobra.Command) error {
installed, err := CheckBackendInstalled(backend)
if err != nil {
return err
}

if installed {
return nil
}

confirm, err := PromptInstallBackend(backend, cmd)
if err != nil {
return err
}

if !confirm {
cmd.Printf("Run 'docker model install-runner --backend %s' to install it manually.\n", backend)
return fmt.Errorf("backend installation cancelled")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if err := InstallBackend(backend, cmd); err != nil {
return err
}

installed, err = CheckBackendInstalled(backend)
if err != nil {
return err
}
if !installed {
return fmt.Errorf("backend %q is still not installed; run 'docker model install-runner --backend %s'", backend, backend)
}

cmd.Printf("Backend %q installed successfully.\n", backend)
return nil
}

func GetRequiredBackend(model string) (string, error) {
modelInfo, err := desktopClient.Inspect(model, false)
if err != nil {
return "", err
}

return GetRequiredBackendFromModelInfo(&modelInfo)
}

func GetRequiredBackendFromModelInfo(modelInfo *dmrm.Model) (string, error) {
config, ok := modelInfo.Config.(*types.Config)
if !ok {
return llamacpp.Name, nil
}

switch config.Format {
case types.FormatSafetensors:
return vllm.Name, nil
case types.FormatGGUF:
return llamacpp.Name, nil
case types.FormatDiffusers:
return "diffusers", nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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"
Suggested change
return "diffusers", nil
return diffusers.Name, nil

default:
return llamacpp.Name, nil
}
}