feat(registry): honor registry mirrors for model pulls and backend do…#931
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
server.Runyou callenvconfig.RegistryMirrors()twice (forClientConfigandBackendsConfig); consider calling it once and reusing the slice to avoid redundant parsing and potential divergence if the env var changes between calls. - In
registryutil.RegistryHosts, it may be helpful to log which mirror ultimately succeeds (not just invalid ones) to make it easier to debug mirror ordering and behavior when pulls fall back to Docker Hub.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `server.Run` you call `envconfig.RegistryMirrors()` twice (for `ClientConfig` and `BackendsConfig`); consider calling it once and reusing the slice to avoid redundant parsing and potential divergence if the env var changes between calls.
- In `registryutil.RegistryHosts`, it may be helpful to log which mirror ultimately succeeds (not just invalid ones) to make it easier to debug mirror ordering and behavior when pulls fall back to Docker Hub.
## Individual Comments
### Comment 1
<location path="pkg/inference/backends/vllm/vllm.go" line_range="53" />
<code_context>
+ Config *Config // Linux-only: extra vllm args (nil = defaults)
+ LinuxBinaryPath string // Linux: custom vllm binary path
+ MetalPythonPath string // macOS ARM64: custom python path
+ RegistryMirrors []string // registry mirrors tried before registry-1.docker.io
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Registry mirrors are only wired through to the Metal backend, not the Linux vLLM backend
Options.RegistryMirrors is only passed to newMetal, so Linux vLLM flows won’t use the configured mirrors. If Linux should also respect these mirrors, thread them into newLinux (and its helpers). If mirrors are intentionally Metal-only, clarify or validate this in Options to avoid confusion about cross-platform behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request implements support for registry mirrors by introducing the MODEL_RUNNER_REGISTRY_MIRRORS environment variable and propagating these settings through the OCI distribution client and various inference backends, including llama.cpp, vLLM, and diffusers. A new utility, registryutil.RegistryHosts, was added to handle the prioritization of mirrors for Docker Hub requests. Feedback was provided regarding an efficiency issue in pkg/internal/registryutil/mirrors.go, where instantiating a new http.Client within a closure could lead to resource exhaustion; using http.DefaultClient was suggested as a more efficient alternative.
…wnloads Signed-off-by: Dorin Geman <dorin.geman@docker.com>
8aee230 to
6c7b2d6
Compare
Add
MODEL_RUNNER_REGISTRY_MIRRORSsupport so model pulls and backend image downloads try configured mirrors before falling back toregistry-1.docker.io.To test:
Notice the logs: