feat: add nanotune clean and report fused/ cache disk usage - #112
feat: add nanotune clean and report fused/ cache disk usage#112rohanshrma222 wants to merge 4 commits into
Conversation
will-lamerton
left a comment
There was a problem hiding this comment.
Thanks for this, it's a well-scoped fix for #70 and it follows the existing patterns closely (useKeyInput/useAutoExit/ExitHint, the (y/n) prompt convention, the data import style non-TTY gate, lazy command import). Capturing the size label before rmSync so "Freed:" stays accurate was a nice touch. A few things to sort before merge.
Blocking
1. The --skip-fuse guard is weaker than the problem it fixes (src/commands/export.tsx)
It only checks that the directory exists. mlx_lm.fuse creates --save-path before it finishes writing, so an interrupted or failed export leaves a present-but-incomplete fused/, and the user gets exactly the cryptic mid-GGUF-conversion failure this PR set out to replace. Please check for a real artifact inside (config.json, or any *.safetensors) rather than the bare directory.
The same check drives clean's branching, so a leftover empty fused/ currently prompts to free 0 B instead of saying "Nothing to clean". A shared hasUsableFusedModel() helper fixes both in one place.
2. getDirectorySize can throw and take nanotune status down with it (src/lib/config.ts)
It uses statSync, which follows symlinks, so a broken symlink or an unreadable entry throws. StatusCommand calls it in the render body, which turns that into a crash in a command that never touched this directory before. Either use lstatSync and skip non-regular entries, or wrap the per-entry stat in try/catch and skip failures.
3. Missing tests
src/lib/config.spec.tscovers every other function in that module, but the newformatFileSizeandgetDirectorySizehave no direct tests. Worth pinning the B/KB/MB/GB thresholds, nested directories, and the missing-directory-returns-0 path.- The interactive delete path is untested: the
--yestest starts incleaning, and the confirm test stops at the prompt. Nothing covers keypressyleading tormSync, orn/Escape exiting without deleting.commands.spec.tsxalready has auseKeyInputharness for this.
Non-blocking
cli.tsxdoesawait import('node:fs')although the file already statically imports fromnode:fsat the top. AddexistsSyncto the static import.- In
clean.tsx,errorMessageis initialised to the no-project string and then disambiguated byhasProjectat render time, so one state variable carries two meanings. Astring | nullerror state, or an early return for the no-project case, reads more clearly. - Design question worth settling before this interface is public:
nanotune cleanis a broad name for a command that removes one specific cache. There's also the base model cache (lib/model-cache.ts), usually larger thanfused/, that users will plausibly expectcleanto handle. Either scope the name or give the command a target flag. getDirectorySizeruns on everyStatusCommandrender rather than memoized. Fine in practice for a handful of model files, butuseMemocosts nothing.
No security concerns: the rmSync target is derived from cwd plus fixed path constants with no user input, deletion is confirmed by default, and --yes is opt-in.
On testing
Completely understood on the Windows/Apple Silicon constraint, and factoring skipFuseValidationError into a pure function to work around it was the right instinct. I'll do a macOS run of pnpm test:all plus the manual sequence you suggested (export -> status -> export --skip-fuse -> clean -> export --skip-fuse) once the above is addressed. I'll add an interrupted-export case to that pass, since that's where the --skip-fuse guard is weakest today.
|
On the clean scope question: keeping it fused/-only for this PR; matches the original issue #70 boundary. Filing a follow-up for a --target/--all flag (or a nanotune cache subcommand) to also cover the base-model cache, alongside the clean/export race that's already tracked. Happy to take either as a next PR if you'd rather see it split differently. |
will-lamerton
left a comment
There was a problem hiding this comment.
Thanks @rohanshrma222, this covers the blocking points well. The shared hasUsableFusedModel() helper is the right shape, and the interactive y/n/Escape coverage plus the interrupted-export case are exactly what was missing. I ran the suite on macOS: 88 passing, all checks green.
One thing slipped through. src/commands/status.tsx:47 still uses existsSync(fusedDir) rather than the new helper, so an interrupted export contradicts itself:
existsSync(fusedDir) : true
hasUsableFusedModel() : false
status prints Fused model cache: 3 B (run nanotune clean to remove), and then clean says Nothing to clean. Swapping that line to hasUsableFusedModel(fusedDir) fixes it. Worth a test alongside it, since the current status test writes a complete fused model.
Two smaller ones that belong in the follow-up issue rather than this PR:
hasUsableFusedModelpasses on any single.safetensors, so a sharded model interrupted after shard 1 of 3 still reads as usable.- The top-level
readdirSyncingetDirectorySizeand inhasUsableFusedModelare unguarded, soEACCESon the directory itself can still takestatusdown. Your change closed thestatSynccase I flagged, just not that one.
On the clean naming: agreed, keep it fused/-only here. Please open the follow-up for the base model cache and link it in the description.
Once status.tsx is updated I'll do the macOS manual pass and approve.
Thx for the review. Yes, status.tsx is still using the old check which will make the implementation confusing. I will make all the suggested changes. |
|
I have raised the new follow-up issue and have mentioned the issue here in the description. |
Description
Fixes #70 ;
nanotune exportfuses the LoRA adapter into a full-precision copy at.nanotune/models/fused/(multi-GB) and never reports or cleans it up. This PR keeps the cache (it speeds up repeat exports via--skip-fuse) but makes its cost visible and reclaimable:nanotune exportandnanotune statusnow report the fused cache's path and size.nanotune cleancommand removes it (interactive confirm, or-y/--yesfor CI/scripts).--skip-fusenow fails fast with a clear message if nofused/cache exists, instead of a cryptic error deep in GGUF conversion.export.md,status.md,index.md, and newclean.md.Edit:
nanotune cleanis deliberately scoped to the fused/ cache only for this PR — it doesn't also cover the larger base-model cache at~/.nanotune/models/base-cache. Follow-up tracked in #125: sharded-model detection inhasUsableFusedModel, the unguarded top-levelreaddirSync, and extendingcleanto cover the base-model cache.Type of Change
Testing
Automated Tests
pnpm test:allcompletes successfully)Manual Testing
nanotune initnanotune datacommands (add/import/list/validate)nanotune trainnanotune exportnanotune benchmarkChecklist
pnpm format)