Repository navigation
add dmrlet - lightweight node agent for Docker Model Runner - #627
Closed
ericcurtin wants to merge 1 commit into
Closed
ericcurtin wants to merge 1 commit into
ericcurtin wants to merge 1 commit into
Conversation
Contributor
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 integrates 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}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
pkg/dmrlet/container/manager.go,NewManagertakes acontainerdaddress but unconditionally selects a Docker CLI runtime and ignores the address, which is confusing given the daemon config and README; consider either wiring the address into an actual containerd-based runtime or renaming/removing the parameter to match the current behavior. - In
daemon.scaleUpyou ignore errors fromcontainer.NewSpecBuilderandmodelStore.GetModelPath(using_for the error), which can lead to creating containers with an empty model path or an unsupported backend; propagate or handle these errors so scaling up fails fast instead of silently misconfiguring replicas. - The daemon client’s error handling in
pkg/dmrlet/daemon/api.go(Client.Serve) assumes the error body is JSON-decodable into a string, but the server useshttp.Error(plain text), so the decode will likely fail and drop the real message; consider reading the body as raw bytes for non-200 responses and returning that content directly in the error.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `pkg/dmrlet/container/manager.go`, `NewManager` takes a `containerd` address but unconditionally selects a Docker CLI runtime and ignores the address, which is confusing given the daemon config and README; consider either wiring the address into an actual containerd-based runtime or renaming/removing the parameter to match the current behavior.
- In `daemon.scaleUp` you ignore errors from `container.NewSpecBuilder` and `modelStore.GetModelPath` (using `_` for the error), which can lead to creating containers with an empty model path or an unsupported backend; propagate or handle these errors so scaling up fails fast instead of silently misconfiguring replicas.
- The daemon client’s error handling in `pkg/dmrlet/daemon/api.go` (`Client.Serve`) assumes the error body is JSON-decodable into a string, but the server uses `http.Error` (plain text), so the decode will likely fail and drop the real message; consider reading the body as raw bytes for non-200 responses and returning that content directly in the error.
## Individual Comments
### Comment 1
<location> `pkg/dmrlet/daemon/daemon.go:590-593` </location>
<code_context>
+ return d.logAggregator.StreamLogs(context.Background(), deployment.Containers[0], lines, follow)
+}
+
+func (d *Daemon) allocatePort() int {
+ port := d.nextPort
+ d.nextPort++
+ return port
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Port allocation is not concurrency-safe and can race between Serve/scaleUp calls
allocatePort updates d.nextPort without synchronization. Serve calls it under d.mu, but scaleUp calls it without holding the lock, so concurrent calls can race and assign duplicate ports. Please protect nextPort with d.mu (or an atomic), or move port assignment under a shared lock so all callers synchronize consistently.
</issue_to_address>
### Comment 2
<location> `pkg/dmrlet/daemon/api.go:500-503` </location>
<code_context>
+ }
+ defer resp.Body.Close()
+
+ if resp.StatusCode != http.StatusOK {
+ return c.fetchStatsEndpoint(ctx, endpoint)
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Client.Serve error handling assumes JSON string body but server uses http.Error with plain text
Because the server uses http.Error for non-200 responses, the body is plain text. Decoding it as JSON into a string will usually fail and drop the real error message. Instead, read resp.Body as raw bytes and surface that content in the error (falling back to resp.Status if the body is empty), and apply the same pattern to the other client methods that ignore the error body.
</issue_to_address>
### Comment 3
<location> `README.md:427` </location>
<code_context>
+| Feature | Kubernetes | dmrlet |
+|---------|------------|--------|
+| Multi-GPU setup | Device plugins + node selectors + resource limits YAML | `dmrlet serve llama3 --gpus all` |
+| Config overhead | 50+ lines YAML minimum | Zero YAML, CLI-only |
+| Time to first inference | Minutes (pod scheduling, image pull) | Seconds (model already local) |
+| Model management | External (mount PVCs, manage yourself) | Integrated with Docker Model Runner store |
</code_context>
<issue_to_address>
**suggestion (typo):** Consider adding "of" for smoother grammar in this table entry.
Change “50+ lines YAML minimum” to “50+ lines of YAML minimum” or “at least 50 lines of YAML” for clearer grammar.
```suggestion
| Config overhead | 50+ lines of YAML minimum | Zero YAML, CLI-only |
```
</issue_to_address>
### Comment 4
<location> `README.md:502` </location>
<code_context>
+# DAEMON: running
+# SOCKET: /var/run/dmrlet.sock
+#
+# GPUS:
+# GPU 0: NVIDIA A100 80GB 81920MB (in use: llama3.2)
+# GPU 1: NVIDIA A100 80GB 81920MB (available)
</code_context>
<issue_to_address>
**issue (typo):** Typo: "GPUS" should be "GPUs".
In the status example, change the header label from "GPUS" to "GPUs" to use the correct plural form.
```suggestion
# GPUs:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces dmrlet, a new container orchestrator for AI inference. The changes are extensive, adding a new CLI tool and several backend packages for managing containers, GPUs, services, and more. The overall architecture is well-designed, with clear separation of concerns between components like the daemon, container manager, GPU allocator, and service registry.
My review focuses on improving the robustness and correctness of the implementation. I've identified a few high-priority issues, including the use of the docker CLI instead of the Go SDK which can be brittle, and some bugs in the API client related to log streaming and error handling. I've also included some medium-severity suggestions to address potential race conditions, incomplete features, and hardcoded values.
Overall, this is a great addition with a solid foundation. Addressing these points will make dmrlet more reliable and maintainable.
ericcurtin
changed the title
ericcurtin
force-pushed
the
dmrlet1
branch
20 times, most recently
from
February 11, 2026 13:23
95d5fd8 to
c83517f
Compare
ericcurtin
force-pushed
the
dmrlet1
branch
3 times, most recently
from
February 11, 2026 17:19
55e6b57 to
c59f589
Compare
ericcurtin
force-pushed
the
dmrlet1
branch
2 times, most recently
from
February 11, 2026 17:31
a2928c9 to
46b5a7a
Compare
ericcurtin
force-pushed
the
dmrlet1
branch
4 times, most recently
from
February 11, 2026 18:05
5ac840d to
a39e81e
Compare
dmrlet is a lightweight node agent that runs inference containers directly with zero YAML overhead. It provides a simple CLI to serve models: dmrlet serve ai/smollm2 # Pulls model, starts inference container, exposes OpenAI API Key features: - Reuses existing pkg/distribution for model management - containerd integration for container lifecycle - GPU detection and passthrough (NVIDIA/AMD) - Auto port allocation (30000-30999 range) - Health checking with configurable timeout - Backend auto-detection (llama-server for GGUF, vLLM for safetensors) Commands: serve, stop, list, pull, version Signed-off-by: Eric Curtin <eric.curtin@docker.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiple critical correctness issues block reliable model serving.
Review effort: Lite
Findings: 16
Open (30)
And 10 more that still need to be addressed.
What changed in this PR
Adds dmrlet, a lightweight CLI and runtime layer for serving Docker Model Runner models.
Changes:
- Adds model serving, lifecycle commands, backend detection, and health checks.
- Adds containerd/native runtimes, GPU handling, networking, and model-store integration.
- Adds daemon APIs, service registry, logging, autoscaling scaffolding, and build integration.
| File | Summary |
|---|---|
README.md |
Documents dmrlet usage and architecture. |
pkg/dmrlet/store/integration.go |
Integrates model-store lookup and paths. |
pkg/dmrlet/service/service.go |
Defines service registry APIs. |
pkg/dmrlet/runtime/process_windows.go |
Provides Windows process runtime. |
pkg/dmrlet/runtime/process_unix.go |
Provides Unix process runtime. |
pkg/dmrlet/runtime/gpu.go |
Detects runtime GPU devices. |
pkg/dmrlet/runtime/containerd.go |
Provides containerd runtime support. |
pkg/dmrlet/network/ports.go |
Allocates inference ports. |
pkg/dmrlet/models/store.go |
Wraps distribution model management. |
pkg/dmrlet/models/detect.go |
Detects backends and builds commands/images. |
pkg/dmrlet/logging/logging.go |
Provides logging APIs. |
pkg/dmrlet/inference/spec.go |
Builds inference specifications. |
pkg/dmrlet/inference/manager.go |
Manages model lifecycle. |
pkg/dmrlet/inference/health.go |
Checks inference readiness. |
pkg/dmrlet/health/health.go |
Provides daemon health APIs. |
pkg/dmrlet/gpu/gpu.go |
Provides GPU inventory and allocation APIs. |
pkg/dmrlet/daemon/daemon.go |
Implements daemon orchestration. |
pkg/dmrlet/daemon/api.go |
Implements the daemon HTTP API. |
pkg/dmrlet/container/spec.go |
Builds container specifications. |
pkg/dmrlet/container/manager.go |
Manages container lifecycle. |
pkg/dmrlet/autoscaler/scaler.go |
Defines autoscaling behavior. |
pkg/dmrlet/autoscaler/metrics.go |
Collects serving metrics. |
Makefile |
Adds dmrlet build targets. |
go.work |
Includes the dmrlet module. |
go.sum |
Updates dependency checksums. |
go.mod |
Adds runtime dependencies. |
cmd/dmrlet/main.go |
Provides the CLI entry point. |
cmd/dmrlet/go.mod |
Defines the CLI module. |
cmd/dmrlet/commands/version.go |
Implements version output. |
cmd/dmrlet/commands/stop.go |
Implements model stopping. |
cmd/dmrlet/commands/serve.go |
Implements model serving. |
cmd/dmrlet/commands/root.go |
Configures the CLI. |
cmd/dmrlet/commands/pull.go |
Implements model pulling. |
cmd/dmrlet/commands/list.go |
Implements model listing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @VERSION=$$(git describe --tags --always --dirty 2>/dev/null || echo "dev"); \ | ||
| GIT_COMMIT=$$(git rev-parse HEAD 2>/dev/null || echo "unknown"); \ | ||
| BUILD_DATE=$$(date -u +"%Y-%m-%dT%H:%M:%SZ" 2>/dev/null || echo "unknown"); \ | ||
| cd cmd/dmrlet && CGO_ENABLED=0 go build -ldflags="-s -w -X 'main.Version=$${VERSION}' -X 'main.GitCommit=$${GIT_COMMIT}' -X 'main.BuildDate=$${BUILD_DATE}'" -o dmrlet . |
Comment on lines
+23
to
+25
| // NewScaler creates a new scaler. | ||
| func NewScaler(serviceRegistry interface{}, daemon interface{}) Scaler { | ||
| return &MockScaler{} |
Comment on lines
+318
to
+320
| // Add GPU support for NVIDIA | ||
| if _, ok := opts.GPUEnvVars["NVIDIA_VISIBLE_DEVICES"]; ok { | ||
| args = append(args, "--gpus", "all") |
| Command: append(opts.Command, opts.Args...), // Combine command and args | ||
| Env: []string{}, | ||
| Mounts: []containerd_runtime.Mount{}, | ||
| HostNet: false, |
Comment on lines
+189
to
+200
| // Model path | ||
| if b.config.ModelPathArg != "" && opts.ModelPath != "" { | ||
| modelFile := "/models" // Default value | ||
| // For llama.cpp, we need to specify the actual model file | ||
| if b.config.Backend == BackendLlamaCpp { | ||
| // Find the actual model file in the model path | ||
| foundFile := findModelFileWithValidation(opts.ModelPath) | ||
| if foundFile == "" { | ||
| // Since this function can't return an error, we'll log it and return an empty slice | ||
| // In a real implementation, this would be handled differently | ||
| return []string{} | ||
| } |
Comment on lines
+147
to
+149
| // Sanitize inputs to prevent path traversal | ||
| safeName := sanitizePathComponent(name) | ||
| safeTag := sanitizePathComponent(tag) |
Comment on lines
+443
to
+449
| **Start the daemon:** | ||
| ```bash | ||
| # Start in foreground | ||
| dmrlet daemon | ||
|
|
||
| # With custom socket path | ||
| dmrlet daemon --socket /tmp/dmrlet.sock |
Comment on lines
+461
to
+465
| dmrlet serve llama3.2 --gpus 0,1 | ||
| dmrlet serve llama3.2 --gpus all | ||
|
|
||
| # Multiple replicas | ||
| dmrlet serve llama3.2 --replicas 2 |
Comment on lines
+472
to
+476
| **List running models:** | ||
| ```bash | ||
| dmrlet ps | ||
| # MODEL BACKEND REPLICAS GPUS ENDPOINTS STATUS | ||
| # llama3.2 llama.cpp 1 [0,1,2,3] localhost:30000 healthy |
| @@ -0,0 +1,237 @@ | |||
| //go:build unix | |||
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiple unresolved build, security, and runtime correctness issues remain.
Review effort: Lite
Findings: 24
Open (39)
And 19 more that still need to be addressed.
Comment on lines
+119
to
+123
| rt = runtime.NewProcessRuntime(log) | ||
| default: | ||
| rt, err = runtime.NewRuntime(ctx, | ||
| runtime.WithRuntimeLogger(log), | ||
| ) |
Comment on lines
+247
to
+251
| switch { | ||
| case strings.Contains(metricName, "requests_per_second") || | ||
| strings.Contains(metricName, "qps") || | ||
| strings.Contains(metricName, "request_count") || | ||
| strings.Contains(metricName, "requests_total"): |
Comment on lines
+18
to
+19
| BackendSGLang Backend = "sglang" | ||
| BackendDiffusers Backend = "diffusers" |
Comment on lines
+42
to
+44
| // Remove existing socket file | ||
| if err := os.RemoveAll(s.socket); err != nil { | ||
| return fmt.Errorf("failed to remove existing socket: %w", err) |
Comment on lines
+122
to
+125
| // Allocate allocates GPUs based on the request. | ||
| func (a *Allocator) Allocate(req AllocationRequest) ([]GPU, error) { | ||
| // Implementation | ||
| return []GPU{}, nil |
Comment on lines
+54
to
+56
| return "docker.io/docker/model-runner:latest-vllm-cuda" | ||
| case "amd": | ||
| return "docker.io/docker/model-runner:latest-vllm-rocm" |
Comment on lines
+90
to
+94
| default: | ||
| return []string{ | ||
| "llama-server", | ||
| "--model", modelPath, | ||
| "--port", fmt.Sprintf("%d", port), |
Comment on lines
+19
to
+23
| // ProcessRuntime runs inference workloads as native OS processes (for Unix/macOS). | ||
| type ProcessRuntime struct { | ||
| log *logrus.Entry | ||
| procs map[string]*managedProcess | ||
| procsMu sync.Mutex |
Comment on lines
+60
to
+64
| func (c *ConcreteChecker) Run(ctx context.Context) { | ||
| // Implementation | ||
| } | ||
|
|
||
| // Check implements the Checker interface. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical build and runtime lifecycle issues, along with multiple functional defects, remain unresolved.
Review effort: Lite
Findings: 27
Open (42)
And 22 more that still need to be addressed.
Comment on lines
+79
to
+80
| if flags.detach { | ||
| return nil |
Comment on lines
+115
to
+121
| // Check if already running | ||
| m.runningMu.RLock() | ||
| if existing, exists := m.running[containerID]; exists { | ||
| m.runningMu.RUnlock() | ||
| return existing, nil | ||
| } | ||
| m.runningMu.RUnlock() |
| // Add GPU devices | ||
| if spec.GPU != nil && spec.GPU.Type != "none" { | ||
| for _, device := range spec.GPU.Devices { | ||
| ociOpts = append(ociOpts, oci.WithDevices(device, device, "rwm")) |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiple unresolved runtime, daemon, CLI, and container integration issues remain, including a critical scaling deadlock.
Review effort: Lite
Findings: 28
Open (43)
And 23 more that still need to be addressed.
Comment on lines
+443
to
+447
| d.mu.Lock() | ||
| port := d.allocatePort() | ||
| idx := len(deployment.Containers) | ||
| containerID := fmt.Sprintf("%s-%d-%s", sanitizeID(model), idx, generateRandomID()) | ||
| d.mu.Unlock() |
Contributor
Author
|
Closing, conflicts and the direction has moved on. Thanks for the reviews! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dmrlet is a "Kubelet for AI" that runs inference containers directly
with zero YAML overhead. It provides a simple CLI to serve models:
dmrlet serve ai/smollm2
Pulls model, starts inference container, exposes OpenAI API
Key features:
Commands: serve, stop, list, pull, version