Add openaiurl flag to list and run commands #530
Conversation
Summary of ChangesHello @ericcurtin, 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 significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
listModelsFromOpenAI, the model filtering logic now usesstrings.Contains(m.ID, modelFilter)instead of the existingnormalizeModelFilter/matchesModelFilterhelpers, which changes semantics from the local listing path; consider reusing the same helpers so that filtering behaves consistently across backends. listModelsFromOpenAItakes a concrete*desktop.OpenAIClientwhile the run path was refactored to depend on thedesktop.ChatClientinterface; consider accepting an interface here as well for consistency and easier testing/mocking.- For OpenAI model listing, errors are wrapped with
fmt.Errorfwhile local listing still useshandleClientError; if appropriate, route OpenAI list errors through the same helper (or a parallel one) to keep error formatting and handling consistent across backends.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `listModelsFromOpenAI`, the model filtering logic now uses `strings.Contains(m.ID, modelFilter)` instead of the existing `normalizeModelFilter`/`matchesModelFilter` helpers, which changes semantics from the local listing path; consider reusing the same helpers so that filtering behaves consistently across backends.
- `listModelsFromOpenAI` takes a concrete `*desktop.OpenAIClient` while the run path was refactored to depend on the `desktop.ChatClient` interface; consider accepting an interface here as well for consistency and easier testing/mocking.
- For OpenAI model listing, errors are wrapped with `fmt.Errorf` while local listing still uses `handleClientError`; if appropriate, route OpenAI list errors through the same helper (or a parallel one) to keep error formatting and handling consistent across backends.
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/list.go:41-42` </location>
<code_context>
+ modelFilter = args[0]
+ }
+
+ // If --openai URL is provided, connect to external OpenAI endpoint
+ if useOpenAI {
+ openaiClient := desktop.NewOpenAIClient(openaiURL)
+ models, err := listModelsFromOpenAI(openaiClient, modelFilter)
</code_context>
<issue_to_address>
**suggestion:** Quiet/JSON flags are ignored in the OpenAI listing path, leading to inconsistent behavior with the local listing path.
In this branch we always print the full JSON returned by `listModelsFromOpenAI` and exit, so `--quiet` and `--json` don’t affect formatting as they do in the local listing path. Please either wire these flags through for the OpenAI case or make it explicit (e.g., in help text) that OpenAI listing always returns the raw JSON payload.
Suggested implementation:
```golang
// If --openai URL is provided, connect to external OpenAI endpoint.
// OpenAI listing returns the raw JSON payload from the endpoint.
if useOpenAI {
// Make the behavior explicit: OpenAI model listing only supports JSON output.
// This ensures --json is not silently ignored and the behavior matches the flag.
if !json {
return fmt.Errorf("listing models from an OpenAI endpoint currently only supports --json output; the raw JSON payload from the endpoint is returned unmodified")
}
openaiClient := desktop.NewOpenAIClient(openaiURL)
models, err := listModelsFromOpenAI(openaiClient, modelFilter)
if err != nil {
return err
}
// The JSON payload from OpenAI is already machine‑readable; forward it directly.
fmt.Fprint(cmd.OutOrStdout(), models)
return nil
}
```
1. This edit assumes the JSON flag is stored in a boolean variable named `json` in this scope (e.g., bound via `cmd.Flags().BoolVar(&json, "json", ...)`). If your variable name differs (e.g., `asJSON`, `jsonOutput`), update the condition `if !json { ... }` accordingly.
2. Consider updating the command help text for the `--openai` flag (where the Cobra flag is defined) to mention: “When used with --openai, the model listing always returns the raw JSON payload from the OpenAI endpoint and requires --json; --quiet is not supported.”
</issue_to_address>
### Comment 2
<location> `cmd/cli/commands/list.go:91-100` </location>
<code_context>
- if matchesModelFilter(m.ID, filter) {
- filtered = append(filtered, m)
- }
+func listModelsFromOpenAI(openaiClient *desktop.OpenAIClient, modelFilter string) (string, error) {
+ models, err := openaiClient.ListModels()
+ if err != nil {
+ return "", fmt.Errorf("failed to list models: %w", err)
+ }
+ if modelFilter != "" {
+ filtered := models.Data[:0]
+ for _, m := range models.Data {
+ if strings.Contains(m.ID, modelFilter) {
+ filtered = append(filtered, m)
}
- models.Data = filtered
}
- return formatter.ToStandardJSON(models)
+ models.Data = filtered
}
+ return formatter.ToStandardJSON(models)
+}
</code_context>
<issue_to_address>
**issue:** Error handling for OpenAI model listing no longer uses the shared `handleClientError` helper, which may change UX and diagnostics.
This path used to call `desktopClient.ListOpenAI()` and route failures through `handleClientError`, which standardizes auth/connection errors into consistent CLI messages. The new `listModelsFromOpenAI` instead wraps errors with `fmt.Errorf("failed to list models: %w", err)`, which may expose raw client errors and diverge from the rest of the CLI. Consider reusing `handleClientError` (or an equivalent) here to keep error handling consistent.
</issue_to_address>
### Comment 3
<location> `cmd/cli/commands/list.go:96-99` </location>
<code_context>
+ if err != nil {
+ return "", fmt.Errorf("failed to list models: %w", err)
+ }
+ if modelFilter != "" {
+ filtered := models.Data[:0]
+ for _, m := range models.Data {
+ if strings.Contains(m.ID, modelFilter) {
+ filtered = append(filtered, m)
}
</code_context>
<issue_to_address>
**question (bug_risk):** OpenAI model filtering semantics changed from normalized/tag-aware matching to simple substring matching.
The previous logic reused `normalizeModelFilter`/`matchesModelFilter` to support repo/tag patterns and exact matches. This new path uses `strings.Contains(m.ID, modelFilter)`, which is looser and may match unintended models with similar prefixes. To keep behavior consistent and avoid accidental matches, consider reusing the existing normalization/matching helpers or at least require an exact ID match here.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the list and run commands to support OpenAI-compatible endpoints by changing the --openai flag from a boolean to a URL string. The introduction of the ChatClient interface is a great improvement for abstraction and testability. The logic for handling OpenAI endpoints is cleanly separated. My main feedback is a suggestion to improve the consistency and safety of the model filtering logic in list.go to prevent potential side effects from in-place slice modifications.
ericcurtin
force-pushed
the
openaiurl
branch
5 times, most recently
from
8155caa to
106cde4
Compare
|
This is like a reduced version of: I find this useful for testing various backends, llamacpp, vllm, etc. |
ericcurtin
force-pushed
the
openaiurl
branch
from
106cde4 to
9913d03
Compare
ericcurtin
changed the title
doringeman
left a comment
doringeman
left a comment
There was a problem hiding this comment.
LGTM! Why empty fields in the table though?
$ docker model ls --openaiurl http://localhost:8080/v1
MODEL NAME CREATED
ai/llama3.2:latest 9 months ago
cmd/cli/commands/list.go
Outdated
|
|
||
| var buf bytes.Buffer | ||
| table := newTable(&buf) | ||
| table.Header([]string{"MODEL NAME", "PARAMETERS", "QUANTIZATION", "ARCHITECTURE", "MODEL ID", "CREATED", "CONTEXT", "SIZE"}) |
There was a problem hiding this comment.
| table.Header([]string{"MODEL NAME", "PARAMETERS", "QUANTIZATION", "ARCHITECTURE", "MODEL ID", "CREATED", "CONTEXT", "SIZE"}) | |
| table.Header([]string{"MODEL NAME", "CREATED"}) |
cmd/cli/commands/list.go
Outdated
| "", | ||
| "", | ||
| "", | ||
| "", | ||
| "", | ||
| "", | ||
| "", |
There was a problem hiding this comment.
| "", | |
| "", | |
| "", | |
| "", | |
| "", | |
| "", | |
| "", | |
| units.HumanDuration(time.Since(time.Unix(model.Created, 0))) + " ago", |
The openaiurl flag enables users to connect to external OpenAI-compatible API endpoints for both listing models and running chat interactions. When this flag is specified, the commands will bypass the local model runner and communicate directly with the provided endpoint. The list command now supports filtering models from external endpoints, and the run command supports both single prompt mode and interactive mode with external endpoints. Signed-off-by: Eric Curtin <eric.curtin@docker.com>
ericcurtin
force-pushed
the
openaiurl
branch
from
9913d03 to
a26d7d6
Compare
|
I did implement this as you say here at one point. Then I changed my mind, there is a parsing advantage to keeping the columns consistent also. Lets go with what you suggested here. |
The openaiurl flag enables users to connect to external
OpenAI-compatible API endpoints for both listing models and running chat
interactions. When this flag is specified, the commands will bypass the
local model runner and communicate directly with the provided endpoint.
The list command now supports filtering models from external endpoints,
and the run command supports both single prompt mode and interactive
mode with external endpoints.