Conversation
Remove dead code for the llama.cpp Docker Compose service that was replaced by SGLang. This includes: - docker-compose.yml.tmpl: Remove inference-engine service block - config.yml.tmpl: Remove 12 inference_* config fields - manager.go: Remove DefaultInference* constants, Inference* struct fields, EnableInferenceEngine toggle, and validation block - up.go/root.go: Remove --enable-inference-engine CLI flag - daemon/api_types.go, handlers.go: Remove EnableInferenceEngine from API - manager_test.go: Update tests to remove llama.cpp references The SGLang inference engine (silo inference up/down) remains unchanged. Agent-thread: https://ampcode.com/threads/T-019c2b39-fff2-70df-b334-fc8a0b56c719 Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019c2b39-fff2-70df-b334-fc8a0b56c719
WalkthroughThis PR removes the inference-engine configuration, related defaults, validation, CLI flag and API propagation, and the conditional inference service from docker-compose. It adds top-level toggles and config for a proxy agent and a deep-research service (EnableProxyAgent, EnableDeepResearch, DeepResearch*, SearchProvider, PerplexityAPIKey) and introduces a new SGLang inference configuration block in the templates. Tests, docs, and config manager were updated to reflect the removed inference fields and the new service-oriented configuration surface. Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI
participant Daemon as Daemon
participant Docker as Docker Compose
participant Proxy as Silo Proxy Agent
participant Deep as Deep Research
participant SGLang as SGLang Inference
rect rgba(200,255,200,0.5)
Note over User,CLI: New flow (after changes)
User->>CLI: run up / install
CLI->>Daemon: up request (no enable_inference_engine)
Daemon->>Daemon: generate config (EnableProxyAgent / EnableDeepResearch, sglang block)
Daemon->>Docker: start services
Docker->>Proxy: conditionally launch silo-proxy-agent
Docker->>Deep: launch deep-research service
Docker->>SGLang: launch sglang inference service (if enabled)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ment - Remove deprecated inference_* config fields from Config struct - Remove llama.cpp-related config.yml schema fields - Add Deep Research Deployment section documenting: - GHCR vs Docker Hub registry differences - SHA-based vs semver versioning - Update flow for propagating changes - GHCR authentication requirements - Graceful pull handling Agent-thread: https://ampcode.com/threads/T-019c2ba5-483b-743c-91b9-47a419fab06b Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019c2ba5-483b-743c-91b9-47a419fab06b
Amp-Thread-ID: https://ampcode.com/threads/T-019c315f-d90c-740d-9d11-9a1388ee5306 Co-authored-by: Amp <amp@ampcode.com> # Conflicts: # AGENTS.md # internal/assets/config.yml.tmpl # internal/config/manager.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/config/manager.go (1)
289-302:⚠️ Potential issue | 🟠 Major
UpdateDeepResearchImagereturns an error on the success path.Line 301 returns
true, fmt.Errorf(...)when the update succeeds. Callers checkingif err != nilwill treat a successful image update as a failure. This should return anilerror (and log or return the old image info separately).Proposed fix
- return true, fmt.Errorf("updated from %s", oldImage) + return true, nil#!/bin/bash # Check how UpdateDeepResearchImage is called to assess impact rg -n 'UpdateDeepResearchImage' --type=go -C5
🧹 Nitpick comments (1)
internal/config/manager.go (1)
304-327: No validation forDeepResearchfields whenEnableDeepResearchis true.The proxy agent block (lines 319-323) validates
ProxyServerURLwhen enabled, but there is no equivalent validation for deep research fields (DeepResearchImage,DeepResearchPort). Consider adding symmetric validation to catch misconfiguration early.Proposed addition
// Proxy agent validation (only when enabled) if config.EnableProxyAgent { if config.ProxyServerURL == "" { return fmt.Errorf("proxy_server_url cannot be empty when proxy agent is enabled") } } + // Deep research validation (only when enabled) + if config.EnableDeepResearch { + if config.DeepResearchImage == "" { + return fmt.Errorf("deep_research_image cannot be empty when deep research is enabled") + } + if config.DeepResearchPort < 1 || config.DeepResearchPort > 65535 { + return fmt.Errorf("deep_research_port must be between 1 and 65535") + } + } + return nil
| DefaultEnableProxyAgent = false | ||
| DefaultEnableDeepResearch = true |
There was a problem hiding this comment.
DefaultEnableDeepResearch = true will be impossible to override to false via config file due to mergeConfigs logic.
The isZeroValue helper (line 246) treats false as the zero value for booleans. When LoadOrDefault merges an existing config with defaults, any enable_deep_research: false set by the user will be detected as "zero" and silently replaced with the default true. This means users cannot disable deep research once it's enabled by default.
The same issue affects SGLang.TrustRemoteCode (default true) and SGLang.DisableRadixCache (default true).
Possible fixes:
- Change the merge strategy for bools to always prefer the existing config's value (i.e., never treat a bool as "missing").
- Use
*bool(pointer) for toggles so thatnilrepresents "not set" vs explicitfalse. - Track which fields were explicitly present in the YAML before merging.
Option 1: Always prefer existing bool values in merge
// mergeStructFields recursively merges struct fields
func mergeStructFields(existing, defaults, result reflect.Value) {
for i := 0; i < existing.NumField(); i++ {
existingField := existing.Field(i)
defaultField := defaults.Field(i)
resultField := result.Field(i)
if !resultField.CanSet() {
continue
}
// For nested structs, merge fields recursively
if existingField.Kind() == reflect.Struct {
mergeStructFields(existingField, defaultField, resultField)
continue
}
+ // Bools have no distinguishable "unset" zero value;
+ // always keep whatever the existing config says.
+ if existingField.Kind() == reflect.Bool {
+ resultField.Set(existingField)
+ continue
+ }
+
if isZeroValue(existingField) {
resultField.Set(defaultField)
} else {
resultField.Set(existingField)
}
}
}Note: Option 1 means a brand-new config file (all bools at Go zero = false) would not pick up true defaults, so you'd need to ensure NewDefaultConfig is used for fresh installs rather than going through the merge path. The existing code already does this (line 185), but verify the full flow.
#!/bin/bash
# Verify how LoadOrDefault handles fresh install vs existing config
# Check if there's a code path where mergeConfigs is called for a brand-new config
ast-grep --pattern $'func LoadOrDefault($_, $_) ($_, $_) {
$$$
}'
2 participants
Summary
Remove dead code for the llama.cpp Docker Compose service that was replaced by SGLang.
Changes
inference-engineservice block (llama.cpp image)inference_*config +enable_inference_engineDefaultInference*constants, 12Inference*struct fields,EnableInferenceEnginefield, validation blockenableInferenceEnginevariable--enable-inference-engineflag and assignmentEnableInferenceEnginefromUpRequestEnableInferenceEngineassignmentNet: -158 lines of dead code
What's NOT changed
silo inference up/down) - still workssilo upgrade- unaffected--allflag forsilo up/down- still worksTesting
make build✅make test✅ (config tests pass; pre-existing failures in logger/inference unrelated)Summary by CodeRabbit
Chores
New Features
Documentation