-
Notifications
You must be signed in to change notification settings - Fork 79
Consolidate model-cli into model-runner monorepo #190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This commit adds detection of the Model Runner context. It supports a variety of scenarios, including Docker Desktop, Docker Cloud, vanilla Moby, and Moby with some sort of manually initialized model runner. To facilitate this, a little bit of refactoring was required to properly initialize the CLI plugin framework. The standalone mode behavior has also been tweaked to support global Docker options (such as --context) that would be unavailable without a top-level docker command. Signed-off-by: Jacob Howard <[email protected]>
Co-authored-by: Dorin-Andrei Geman <[email protected]>
Co-authored-by: Dorin-Andrei Geman <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
[AIE-186] Add detection of Model Runner context
Signed-off-by: Dorin Geman <[email protected]>
Signed-off-by: Dorin Geman <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Most systems won't have NVIDIA GPUs and this behavior better aligns with what's done in Docker Desktop. Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Emily Casey <[email protected]>
Signed-off-by: Emily Casey <[email protected]>
Add `docker model package` command
Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
This commit also adds some uninstall functionality. Signed-off-by: Jacob Howard <[email protected]>
Also add a mechanism to treat Docker Desktop as Moby for testing. Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
Co-authored-by: Dorin-Andrei Geman <[email protected]>
Signed-off-by: Jacob Howard <[email protected]>
23e461d to
4905f3e
Compare
4905f3e to
8abc90c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 95 out of 4350 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| buildkitd-flags: --debug | ||
| - | ||
| name: Validate | ||
| uses: docker/bake-action@v6 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium
Uses Step
8abc90c to
36f5995
Compare
36f5995 to
4527193
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 95 out of 4350 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Dorin Geman <[email protected]>
4527193 to
344ca1b
Compare
|
Hmm, I wonder if stripping |
Signed-off-by: Dorin Geman <[email protected]>
Signed-off-by: Dorin Geman <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 96 out of 97 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Dorin Geman <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 95 out of 96 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Reviewer's GuideThis PR merges the standalone Class diagram for ModelRunnerContext and Client in desktop packageclassDiagram
class ModelRunnerContext {
+kind: ModelRunnerEngineKind
+urlPrefix: *url.URL
+client: DockerHttpClient
+EngineKind()
+URL(path string) string
+Client() DockerHttpClient
}
class Client {
+modelRunner: ModelRunnerContext
+Status() Status
+Pull(model string, ignoreRuntimeMemoryCheck bool, progress func(string)) (string, bool, error)
+Push(model string, progress func(string)) (string, bool, error)
+List() ([]Model, error)
+ListOpenAI(backend, apiKey string) (OpenAIModelList, error)
+Inspect(model string, remote bool) (Model, error)
+InspectOpenAI(model string) (OpenAIModel, error)
+Chat(backend, model, prompt, apiKey string, outputFunc func(string), shouldUseMarkdown bool) error
+Remove(models []string, force bool) (string, error)
+PS() ([]BackendStatus, error)
+DF() (DiskUsage, error)
+Unload(req UnloadRequest) (UnloadResponse, error)
+ConfigureBackend(request ConfigureRequest) error
+Requests(modelFilter string, streaming bool, includeExisting bool) (io.ReadCloser, func(), error)
+Tag(source, targetRepo, targetTag string) error
+LoadModel(ctx context.Context, r io.Reader) error
}
ModelRunnerContext <|-- Client
class Status {
+Running: bool
+Status: []byte
+Error: error
}
class BackendStatus {
+BackendName: string
+ModelName: string
+Mode: string
+LastUsed: time.Time
}
class DiskUsage {
+ModelsDiskUsage: int64
+DefaultBackendDiskUsage: int64
}
class UnloadRequest {
+All: bool
+Backend: string
+Models: []string
}
class UnloadResponse {
+UnloadedRunners: int
}
class ProgressMessage {
+Type: string
+Message: string
+Total: uint64
+Pulled: uint64
+Layer: Layer
}
class Layer {
+ID: string
+Size: uint64
+Current: uint64
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload. (link)
- An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload. (link)
- An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload. (link)
General comments:
- This consolidation PR touches too many areas at once—consider splitting it into smaller, focused PRs (e.g. CLI commands, desktop client, standalone container logic) to make review and testing more manageable.
- Several streaming routines (e.g. Chat and Requests) use the default bufio.Scanner buffer size; consider replacing them with a buffered reader or increasing the scanner buffer to safely handle larger payloads without truncation.
- Query‐string parameters are built by manual string concatenation in multiple places; using net/url.Values to construct URLs would be safer and avoid potential encoding issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- This consolidation PR touches too many areas at once—consider splitting it into smaller, focused PRs (e.g. CLI commands, desktop client, standalone container logic) to make review and testing more manageable.
- Several streaming routines (e.g. Chat and Requests) use the default bufio.Scanner buffer size; consider replacing them with a buffered reader or increasing the scanner buffer to safely handle larger payloads without truncation.
- Query‐string parameters are built by manual string concatenation in multiple places; using net/url.Values to construct URLs would be safer and avoid potential encoding issues.
## Individual Comments
### Comment 1
<location> `cmd/cli/docs/reference/model_status.md:17` </location>
<code_context>
+command: docker model status
+short: Check if the Docker Model Runner is running
+long: |
+ Check whether the Docker Model Runner is running and displays the current inference engine.
+usage: docker model status
+pname: docker model
</code_context>
<issue_to_address>
**issue (typo):** Change 'and displays' to 'and display' for grammatical correctness.
Update the sentence to: 'Check whether the Docker Model Runner is running and display the current inference engine.'
</issue_to_address>
### Comment 2
<location> `.github/workflows/cli-validate.yml:38` </location>
<code_context>
uses: docker/bake-action/subaction/list-targets@v6
</code_context>
<issue_to_address>
**security (yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha):** An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
*Source: opengrep*
</issue_to_address>
### Comment 3
<location> `.github/workflows/cli-validate.yml:57` </location>
<code_context>
uses: docker/setup-buildx-action@v3
</code_context>
<issue_to_address>
**security (yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha):** An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
*Source: opengrep*
</issue_to_address>
### Comment 4
<location> `.github/workflows/cli-validate.yml:62` </location>
<code_context>
uses: docker/bake-action@v6
</code_context>
<issue_to_address>
**security (yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha):** An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| ## Description | ||
|
|
||
| Check whether the Docker Model Runner is running and displays the current inference engine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (typo): Change 'and displays' to 'and display' for grammatical correctness.
Update the sentence to: 'Check whether the Docker Model Runner is running and display the current inference engine.'
| - | ||
| name: List targets | ||
| id: generate | ||
| uses: docker/bake-action/subaction/list-targets@v6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
security (yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha): An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
Source: opengrep
| uses: actions/checkout@v4 | ||
| - | ||
| name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
security (yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha): An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
Source: opengrep
| buildkitd-flags: --debug | ||
| - | ||
| name: Validate | ||
| uses: docker/bake-action@v6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
security (yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha): An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
Source: opengrep
<!--Delete sections as needed --> ## Description <!-- Tell us what you did and why --> ## Related issues or tickets * docker/model-runner#190 * docker/model-runner#195 ## Reviews <!-- Notes for reviewers here --> <!-- List applicable reviews (optionally @tag reviewers) --> - [ ] Technical review - [ ] Editorial review - [ ] Product review Signed-off-by: CrazyMax <[email protected]>
Compared to #165 (comment), this time I used
git filter-repo.Consolidated model-cli (367 commits) + b1f0a10 + a39f8ce +344ca1b + bbb7e9b + 13d99c4 = 372 commits.
Summary by Sourcery
Consolidate the standalone model-cli project into the model-runner monorepo and introduce a fully integrated Docker CLI plugin for managing, running, packaging, and monitoring AI/ML models. This PR imports the entire cmd/cli directory, context detection, standalone runner support, desktop integration, CLI commands, documentation, tests, and CI configuration.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores: