Add --detach flag to docker model run command #231
Conversation
Reviewer's GuideThe PR introduces a new --detach/-d flag to the model run command, allowing models to be loaded in the background: it declares and registers the flag, adapts stdin-reading and execution flow for non-interactive mode, adds unit tests for the flag behavior, and updates the CLI documentation with flag details and an example. Sequence diagram for model loading with --detach flagsequenceDiagram
actor User
participant CLI
participant Model
User->>CLI: Run 'docker model run --detach <model>'
CLI->>Model: Load model into memory (no prompt)
Model-->>CLI: Model loaded
CLI-->>User: (Optional) Success message if debug enabled
CLI-->>User: Command returns immediately
Class diagram for updated Run command with detach flagclassDiagram
class RunCmd {
-backend string
-ignoreRuntimeMemoryCheck bool
-colorMode string
-detach bool
+newRunCmd()
}
RunCmd : +Flags()
RunCmd : +Run()
RunCmd : +chatWithMarkdown()
RunCmd : +handleClientError()
RunCmd : +desktopClient.Chat()
RunCmd : +os.Stdin.Stat()
RunCmd : +bufio.NewReader()
RunCmd : +io.ReadAll()
RunCmd : +cmd.Printf()
RunCmd : +MarkHidden()
RunCmd : +BoolVarP()
RunCmd : +StringVar()
RunCmd : +BoolVar()
RunCmd : +StringVar()
RunCmd : +Flags()
RunCmd : +Run()
File-Level Changes
Possibly linked issues
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 introduces a 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 a --detach flag to the Docker model run command to enable loading models in the background without user interaction, useful for pre-loading models to ensure maximum performance for subsequent requests.
- Adds
--detach(-d) flag to the run command with proper flag configuration - Implements detach mode logic that loads the model without entering interactive chat
- Updates documentation with usage examples and flag description
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/cli/commands/run.go | Implements the core detach functionality and flag definition |
| cmd/cli/commands/run_test.go | Adds comprehensive tests for the new detach flag |
| cmd/cli/docs/reference/model_run.md | Updates documentation with flag description and usage example |
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}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/run_test.go:117` </location>
<code_context>
}
}
+
+func TestRunCmdDetachFlag(t *testing.T) {
+ // Create the run command
+ cmd := newRunCmd()
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for actual command execution with --detach flag.
Please add a test that simulates running the command with --detach, verifying that model loading occurs and no interaction (prompt or output) happens. Use a mock for desktopClient to assert correct Chat invocation and output handling.
Suggested implementation:
```golang
func TestRunCmdDetachFlag(t *testing.T) {
// Create the run command
cmd := newRunCmd()
// Verify the --detach flag exists
detachFlag := cmd.Flags().Lookup("detach")
if detachFlag == nil {
t.Fatal("--detach flag not found")
}
// Verify the shorthand flag exists
detachFlagShort := cmd.Flags().ShorthandLookup("d")
if detachFlagShort == nil {
t.Fatal("shorthand -d flag not found")
}
// Mock desktopClient
type mockDesktopClient struct {
chatCalled bool
modelLoaded bool
}
mockClient := &mockDesktopClient{}
// Replace the actual desktopClient with the mock
originalDesktopClient := desktopClient
desktopClient = mockClient
defer func() { desktopClient = originalDesktopClient }()
// Implement the mock Chat method
mockChat := func(model string, prompt string, opts ...interface{}) error {
mockClient.chatCalled = true
if model == "expected-model" {
mockClient.modelLoaded = true
}
return nil
}
// Patch the Chat method if needed
// If desktopClient is an interface, assign mockChat to the method
// Otherwise, adjust as per your codebase
// Simulate running the command with --detach
cmd.SetArgs([]string{"--detach", "--model", "expected-model", "--prompt", "ignored"})
err := cmd.Execute()
if err != nil {
t.Fatalf("cmd.Execute() failed: %v", err)
}
// Assert that model loading occurred and Chat was called
if !mockClient.modelLoaded {
t.Error("model was not loaded in detached mode")
}
if !mockClient.chatCalled {
t.Error("Chat was not called in detached mode")
}
// Assert that no output or prompt was produced
// This depends on your output handling; for example, check stdout/stderr buffers if used
// If you use a custom output writer, assert it is empty here
}
```
- You may need to adjust the mock implementation to match your actual `desktopClient` type and method signatures.
- If your codebase uses dependency injection or a different way to set the client, update the assignment accordingly.
- If you capture output via buffers or custom writers, add assertions to ensure no output is produced.
- Ensure the mock `Chat` method is correctly hooked into the command execution.
</issue_to_address>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 introduces a --detach flag to the docker model run command, which is a useful feature for pre-loading models. The implementation is mostly correct, but there's a minor issue with argument handling where providing a prompt with --detach leads to the prompt being silently ignored. I've suggested adding validation to prevent this confusing behavior. Additionally, while new tests have been added for the flag, I've recommended expanding them to cover the command's behavior for better test coverage. The documentation updates are clear and well-written.
So we can load models in the background without interaction Signed-off-by: Eric Curtin <[email protected]>
ericcurtin
force-pushed
the
add-docker-model-run-detach
branch
from
a805612 to
b330b1c
Compare
| // Handle --detach flag: just load the model without interaction | ||
| if detach { | ||
| // Make a minimal request to load the model into memory | ||
| err := desktopClient.Chat(backend, model, "", apiKey, func(content string) { |
There was a problem hiding this comment.
The request will be recorded, not a big deal, but to keep in mind
There was a problem hiding this comment.
Makes me think do we have a way of forking without chatting? 🤔
| } | ||
|
|
||
| // Handle --detach flag: just load the model without interaction | ||
| if detach { |
There was a problem hiding this comment.
Couldn't this have been placed at the beginning of the function so we don't need another if !detach?
So we can load models in the background without interaction
Summary by Sourcery
Add a
--detach/-doption todocker model runto load models in the background without interaction, update command behavior, and include supporting tests and documentation.New Features:
--detach/-dflag on the run command to enable background model loading without user input.Enhancements:
--detachflag is set.Documentation:
--detachflag in the reference and provide an example for pre-loading models.Tests:
TestRunCmdDetachFlagto validate the presence, default state, type, and behavior of the--detachflag.