Add --model flag to launch command with auto-pull#674
Add --model flag to launch command with auto-pull#674ericcurtin wants to merge 1 commit intomainfrom
Conversation
The launch command now supports a --model flag to specify which model to use. When provided, the model will be automatically pulled if not available locally and preloaded in the background. Container and host apps receive the model name via the OPENAI_MODEL environment variable. For host apps, the command now verifies the endpoint is TCP-reachable and falls back to the standalone runner port if the Docker socket URL isn't accessible externally. This ensures host apps can connect to the model runner service properly. Signed-off-by: Eric Curtin <eric.curtin@docker.com>
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 upgrades 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. Changelog
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 1 issue, and left some high level feedback:
- The
ensureEndpointReachableTCP probes and HTTP health check are hard-coded to 2–3 second timeouts and ignorecmd.Context(), which could makelaunchslow or unresponsive on flaky networks; consider using a context-aware dialer / HTTP client tied tocmd.Context()so the operation can be cancelled promptly. - The background
desktopClient.Preloadgoroutine logs viacmd.PrintErrf, which isn’t obviously safe for concurrent use and may race with other command output; consider routing background logging through a dedicated logger or channel instead of writing directly from the goroutine.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ensureEndpointReachable` TCP probes and HTTP health check are hard-coded to 2–3 second timeouts and ignore `cmd.Context()`, which could make `launch` slow or unresponsive on flaky networks; consider using a context-aware dialer / HTTP client tied to `cmd.Context()` so the operation can be cancelled promptly.
- The background `desktopClient.Preload` goroutine logs via `cmd.PrintErrf`, which isn’t obviously safe for concurrent use and may race with other command output; consider routing background logging through a dedicated logger or channel instead of writing directly from the goroutine.
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/launch.go:390-392` </location>
<code_context>
+ // Verify it's actually the model runner by making a quick health check.
+ healthURL := "http://" + fallbackHost + "/"
+ client := &http.Client{Timeout: 3 * time.Second}
+ resp, err := client.Get(healthURL) //nolint:gosec // localhost health check
+ if err == nil {
+ resp.Body.Close()
+ cmd.PrintErrf("Using standalone runner at %s\n", fallbackHost)
+ ep.host = "http://" + fallbackHost
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Health check only verifies request success and ignores status, which could mask misconfigured services on the fallback port.
Any successful `Get` (even a 500 or an unrelated HTTP service) is currently treated as “this is the model runner,” and the endpoints are rewritten. Please at least check for a 200 (`StatusOK`) or another minimal response invariant so we don’t redirect traffic to an arbitrary process just because it’s listening on the fallback port.
Suggested implementation:
```golang
if err == nil {
conn.Close()
// Verify it's actually the model runner by making a quick health check.
healthURL := "http://" + fallbackHost + "/"
client := &http.Client{Timeout: 3 * time.Second}
resp, err := client.Get(healthURL) //nolint:gosec // localhost health check
if err == nil {
if resp.StatusCode == http.StatusOK {
resp.Body.Close()
cmd.PrintErrf("Using standalone runner at %s\n", fallbackHost)
ep.host = "http://" + fallbackHost
ep.container = "http://" + net.JoinHostPort("host.docker.internal", fallbackPort)
return nil
}
resp.Body.Close()
}
}
```
This change assumes `net/http` is already imported in `cmd/cli/commands/launch.go` (it must be, since `http.Client` is used). If it's not, add:
```go
import "net/http"
```
to the import block.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| resp, err := client.Get(healthURL) //nolint:gosec // localhost health check | ||
| if err == nil { | ||
| resp.Body.Close() |
There was a problem hiding this comment.
suggestion (bug_risk): Health check only verifies request success and ignores status, which could mask misconfigured services on the fallback port.
Any successful Get (even a 500 or an unrelated HTTP service) is currently treated as “this is the model runner,” and the endpoints are rewritten. Please at least check for a 200 (StatusOK) or another minimal response invariant so we don’t redirect traffic to an arbitrary process just because it’s listening on the fallback port.
Suggested implementation:
if err == nil {
conn.Close()
// Verify it's actually the model runner by making a quick health check.
healthURL := "http://" + fallbackHost + "/"
client := &http.Client{Timeout: 3 * time.Second}
resp, err := client.Get(healthURL) //nolint:gosec // localhost health check
if err == nil {
if resp.StatusCode == http.StatusOK {
resp.Body.Close()
cmd.PrintErrf("Using standalone runner at %s\n", fallbackHost)
ep.host = "http://" + fallbackHost
ep.container = "http://" + net.JoinHostPort("host.docker.internal", fallbackPort)
return nil
}
resp.Body.Close()
}
}This change assumes net/http is already imported in cmd/cli/commands/launch.go (it must be, since http.Client is used). If it's not, add:
import "net/http"to the import block.
![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 introduces a --model flag to the launch command, which simplifies using a specific model with an application. The changes include automatically pulling a model if it's not available locally and preloading it in the background. The model is then passed to the application via an environment variable. Additionally, the PR improves connectivity for host applications by adding a TCP reachability check for the model runner endpoint, with a fallback mechanism.
The implementation is mostly solid, but I've found a couple of areas for improvement in the new ensureEndpointReachable function. One is a correctness issue in the health check logic, and the other is a suggestion to make the code more robust.
| if err == nil { | ||
| resp.Body.Close() | ||
| cmd.PrintErrf("Using standalone runner at %s\n", fallbackHost) | ||
| ep.host = "http://" + fallbackHost | ||
| ep.container = "http://" + net.JoinHostPort("host.docker.internal", fallbackPort) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The health check for the fallback endpoint only verifies that the HTTP request doesn't produce an error, but it doesn't check the HTTP status code of the response. This could lead to incorrectly identifying a service as the model runner if another service is listening on that port. The check should validate that the response status is http.StatusOK.
Additionally, it's best practice to use defer resp.Body.Close() to ensure the response body is always closed.
if err == nil {
defer resp.Body.Close()
if resp.StatusCode == http.StatusOK {
cmd.PrintErrf("Using standalone runner at %s\n", fallbackHost)
ep.host = "http://" + fallbackHost
ep.container = "http://" + net.JoinHostPort("host.docker.internal", fallbackPort)
return nil
}
}| if !strings.Contains(host, ":") { | ||
| host = net.JoinHostPort(host, "80") | ||
| } |
There was a problem hiding this comment.
The code assumes port 80 for URLs without an explicit port. This is correct for the http scheme, but would be incorrect for https (which should be 443). To make the function more robust for future use with HTTPS endpoints, consider checking u.Scheme to determine the default port.
if !strings.Contains(host, ":") {
port := "80"
if u.Scheme == "https" {
port = "443"
}
host = net.JoinHostPort(host, port)
}1 participant
The launch command now supports a --model flag to specify which model to use. When provided, the model will be automatically pulled if not available locally and preloaded in the background. Container and host apps receive the model name via the OPENAI_MODEL environment variable.
For host apps, the command now verifies the endpoint is TCP-reachable and falls back to the standalone runner port if the Docker socket URL isn't accessible externally. This ensures host apps can connect to the model runner service properly.