feat: add model cache and runtime footprint report - #359
Conversation
PR Context Summary
Suggested issue links
Use |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds local model-cache inspection and runtime status reporting for five models. The report is available through a CLI and an admin-only API endpoint, with optional path exposure for local use and path suppression for the API. ChangesModel footprint reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant StatusAPI
participant build_report
participant LocalCaches
participant RuntimeState
Admin->>StatusAPI: GET /api/status/models/footprint
StatusAPI->>build_report: build_report(include_paths=False)
build_report->>LocalCaches: resolve local model caches
LocalCaches-->>build_report: cache metadata
build_report->>RuntimeState: read manager and Redis status
RuntimeState-->>build_report: loaded, device, and last-use data
build_report-->>StatusAPI: path-free report
StatusAPI-->>Admin: JSON response
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
|
Macroscope skipped reviewing this pull request. Per-review cost limit exceeded (workspace setting). This review would cost an estimated $2.19, which exceeds your per-review limit of $2.00. The top 3 files driving up this estimate:
Tip To get this pull request reviewed, you can:
|
ApprovabilityVerdict: Needs human review Unable to check for correctness in b0c703a. This PR introduces a new feature (model footprint reporting) with a new admin API endpoint, CLI tool, and substantial new logic. The author does not own any of the changed files, all of which have a designated code owner who should review this contribution. You can customize Macroscope's approvability policy. Learn more. |
![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
🧹 Nitpick comments (1)
docs/guides/model-footprint.md (1)
44-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language specifier to the fenced code block.
The code block at line 44 has no language tag, triggering markdownlint MD040. Since it contains an HTTP endpoint reference,
textis the most appropriate specifier.♻️ Proposed fix
-``` +```text GET /api/status/models/footprint (admin-only)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/guides/model-footprint.mdat line 44, Update the fenced code block near
the HTTP endpoint reference in model-footprint.md to specify the text language,
preserving the existing endpoint content.</details> <!-- cr-comment:v1:399d9c73afe59788e848617d --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@backend/tests/test_model_footprint.py:
- Around line 161-169: Replace the tautological assertion in
test_not_found_reports_checked_locations with a direct assertion of the intended
note content: verify that info.note is present and includes the expected “not
found” text, preserving the existing exists and bytes_on_disk checks.
Nitpick comments:
In@docs/guides/model-footprint.md:
- Line 44: Update the fenced code block near the HTTP endpoint reference in
model-footprint.md to specify the text language, preserving the existing
endpoint content.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `f00c403c-0f5e-4c92-a420-0212c9d219bc` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between de5f27900fcd35ab6232b1853ae003da2280ae21 and f0432f5156d2cd1c036c19f216ec57e0858418f6. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `backend/scripts/model_footprint_report.py` * `backend/src/find_api/core/model_footprint.py` * `backend/src/find_api/routers/status.py` * `backend/tests/test_model_footprint.py` * `backend/tests/test_status.py` * `docs/guides/model-footprint.md` * `docs/overhaul/inventory/lane-f-ml.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
Please fix the CI. |
|
Thanks for this — the report itself is well built, and the read-only/no-paths default on the API is exactly the boundary I wanted. Three things before I can merge:
Also, your branch is named Ping me once CI is green and I'll take another pass. |
Abhash-Chakraborty
added
under-review
|
@macroscope-app review Please review this PR against its linked issue, local-first privacy rules, and the current Find repo instructions. |
Abhash-Chakraborty
added
stale
|
This PR is marked as stale. It will close automatically after 7 days without renewed activity; remove the stale label when work resumes. |
|
Checking in — no movement here since the 13th, and three things are red:
Marking it stale so the queue reflects reality. Push anything and the label comes straight off — the underlying feature is still wanted, it just needs to be sitting on the right branch. Shout if you want a hand with the rebase. |
Maintainer merge to unblock the PR: it targeted main from a 50-commit-old base, which is why it showed as conflicting and why backend-check failed. Conflicts (2): - backend/tests/test_status.py: append/append at the file tail. Canary added a worker_health assertion to the existing /status/models test while this branch appended a new footprint test. Both kept. - docs/overhaul/inventory/lane-f-ml.md: canary deleted the whole docs/overhaul/inventory/ tree, so this branch's edit to it is dropped. Fixes: - ruff format on core/model_footprint.py and tests/test_model_footprint.py, which was the actual backend-check failure. - Retargeted six dangling references. Three pointed at docs/overhaul/inventory/lane-f-ml.md, which no longer exists, and three cited "issue Abhash-Chakraborty#45" for the proposed CPU pack. Abhash-Chakraborty#45 is closed and is about installer model downloads; the CPU-only runtime profile is Abhash-Chakraborty#339. Left unchanged these would have sent a reader to a deleted file and a closed, unrelated issue. - Added the missing language to a fenced block in docs/guides/model-footprint.md (CodeRabbit nitpick). Verified against current canary: ruff check clean, ruff format clean (152 files), and the full suite at 635 passed / 8 skipped, up from canary's 619/6. The two new skips are symlink-guarded huggingface_hub cache tests that do run on the Linux CI runner. Reviewed and left as-is: the route is registered before /status/{job_id} so there is no path-matching shadowing; every integration point the module reaches for still exists on canary (ModelManager.last_used, current_torch_device, detect_capabilities, resolve_execution, get_redis_connection).
Abhash-Chakraborty
removed
the
stale
|
This is the measurement source I wanted for #45 — the "never download weights, degrade to not-cached on any error" constraint is the right call, and keeping paths CLI-only while Its been sitting since mid-July though and CI never completed on it (no backend-check run at all). Please rebase onto current canary and push so the full suite actually runs. One nit: the script docstring says the proposed CPU pack is tracked by #339 but the PR body says #45 — pick one. And Rebase + green CI and I will merge this. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/src/find_api/routers/status.py (1)
21-35: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid a full cache scan on every request.
The route calls
build_report()synchronously for each request. The report scans cache directories and Hugging Face indexes, which can slow repeated admin polling and increase filesystem I/O when the page cache is cold.Add a short-lived, concurrency-safe TTL cache for cache-resolution data, or document the scan cost and expected polling interval.
🤖 Prompt for 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. In `@backend/src/find_api/routers/status.py` around lines 21 - 35, Update get_model_footprint and the underlying build_report cache-resolution flow to avoid rescanning cache directories and Hugging Face indexes on every request. Add a short-lived, concurrency-safe TTL cache for the resolved cache data while preserving fresh model state in each report; alternatively, explicitly document the scan cost and required polling interval if caching is not implemented.
🤖 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.
Nitpick comments:
In `@backend/src/find_api/routers/status.py`:
- Around line 21-35: Update get_model_footprint and the underlying build_report
cache-resolution flow to avoid rescanning cache directories and Hugging Face
indexes on every request. Add a short-lived, concurrency-safe TTL cache for the
resolved cache data while preserving fresh model state in each report;
alternatively, explicitly document the scan cost and required polling interval
if caching is not implemented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a6033722-b174-431c-aefa-0cff59a5bc4a
📒 Files selected for processing (6)
backend/scripts/model_footprint_report.pybackend/src/find_api/core/model_footprint.pybackend/src/find_api/routers/status.pybackend/tests/test_model_footprint.pybackend/tests/test_status.pydocs/guides/model-footprint.md
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/tests/test_status.py
- docs/guides/model-footprint.md
- backend/tests/test_model_footprint.py
- backend/src/find_api/core/model_footprint.py
…ASCII backend-check was failing on two of this PR's own tests. Root cause: the HF cache tests build a real on-disk Hub cache and call the real huggingface_hub, but the dev dependency group deliberately excludes the ML extras, so huggingface_hub is not installed in CI. _hf_hub_cache_matches bails out at the import and reports "not cached" — which is also what an empty cache looks like, so test_finds_cached_repo_by_full_id failed and test_pack_totals_sum_correctly saw 4 of 5 models cached instead of 5. It only ever passed locally on Windows because both tests skip there for a different reason (scan_cache_dir needs real symlinks, which need Developer Mode). Two more tests in that class were passing for the wrong reason — "not cached" is the expected result either way — so the whole HF path had no real coverage anywhere. Stub scan_cache_dir instead of building a real cache. That drops both the huggingface_hub and the symlink requirement, so the needle-matching and aggregation logic — the part that is ours — is now genuinely covered on every platform. The real-library check is kept as a separate integration test guarded by importorskip, and new cases cover case-insensitive matching, multi-repo summing with its note text, and a corrupted cache degrading to "not cached" rather than raising. Separately, the CLI used an em dash as its "no value" placeholder. The default Windows console codepage is cp1252, where that renders as mojibake, and the project ships a Windows desktop build. The script is now ASCII-only, with tests asserting the rendered output encodes as cp1252 and that the source stays ASCII. Verified: report contains no filesystem paths or host identifiers with include_paths=False, --json and --no-paths both behave, and all three CLI modes render. Full backend suite 688 passed, 7 skipped; ruff check and format clean. Not changed: the shared Redis client has no socket timeout, so the cross-process loaded-model scan in _aggregate_manager_status can hang on an unreachable-but-accepting Redis. That client is shared with the rq workers, which rely on long blocking reads, so it needs its own change rather than one buried in this PR.
Abhash-Chakraborty
added
the
api
Abhash-Chakraborty
added
testing
Abhash-Chakraborty
left a comment
Abhash-Chakraborty
left a comment
There was a problem hiding this comment.
backend-check was red because the HF cache tests need huggingface_hub, which the dev group deliberately excludes — they only ever passed locally on Windows, where they skip for the unrelated symlink reason. Stubbed scan_cache_dir so that path is genuinely covered everywhere, and made the CLI ASCII-only since the em-dash placeholder was mojibake on a cp1252 console. 688 passed, ruff clean, no paths leak with include_paths=False — approving.
76456e4
into
Abhash-Chakraborty:canary
2 participants
Closes #345.
What
Adds a reproducible, read-only report of what each ML model pack downloads
and loads: configured identifier, on-disk cache footprint, loaded/unloaded
state, execution device, and last-use time.
find_api.core.model_footprint.build_report()— core report builder,one
ModelSpecper model (siglip, florence-2, yolo, insightface,paddleocr), each with a best-effort local cache resolver.
GET /api/status/models/footprint— admin-only endpoint,include_paths=False.Never returns filesystem paths, host info, or media metadata.
backend/scripts/model_footprint_report.py— local CLI (--json,--no-paths); includes paths by default since it runs with theoperator's own filesystem access.
docs/guides/model-footprint.md— usage guide, pack table, currentapproximate sizes for planning.
docs/overhaul/inventory/lane-f-ml.md— points future pack-sizeproposals at this report as the measurement source.
Why
#45 (installer model downloads/cache management) and model benchmark work
need real numbers instead of hand-estimated cache sizes. This gives both a
stable measurement source.
Design constraints
read or a local cache-index read (
huggingface_hub.scan_cache_dir()reads on-disk metadata only, no network call).
degrades to "not cached" rather than raising.
/status/models/footprint) never exposes paths,credentials, or media metadata — path info is CLI-only.
Testing
backend/tests/test_model_footprint.py— new, covers each cacheresolver against temporary fake cache dirs (no real downloads) and
build_report()assembly/pack totals. Two symlink-dependent assertionsskip cleanly on platforms without real symlink support (e.g. Windows
without Developer Mode) rather than faking a huggingface_hub cache
scan_cache_dir()wouldn't actually recognize.backend/tests/test_status.py— new endpoint test asserting all 5model keys are reported, loaded state reflects
ModelManager, and nopathkey appears anywhere in the response.Out of scope
proposed_cpupack (ONNX replacements) is listed withstatus: "not_implemented"— tracked separately by feat: design installer model downloads and cache management #45.model_manager.py; the report only reads its existingget_status()/last_usedstate.Summary by CodeRabbit
New Features
Documentation