Add configure command to support Compose models implementation #106
Conversation
Signed-off-by: Jacob Howard <[email protected]>
xenoscopic
force-pushed
the
configure-command
branch
from
0d2ea89 to
c3a000b
Compare
| var opts scheduling.ConfigureRequest | ||
|
|
||
| c := &cobra.Command{ | ||
| Use: "configure [--context-size=<n>] MODEL [-- <runtime-flags...>]", |
There was a problem hiding this comment.
do we need -- here? As model is a required argument, everything after could be considered runtime flags (comparable to docker run IMAGE <command...>
There was a problem hiding this comment.
We need it so we can pass runtime flags that start with -- so they're not treated as flags for the configure command itself.
There was a problem hiding this comment.
Interesting. docker run doesn't require this and I can't see any magic code for this purpose on https://github.com/docker/cli/blob/master/cli/command/container/run.go#L36
There was a problem hiding this comment.
Yeah, but the command doesn't usually contain "--" so it's parsed as a flag.
There was a problem hiding this comment.
it's not:
$ docker run alpine/curl --help
Usage: curl [options...] <url>
-d, --data <data> HTTP POST data
...
There was a problem hiding this comment.
Since the -- terminator is a de facto standard (in GNU getopt, Cobra, Argparse, etc.) indicating the end of flag parsing, let's go with it for now and then we can potentially make it optional in the future (see, e.g., git help log, where -- is recommended but optional in certain cases).
This will give us the most flexibility later to evolve the command (e.g. supporting multiple model specifications), because honestly this command is being added with a little less design time than we'd ideally like.
For now though, it's the most unambiguous option and doesn't risk breaking in the future since it's a multi-decade convention.
There was a problem hiding this comment.
If you declare this command with this syntax this will have to stay forever, otherwise Compose integration will be broken
There was a problem hiding this comment.
I'm okay with it staying forever, it's been in use as a parsing convention for several decades now and it's baked into the APIs and CLIs of many libraries and tools. In git for example, anything taking arbitrary pathspecs uses a -- convention (because that's the GNU convention for arbitrary args). It doesn't cost anything to support it and there's no risk of support for it being removed from argument parsing libraries.
If we didn't use it though, we wouldn't have any way to (in the future) support an arbitrary number of model specs (because we'd have no way of knowing when the model specs ended and the runtime flags began). So I think leaning into the standard here gives us more flexibility with the command design. Doing a clever parsing heuristic now would give us far less flexibility in the other direction.
| ) | ||
| } | ||
| opts.Model = args[0] | ||
| opts.RuntimeFlags = args[1:] |
There was a problem hiding this comment.
-- will be part of the sub-slice
There was a problem hiding this comment.
It won't, it's parsed and excluded automatically by cobra.
Signed-off-by: Dorin Geman <[email protected]>
Signed-off-by: Dorin Geman <[email protected]>
| Args: func(cmd *cobra.Command, args []string) error { | ||
| argsBeforeDash := cmd.ArgsLenAtDash() | ||
| if argsBeforeDash == -1 { | ||
| // No "--" used, so we need exactly 1 total argument. |
There was a problem hiding this comment.
The flags are not included, they're not counted.
Signed-off-by: Jacob Howard <[email protected]>
This PR adds a
docker model configurecommand that can set runtime configuration for a model. Right now it uses the--terminator convention to delineate runtime flags.This supports the new
models:spec in Compose.This requires an update to the
github.com/docker/model-runnerdep, hence the large dependency updates; this update includes docker/model-runner#95, so I've opened it in draft for now until we can get that (and anything else we want) intomainin that repo (and then do a finalgo mod update).