-
Notifications
You must be signed in to change notification settings - Fork 79
get --runtime-args back #518
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
Summary of ChangesHello @ilopezluna, 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 addresses a critical user workflow issue by reintroducing the ability to pass raw runtime flags to the inference backends. It integrates this functionality across the CLI commands and backend configuration, ensuring that users can customize model runtime behavior as needed. The change restores previously broken workflows and enhances the flexibility of model deployment and configuration. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request reintroduces the --runtime-args flag for the compose up command and adds support for passing runtime arguments to the configure command. The changes span across command definitions, backend configurations, and documentation to support passing these flags down to the inference engines. The implementation correctly handles parsing flags from both a raw string and a string slice. I've found one critical issue where swapped arguments in an error message formatting call would lead to a panic. Otherwise, the changes look good and include necessary tests and documentation updates.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/configure.go:44` </location>
<code_context>
+ RunE: func(cmd *cobra.Command, args []string) error {
</code_context>
<issue_to_address>
**issue (bug_risk):** Runtime flags slice includes the model name and likely the literal "--"
Here `opts.RuntimeFlags = args[1:]` will still include the literal "--" when present (e.g. `docker model configure foo -- --embeddings` yields `RuntimeFlags = ["--", "--embeddings"]`). To avoid depending on positional slicing and to drop "--" explicitly, consider mirroring the `Args` logic and using `cmd.ArgsLenAtDash()`: treat `args[:argsBeforeDash]` as the model (length 1) and `args[argsBeforeDash+1:]` as runtime flags.
</issue_to_address>
### Comment 2
<location> `cmd/cli/commands/compose.go:99` </location>
<code_context>
+ if err := desktopClient.ComposeUp(ctx, compose.UpRequest{
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Error formatting string and logging call have minor issues and inconsistent formatting
The new `configErrFmtString` has a double space before `"and runtime-flags"`, and the `_ = sendErrorf(...)` line appears mis-indented compared to gofmt output. Also, the format string now has three verbs (`%s`, `%d`, `%s`); please ensure both `sendErrorf` and `fmt.Errorf` pass exactly three matching arguments.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if err != nil { | ||
| return err | ||
| } | ||
| opts.RuntimeFlags = args[1:] |
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 (bug_risk): Runtime flags slice includes the model name and likely the literal "--"
Here opts.RuntimeFlags = args[1:] will still include the literal "--" when present (e.g. docker model configure foo -- --embeddings yields RuntimeFlags = ["--", "--embeddings"]). To avoid depending on positional slicing and to drop "--" explicitly, consider mirroring the Args logic and using cmd.ArgsLenAtDash(): treat args[:argsBeforeDash] as the model (length 1) and args[argsBeforeDash+1:] as runtime flags.
c6114f2 to
39188cc
Compare
Address #515
Revert removal of
--runtime-argsas its being used by users, so when I removed in here I broke their workflows.