Add unload single model functionality #318
Conversation
WalkthroughAdds API and UI support to unload a single model: new ProcessGroup.StopProcess(modelID, strategy), a /api/models/unload/*model handler, tests updated to wait on process shutdown signals, UI API method unloadSingleModel and per-model unload controls, plus a dev proxy entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant UI as Models Page
participant API as APIProvider
participant S as /api/models/unload/*model
participant PM as ProxyManager
participant PG as ProcessGroup
participant P as Process
U->>UI: Click "Unload" on model
UI->>API: unloadSingleModel(modelId)
API->>S: POST /api/models/unload/{modelId}
S->>PM: apiUnloadSingleModelHandler (resolve real name)
PM->>PG: find group for model
PM->>PG: StopProcess(realModelName, strategy)
PG->>P: StopImmediately / Stop (per strategy)
P-->>PG: cmdWaitChan signals exit
PG-->>PM: return nil
PM-->>S: 200 OK
S-->>API: 200 OK
API-->>UI: resolved Promise
UI-->>U: UI updates model state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/pages/Models.tsx (1)
88-95: Rename the top-level control to “Unload All” to meet the PR objectiveThe objective calls for explicitly labeling the global action.
Apply this diff:
- <RiStopCircleLine size="24" /> {isUnloading ? "Unloading..." : "Unload"} + <RiStopCircleLine size="24" /> {isUnloading ? "Unloading..." : "Unload All"}
🧹 Nitpick comments (7)
ui/vite.config.ts (1)
18-18: Dev proxy addition looks goodProxying /unload to the backend aligns with the new endpoint.
For consistency, consider converging state‑changing routes under /api (e.g., POST /api/models/:id/unload) rather than mixing /api and root paths with GET.
proxy/processgroup.go (1)
89-100: Don’t hold the group lock while stopping a processHolding pg.Lock during StopImmediately can block request paths (e.g., swap lock in ProxyRequest). Unlock before stopping; optionally clear lastUsedProcess if it matches.
Apply this diff:
func (pg *ProcessGroup) StopProcess(modelID string) error { - pg.Lock() - defer pg.Unlock() - - process, exists := pg.processes[modelID] - if !exists { - return fmt.Errorf("process not found for %s", modelID) - } - - process.StopImmediately() - return nil + pg.Lock() + process, exists := pg.processes[modelID] + if !exists { + pg.Unlock() + return fmt.Errorf("process not found for %s", modelID) + } + if pg.lastUsedProcess == modelID { + pg.lastUsedProcess = "" + } + pg.Unlock() + + process.StopImmediately() + return nil }proxy/proxymanager_test.go (1)
413-457: Also assert non-target models remain loadedVerify unloading model1 doesn’t affect model2 in the same group.
Apply this diff:
assert.Equal(t, proxy.processGroups[testGroupId].processes["model1"].CurrentState(), StateStopped) + assert.Equal(t, proxy.processGroups[testGroupId].processes["model2"].CurrentState(), StateReady) }proxy/proxymanager.go (1)
632-653: Use consistent error responses and proper content negotiationLeverage sendErrorResponse for errors so clients get JSON when Accept: application/json.
Apply this diff:
func (pm *ProxyManager) unloadSingleModelHandler(c *gin.Context) { requestedModel := strings.TrimPrefix(c.Param("model"), "/") realModelName, found := pm.config.RealModelName(requestedModel) if !found { - c.String(http.StatusNotFound, "Model not found") + pm.sendErrorResponse(c, http.StatusNotFound, "model not found") return } processGroup := pm.findGroupByModelName(realModelName) if processGroup == nil { - c.String(http.StatusInternalServerError, "process group not found for model %s", requestedModel) + pm.sendErrorResponse(c, http.StatusInternalServerError, fmt.Sprintf("process group not found for model %s", requestedModel)) return } if err := processGroup.StopProcess(realModelName); err != nil { - c.String(http.StatusInternalServerError, "error stopping process: %s", err.Error()) + pm.sendErrorResponse(c, http.StatusInternalServerError, fmt.Sprintf("error stopping process: %s", err.Error())) return } else { c.String(http.StatusOK, "OK") } }Also consider using a non‑GET method for this mutation (e.g., POST /api/models/:id/unload or DELETE /api/models/:id) for better REST semantics and CSRF safety.
ui/src/contexts/APIProvider.tsx (2)
193-205: Avoid GET for a state-changing operation; align under /apiPrefer a non‑GET verb under /api for consistency with unloadAllModels.
Example (assuming backend support):
await fetch(`/api/models/${model}/unload`, { method: "POST" });
226-226: Include unloadSingleModel (and connectionStatus) in useMemo depsPrevents stale closures and satisfies exhaustive-deps linting.
Add unloadSingleModel and connectionStatus to the dependency array of useMemo.
ui/src/pages/Models.tsx (1)
122-134: Show “Load” for both stopped and shutdown statesUsers likely expect to load from either state.
Apply this diff:
- {model.state === "stopped" ? ( + {model.state === "stopped" || model.state === "shutdown" ? ( <button className="btn btn--sm" onClick={() => loadModel(model.id)}> Load </button> ) : ( <button className="btn btn--sm" onClick={() => unloadSingleModel(model.id)} disabled={model.state !== "ready"} > Unload </button> )}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
proxy/processgroup.go(1 hunks)proxy/proxymanager.go(2 hunks)proxy/proxymanager_test.go(1 hunks)ui/src/contexts/APIProvider.tsx(3 hunks)ui/src/pages/Models.tsx(2 hunks)ui/vite.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
proxy/processgroup.go (1)
proxy/process.go (4)
StopImmediately(36-36)ID(40-75)p(308-317)p(321-333)
proxy/proxymanager_test.go (3)
proxy/config.go (5)
DEFAULT_GROUP_ID(18-18)AddDefaultGroupToConfig(370-412)Config(149-169)ModelConfig(20-40)GroupConfig(116-121)proxy/process.go (2)
StateStopped(24-24)StateReady(26-26)proxy/proxymanager.go (1)
New(47-130)
ui/src/pages/Models.tsx (1)
ui/src/contexts/APIProvider.tsx (1)
useAPI(240-246)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests
🔇 Additional comments (4)
proxy/proxymanager_test.go (1)
404-409: Deterministic wait on shutdown channel is a solid improvementWaiting on cmdWaitChan removes timing flakiness vs. fixed sleeps.
ui/src/contexts/APIProvider.tsx (1)
19-19: API surface update is appropriateAdding unloadSingleModel to the context interface matches UI usage.
ui/src/pages/Models.tsx (1)
40-40: Wiring unloadSingleModel into the panel is correctMatches the new API and per-model control objective.
proxy/proxymanager.go (1)
232-234: Static routes already take precedence — no reordering neededGin (via julienschmidt/httprouter) prioritizes exact/static routes over wildcard/catch‑all routes, so "/unload" will be handled by the static route regardless of registration order. "/unload/*model" matches "/unload/" and deeper paths and the router may perform a trailing‑slash redirect between forms. Leave proxy/proxymanager.go unchanged.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ui/src/contexts/APIProvider.tsx (2)
195-203: Safely encode model IDs when building the unload URL.Line 195 concatenates the raw
modelstring into the path. Any reserved characters (?,#, spaces, etc.) would corrupt the request (e.g.,model="foo?bar"turns everything after?into query parameters), and we already have real model IDs containing path separators. Please URL-encode each path segment so the backend reliably receives the intended identifier without breaking slash semantics.Apply this diff:
- const response = await fetch(`/api/models/unload/${model}`, { + const encodedModel = model + .split("/") + .map((segment) => encodeURIComponent(segment)) + .join("/"); + + const response = await fetch(`/api/models/unload/${encodedModel}`, { method: "POST", });
221-235: Keep the provider’s memoized value aligned with its dependencies.Line 226 adds
unloadSingleModelto the context value, but the dependency array on Line 233 still omits it (and it already missedconnectionStatus). If either callback/state ever changes, consumers would keep seeing stale references. Please add both dependencies so the memo stays consistent.Apply this diff:
- [models, listModels, unloadAllModels, loadModel, enableAPIEvents, proxyLogs, upstreamLogs, metrics] + [ + models, + listModels, + unloadAllModels, + unloadSingleModel, + loadModel, + enableAPIEvents, + proxyLogs, + upstreamLogs, + metrics, + connectionStatus, + ]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
proxy/processgroup.go(1 hunks)proxy/proxymanager.go(0 hunks)proxy/proxymanager_api.go(3 hunks)proxy/proxymanager_test.go(1 hunks)ui/src/contexts/APIProvider.tsx(4 hunks)ui/src/pages/Models.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- proxy/proxymanager.go
🚧 Files skipped from review as they are similar to previous changes (2)
- proxy/proxymanager_test.go
- ui/src/pages/Models.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
proxy/proxymanager_api.go (2)
proxy/proxymanager.go (1)
ProxyManager(27-45)proxy/process.go (1)
StopImmediately(36-36)
proxy/processgroup.go (1)
proxy/process.go (2)
StopStrategy(33-33)StopImmediately(36-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests
🔇 Additional comments (1)
proxy/processgroup.go (1)
89-110: LGTM – per-model stop logic is thread-safe and aligns with existing strategies.Locking, clearing
lastUsedProcess, and delegating to the appropriate stop method mirror the all-process path and should behave correctly for both immediate and graceful strategies.
|
@mostlygeek Suggest to rename top button to "Unload All" |
/unload/:model_idto unload a specific model.Fixes #312
Summary by CodeRabbit
New Features
Tests
Chores
Documentation