fix(uninstall): delete all installed host models - #8919
Conversation
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe uninstall flow now inventories all local Ollama models and removes them when ChangesHost model cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The uninstall behavior now removes all reported host models and eligible cache data while preserving credentials and shared stores; no actionable merge-blocking risk remains after normal checks and review. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 12b1048 in the TypeScript / code-coverage/cliThe overall coverage in commit 12b1048 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-8919.docs.buildwithfern.com/nemoclaw |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/smoke-macos-install.sh`:
- Line 62: Update the --delete-models help description in
scripts/smoke-macos-install.sh to say “non-credential Hugging Face cache data,”
clarifying that credentials are preserved. Update the public help-output
assertion in test/uninstall.test.ts to require the same wording.
In `@src/lib/actions/root-help.ts`:
- Around line 84-86: Update the --delete-models description in
src/lib/actions/root-help.ts (lines 84-86), the planned cleanup description in
src/commands/internal/uninstall/plan.ts (line 23), and the runtime cleanup
description in src/commands/internal/uninstall/run-plan.ts (line 33) to state
that Hugging Face cache data is deleted while authentication files remain,
matching the confirmation prompt wording.
In `@src/lib/actions/uninstall/run-plan.ts`:
- Around line 1777-1783: Add an explicit timeout to each `runtime.run`
invocation used by the Ollama removal loop in `run-plan.ts`, preserving the
existing environment and ignored-stdio options. Ensure the timeout applies
independently to every `ollama rm` model attempt, and add a regression assertion
verifying the configured timeout is passed to the command.
In `@src/lib/domain/uninstall/plan.ts`:
- Around line 110-125: The uninstall plan and executor disagree on where Hugging
Face cache cleanup occurs. In src/lib/domain/uninstall/plan.ts:110-125, move the
delete/preserve Hugging Face cache actions to the step executed by
runLocalModelRuntimeCleanup(), or alternatively keep Model stores authoritative
and update src/lib/actions/uninstall/run-plan.ts:2285-2286 to dispatch those
declared actions there; ensure the JSON plan, logs, and actual execution use the
same step.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0e022589-3add-462e-9be8-17b9cf501e51
📒 Files selected for processing (20)
docs/inference/choose-local-inference-server.mdxdocs/inference/set-up-llama-cpp.mdxdocs/manage-sandboxes/uninstall-nemoclaw.mdxdocs/reference/commands.mdxdocs/reference/host-files-and-state.mdxscripts/smoke-macos-install.shsrc/commands/internal/uninstall/plan.tssrc/commands/internal/uninstall/run-plan.tssrc/lib/actions/root-help.tssrc/lib/actions/uninstall/run-plan-dual-station.test.tssrc/lib/actions/uninstall/run-plan-local-model-profile.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/domain/uninstall/paths.test.tssrc/lib/domain/uninstall/paths.tssrc/lib/domain/uninstall/plan.test.tssrc/lib/domain/uninstall/plan.tssrc/lib/inference/local-model-profile/cleanup-path-safety.test.tssrc/lib/inference/local-model-profile/cleanup.test.tssrc/lib/inference/local-model-profile/cleanup.tstest/uninstall.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
6 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: Manual-only E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
cv
left a comment
There was a problem hiding this comment.
Product scope for #8917 is accepted. Please address these blockers before approval:
- Add a finite timeout to each
ollama rmcall and test the configured timeout. A stalled request currently blocks later cleanup and failure reporting. - Align Hugging Face cache deletion with its declared uninstall plan step. The plan places it in
Model stores, but execution runs it duringStopping services. - Update every
--delete-modelsdescription to state that Hugging Face authentication files remain. The current wording can imply that the command deletes credentials. - Refactor the conditional mock logic in
run-plan-local-model-profile.test.tssocodebase-growth-guardrailspasses.
The current CodeRabbit threads identify the affected lines. Local npm run build:cli and npm run typecheck:cli passed. Focused CLI source tests passed.
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/inference/local-model-profile/cleanup.test.ts`:
- Around line 386-394: Update the test around cleanupHuggingFaceCacheData to
create managed runtime state or another target affected by runtime cleanup, then
capture its original contents and assert they remain unchanged after the call.
Keep the existing Hugging Face cache assertions intact so the test verifies both
cache removal and runtime-cleanup exclusion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 80f4b4e1-0684-45be-98ad-002eedd8d3d3
📒 Files selected for processing (10)
docs/reference/host-files-and-state.mdxsrc/lib/actions/uninstall/run-plan-dual-station.test.tssrc/lib/actions/uninstall/run-plan-local-model-profile.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/domain/uninstall/plan.test.tssrc/lib/domain/uninstall/plan.tssrc/lib/inference/local-model-profile/cleanup-entry.tssrc/lib/inference/local-model-profile/cleanup-path-safety.test.tssrc/lib/inference/local-model-profile/cleanup.test.tssrc/lib/inference/local-model-profile/cleanup.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/actions/uninstall/run-plan-dual-station.test.ts
- src/lib/domain/uninstall/plan.test.ts
- src/lib/actions/uninstall/run-plan-local-model-profile.test.ts
- docs/reference/host-files-and-state.mdx
- src/lib/domain/uninstall/plan.ts
- src/lib/actions/uninstall/run-plan.ts
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
Summary
Previously,
uninstall --delete-modelsremoved only two statically listed Ollama models and preserved the shared Hugging Face cache. It now removes every model reported by the host's loopback Ollama inventory and deletes non-credential Hugging Face cache data after managed model runtimes stop, while preserving authentication files and shared model stores needed by sibling gateways. Inventory, path-safety, or deletion failures now make uninstall return nonzero.Related Issue
Fixes #8917
Changes
ollama listinventory pinned to127.0.0.1:11434, then attempt removal of every reported model and propagate failures.~/.cache/huggingface/only after managed local and distributed runtimes stop. Preservetokenandstored_tokens, reject unsafe cache ownership or paths, and keep shared model stores while sibling gateways remain.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/inference/choose-local-inference-server.mdx,docs/inference/set-up-llama-cpp.mdx,docs/manage-sandboxes/uninstall-nemoclaw.mdx,docs/reference/commands.mdx,docs/reference/host-files-and-state.mdxDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project integration test/uninstall.test.ts -t "uses NemoHermes branding"passed 2/2 matching tests.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Rebecca Sliter 571084+rsliter@users.noreply.github.com
Summary by CodeRabbit
New Features
--delete-modelsnow removes all locally installed Ollama models and non-credential Hugging Face cache data.Documentation