-
Notifications
You must be signed in to change notification settings - Fork 16
add compose metadata subcommand to return information about model runner cli usage as a Compose provider #77
Conversation
2e99bb1 to
a749f65
Compare
xenoscopic
left a comment
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.
LGTM
|
@glours Happy to merge whenever you're ready, just let me know. |
|
@xenoscopic I'll let you know, I'm waiting for Compose v2.37.0 to be available via Docker Desktop module update |
|
@glours Sounds good, I'll wait for your message. |
…ner cli usage as a Compose provider Signed-off-by: Guillaume Lours <[email protected]>
a749f65 to
bb6b18a
Compare
|
@xenoscopic you can merge it, could we expect a release of the cli via desktop module soon as Compose v2.37.1 will be embedded in DD patch release which should be available today IIRC (ping me in DM if needed) |
|
I think we can do that, @glours. |
|
@doringeman thanks, because there will be one on the Compose side 😅 |
|
Let's merge / ship it for 4.43 (we can't take it back to 4.42 because of the other stuff that's gone into Model CLI since then). I will do a new CLI release today, 0.1.30, to fix the |
| var model []string | ||
| c := &cobra.Command{ | ||
| Use: "down", | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| // No required cleanup on down | ||
| return nil | ||
| }, | ||
| } | ||
| c.Flags().StringArrayVar(&model, "model", nil, "model to use") |
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.
In testing, I'm seeing things fail on docker compose down:
✘ llama3.2 exit status 1 0.0s
✘ qwen3 exit status 1 0.0s
failed to remove service provider: exit status 1
Are we sure that we should remove the model argument to down?
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.
If I add these two lines back, then compose down does seem to work correctly.
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.
Can we test this somehow in the builds?
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.
You need to use Compose v2.37.x for your test, this is link to this change docker/compose#12903
If you test with the latest Desktop version 4.42.1 you should not have the issue
|
@glours Will Compose 2.37.1 work with model providers without this change? I know this change won't work without 2.37.1, but I'm wondering if we are okay to ship without this in our Docker CE release or if we need to get this in for compatibility. |
|
@xenoscopic yes if the metadata subcommand is not present, Compose will ignore this the step |
compose metadatasubcommand will return information about parameters forupanddowncommandsIt will let Compose know that
modeloptions is required and help Docker LSP to propose a better UX.{ "description": "Docker Model Runner", "up": { "parameters": [ { "name": "model", "description": "model to use", "required": true, "type": "stringArray", "default": "[]" } ] }, "down": { "parameters": null } }