Skip to content
This repository was archived by the owner on Oct 6, 2025. It is now read-only.

Comments

[AIE-186] Add detection of Model Runner context#50

Merged
xenoscopic merged 5 commits intomainfrom
context-detection
May 12, 2025
Merged

[AIE-186] Add detection of Model Runner context#50
xenoscopic merged 5 commits intomainfrom
context-detection

Conversation

@xenoscopic
Copy link
Contributor

@xenoscopic xenoscopic commented May 10, 2025

This PR adds detection of the Model Runner context. It supports a variety of scenarios, including Docker Desktop, Docker Cloud, vanilla Moby, and Moby with a 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.

@xenoscopic xenoscopic requested review from doringeman and p1-0tr May 10, 2025 00:05
}

// EngineKind returns the Docker engine kind associated with the model runner.
func (c *ModelRunnerContext) EngineKind() ModelRunnerEngineKind {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@doringeman You can use this value in #48 to determine how install-runner should behave. If the kind is ModelRunnerEngineKindDesktop, then I would alias to docker desktop enable model-runner. If it's ModelRunnerEngineKindCloud or ModelRunnerEngineKindMobyManual (i.e. DMR_HOST is set) then install-runner should be a no-op and maybe print an error. The container-based logic should only occur if the kind is ModelRunnerEngineKindMoby.

Comment on lines +13 to +19
// modelRunner is the model runner context. It is initialized by the root
// command's PersistentPreRunE.
var modelRunner *desktop.ModelRunnerContext

// desktopClient is the model runner client. It is initialized by the root
// command's PersistentPreRunE.
var desktopClient *desktop.Client
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love these as globals, but because of the plugin framework, the only way to compute them properly is after the CLI is initialized, which can't be done until PreRunE (or PersistentPreRunE in our case). So they can't be passed off to subcommands' constructors. I suppose they could be replaced by accessor functions or guarded with locks, but Cobra's initialization is single-threaded and deterministic.

// Compute the URL prefix based on the associated engine kind.
var rawURLPrefix string
if kind == ModelRunnerEngineKindMoby {
rawURLPrefix = "http://localhost:12434"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@doringeman If you want to relocate this constant as part of #48, feel free. This should match whatever we expect the default installed container to listen on.

Copy link
Collaborator

@doringeman doringeman left a comment

Choose a reason for hiding this comment

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

Super nice!
Just 2 nits, and note that it currently breaks the unit tests.

xenoscopic and others added 4 commits May 12, 2025 09:49
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 <jacob.howard@docker.com>
Co-authored-by: Dorin-Andrei Geman <dorin.geman@docker.com>
Co-authored-by: Dorin-Andrei Geman <dorin.geman@docker.com>
Signed-off-by: Jacob Howard <jacob.howard@docker.com>
@xenoscopic xenoscopic force-pushed the context-detection branch from 7105999 to 679d0e3 Compare May 12, 2025 15:54
Signed-off-by: Jacob Howard <jacob.howard@docker.com>
@xenoscopic xenoscopic force-pushed the context-detection branch from 679d0e3 to 2db6008 Compare May 12, 2025 16:04
Copy link
Collaborator

@doringeman doringeman left a comment

Choose a reason for hiding this comment

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

Awesome!

@xenoscopic xenoscopic merged commit 9ef90db into main May 12, 2025
4 checks passed
@xenoscopic xenoscopic deleted the context-detection branch May 12, 2025 16:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants