Add docker model start and stop commands #216
Conversation
To start and stop the docker model runner container. Signed-off-by: Eric Curtin <eric.curtin@docker.com>
Reviewer's GuideAdd standalone start/stop support for the Docker Model Runner container by introducing controller start/stop utilities, registering new CLI commands with context validation, and updating reference documentation. Sequence diagram for starting the Docker Model Runner container via CLIsequenceDiagram
actor User
participant CLI
participant "modelRunner"
participant "desktop"
participant "standalone"
participant "DockerClient"
User->>CLI: Run `docker model start`
CLI->>modelRunner: EngineKind()
alt Desktop context
CLI->>User: Print "Standalone start not supported with Docker Desktop"
CLI->>User: Print "Use `docker desktop enable model-runner` instead"
else MobyManual context
CLI->>User: Print "Standalone start not supported with MODEL_RUNNER_HOST set"
else Supported context
CLI->>desktop: DockerClientForContext()
desktop->>DockerClient: Create client
CLI->>standalone: StartControllerContainer()
standalone->>DockerClient: FindControllerContainer()
standalone->>DockerClient: ensureContainerStarted()
standalone->>CLI: Print "Model runner container started successfully"
CLI->>User: Print success message
end
Sequence diagram for stopping the Docker Model Runner container via CLIsequenceDiagram
actor User
participant CLI
participant "modelRunner"
participant "desktop"
participant "standalone"
participant "DockerClient"
User->>CLI: Run `docker model stop`
CLI->>modelRunner: EngineKind()
alt Desktop context
CLI->>User: Print "Standalone stop not supported with Docker Desktop"
CLI->>User: Print "Use `docker desktop disable model-runner` instead"
else MobyManual context
CLI->>User: Print "Standalone stop not supported with MODEL_RUNNER_HOST set"
else Supported context
CLI->>desktop: DockerClientForContext()
desktop->>DockerClient: Create client
CLI->>standalone: StopControllerContainer()
standalone->>DockerClient: FindControllerContainer()
standalone->>DockerClient: ContainerStop()
standalone->>CLI: Print "Model runner container stopped successfully"
CLI->>User: Print success message
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @ericcurtin, 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 enhances the Docker Model Runner CLI by adding dedicated commands to start and stop the model runner container. These new commands provide users with direct control over the container's state, particularly in standalone environments where Docker Desktop or a manually configured host is not in use. The changes include the command definitions, the underlying functions for container interaction, and updated documentation to reflect the new functionality. 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
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds Docker CLI commands to start and stop the model runner container, providing basic lifecycle management functionality.
- Implements
StartControllerContainerandStopControllerContainerfunctions with container lookup and state management - Creates new CLI commands
docker model startanddocker model stopwith proper error handling for unsupported contexts - Adds comprehensive documentation files for both commands
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmd/cli/pkg/standalone/containers.go | Adds container start/stop functions with proper error handling and status reporting |
| cmd/cli/commands/start-runner.go | Implements CLI command for starting model runner containers |
| cmd/cli/commands/stop-runner.go | Implements CLI command for stopping model runner containers |
| cmd/cli/commands/root.go | Registers new start and stop commands with the CLI |
| cmd/cli/docs/reference/model_start.md | Documentation for the start command |
| cmd/cli/docs/reference/model_stop.md | Documentation for the stop command |
| cmd/cli/docs/reference/docker_model_start.yaml | YAML metadata for start command documentation |
| cmd/cli/docs/reference/docker_model_stop.yaml | YAML metadata for stop command documentation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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 introduces docker model start and docker model stop commands to manage the lifecycle of the Docker Model Runner container. The implementation adds new cobra commands, corresponding business logic in the standalone package, and documentation.
My review has identified a critical issue in the start command's implementation that will prevent it from working as intended. There's also a high-severity issue with the stop command regarding idempotency and side effects. Additionally, I've provided some medium-severity suggestions to improve code quality and maintainability in the new command files and the container management logic.
| containerID, containerName, _, err := FindControllerContainer(ctx, dockerClient) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to find model runner container: %w", err) | ||
| } | ||
| if containerID == "" { | ||
| return errors.New("model runner container not found") | ||
| } |
There was a problem hiding this comment.
The StartControllerContainer function is currently implemented in a way that will prevent it from ever starting a stopped container. It calls FindControllerContainer, which is designed to find running containers. Critically, FindControllerContainer first prunes (removes with docker rm) any stopped controller containers. This means a stopped container will be removed instead of started, and the command will always fail with "model runner container not found".
To fix this, StartControllerContainer should implement its own logic to find the container. It should list all containers (including stopped ones) using client.ContainerList with All: true and the appropriate label filters. Then, it should check the container's state. If it's already running, it can print a message and return. If it's stopped, it should start it. This function should not call FindControllerContainer.
| containerID, containerName, _, err := FindControllerContainer(ctx, dockerClient) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to find model runner container: %w", err) | ||
| } | ||
| if containerID == "" { | ||
| return errors.New("model runner container not found") | ||
| } |
There was a problem hiding this comment.
The stop command should be idempotent, meaning it should succeed even if the container is already stopped. Currently, it will return a "model runner container not found" error in that case.
Furthermore, this function calls FindControllerContainer, which has a side effect of pruning (removing) any stopped controller containers. A stop command should not remove containers.
To address these issues, StopControllerContainer should not use FindControllerContainer. Instead, it should list running containers with the correct labels. If a running container is found, it should be stopped. If no running container is found, the function should succeed without error, as the desired state (stopped) is already achieved.
| if kind := modelRunner.EngineKind(); kind == types.ModelRunnerEngineKindDesktop { | ||
| cmd.Println("Standalone start not supported with Docker Desktop") | ||
| cmd.Println("Use `docker desktop enable model-runner` instead") | ||
| return nil | ||
| } else if kind == types.ModelRunnerEngineKindMobyManual { | ||
| cmd.Println("Standalone start not supported with MODEL_RUNNER_HOST set") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This if-else if chain can be refactored into a switch statement for better readability and maintainability, especially if more engine kinds are added in the future.
Additionally, messages that inform the user why a command is not being executed should be printed to stderr rather than stdout. You can use cmd.PrintErrln() for this purpose.
| if kind := modelRunner.EngineKind(); kind == types.ModelRunnerEngineKindDesktop { | |
| cmd.Println("Standalone start not supported with Docker Desktop") | |
| cmd.Println("Use `docker desktop enable model-runner` instead") | |
| return nil | |
| } else if kind == types.ModelRunnerEngineKindMobyManual { | |
| cmd.Println("Standalone start not supported with MODEL_RUNNER_HOST set") | |
| return nil | |
| } | |
| switch modelRunner.EngineKind() { | |
| case types.ModelRunnerEngineKindDesktop: | |
| cmd.PrintErrln("Standalone start not supported with Docker Desktop\nUse `docker desktop enable model-runner` instead") | |
| return nil | |
| case types.ModelRunnerEngineKindMobyManual: | |
| cmd.PrintErrln("Standalone start not supported with MODEL_RUNNER_HOST set") | |
| return nil | |
| } |
| if kind := modelRunner.EngineKind(); kind == types.ModelRunnerEngineKindDesktop { | ||
| cmd.Println("Standalone stop not supported with Docker Desktop") | ||
| cmd.Println("Use `docker desktop disable model-runner` instead") | ||
| return nil | ||
| } else if kind == types.ModelRunnerEngineKindMobyManual { | ||
| cmd.Println("Standalone stop not supported with MODEL_RUNNER_HOST set") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This if-else if chain can be refactored into a switch statement for better readability and maintainability, especially if more engine kinds are added in the future.
Additionally, messages that inform the user why a command is not being executed should be printed to stderr rather than stdout. You can use cmd.PrintErrln() for this purpose.
| if kind := modelRunner.EngineKind(); kind == types.ModelRunnerEngineKindDesktop { | |
| cmd.Println("Standalone stop not supported with Docker Desktop") | |
| cmd.Println("Use `docker desktop disable model-runner` instead") | |
| return nil | |
| } else if kind == types.ModelRunnerEngineKindMobyManual { | |
| cmd.Println("Standalone stop not supported with MODEL_RUNNER_HOST set") | |
| return nil | |
| } | |
| switch modelRunner.EngineKind() { | |
| case types.ModelRunnerEngineKindDesktop: | |
| cmd.PrintErrln("Standalone stop not supported with Docker Desktop\nUse `docker desktop disable model-runner` instead") | |
| return nil | |
| case types.ModelRunnerEngineKindMobyManual: | |
| cmd.PrintErrln("Standalone stop not supported with MODEL_RUNNER_HOST set") | |
| return nil | |
| } |
| } else { | ||
| printer.Printf("Stopping model runner container %s...\n", containerID[:12]) | ||
| } | ||
| stopTimeout := 10 // seconds |
There was a problem hiding this comment.
The timeout value 10 is a magic number. It's better to define it as a named constant at the package level (e.g., const defaultStopTimeoutSeconds = 10) to improve readability and maintainability. The comment // seconds is helpful, but a constant name would be self-documenting.
|
@doringeman start/stop is supposed to be like install or uninstall without: docker pull and docker rmi -f if that makes sense, this is incomplete though. |
3 participants
To start and stop the docker model runner container.
Summary by Sourcery
Implement start and stop commands for the standalone Docker Model Runner container
New Features:
docker model startanddocker model stopCLI subcommandsDocumentation:
docker model startanddocker model stopcommands