Design model pack cache/download contract for installer mode#350
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 Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds configurable model-cache and offline settings, immutable versioned model-pack metadata and cache lifecycle interfaces, installer-mode design documentation, a placeholder backend, and focused pytest coverage. ChangesModel pack installer contract
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Approved This PR adds design documentation and interface contracts (Protocol/dataclasses) for a future model pack cache system. All implementation methods raise NotImplementedError, the new config settings have inert defaults, and no existing code paths are modified - zero runtime behavior change. 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)
backend/src/find_api/core/config.py (1)
50-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a cross-field validator for
ML_OFFLINE_ONLYwithMODEL_CACHE_DIR.When
ML_OFFLINE_ONLY=True, models must be served from cache, which requiresMODEL_CACHE_DIRto point to a valid directory. The existingvalidate_remote_ml_configvalidator follows the same pattern forML_MODE=remote. Adding a similar guard would catch misconfigured deployments early, though this can wait until the cache implementation lands.♻️ Suggested validator
MODEL_CACHE_DIR: str = "" ML_OFFLINE_ONLY: bool = False + + `@model_validator`(mode="after") + def validate_offline_cache_config(self): + """Require a cache directory when offline-only mode is enabled.""" + if self.ML_OFFLINE_ONLY and not self.MODEL_CACHE_DIR.strip(): + raise ValueError( + "ML_OFFLINE_ONLY requires MODEL_CACHE_DIR to be set " + "so models can be loaded from the local cache." + ) + return self🤖 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/core/config.py` around lines 50 - 51, Extend the existing validate_remote_ml_config validation to also cover ML_OFFLINE_ONLY: when it is true, require MODEL_CACHE_DIR to be configured as a valid directory, while leaving current validation unchanged for other modes. Reuse the existing cross-field validation pattern and defer enforcement only if cache support is not yet available.
🤖 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 `@backend/src/find_api/core/model_pack.py`:
- Around line 87-95: Update the install method’s on_progress annotation to use
the typing Callable type instead of the builtin callable, importing Callable
from typing or collections.abc while preserving the existing PackProgress
argument and optional callback behavior.
---
Nitpick comments:
In `@backend/src/find_api/core/config.py`:
- Around line 50-51: Extend the existing validate_remote_ml_config validation to
also cover ML_OFFLINE_ONLY: when it is true, require MODEL_CACHE_DIR to be
configured as a valid directory, while leaving current validation unchanged for
other modes. Reuse the existing cross-field validation pattern and defer
enforcement only if cache support is not yet available.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7fb4c66-5404-4107-bed4-7af0baa8a64b
📒 Files selected for processing (3)
backend/src/find_api/core/config.pybackend/src/find_api/core/model_pack.pybackend/tests/test_model_pack.py
Abhash-Chakraborty
added
enhancement
macroscopeapp
Bot
dismissed
their stale review
Dismissing prior approval to re-evaluate 4264d7c
Abhash-Chakraborty
left a comment
Abhash-Chakraborty
left a comment
There was a problem hiding this comment.
Good direction, but please fix the failing backend check: remove the unused field import and use Callable instead of callable for the progress callback. Then rerun the backend checks.
macroscopeapp
Bot
dismissed
their stale review
Dismissing prior approval to re-evaluate f5bf5a1
macroscopeapp
Bot
dismissed
their stale review
Dismissing prior approval to re-evaluate 4f66db3
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 34345071 | Triggered | Generic Password | 329215e | docker-compose.yml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Maintainer follow-up completed: rebased this branch onto current main, applied Ruff formatting to the model-pack contract/tests, and verified the focused model-pack plus remote-config suite locally. GitHub CI, CodeQL, hardware matrix, frontend/backend checks, and TestSprite are green. The cross-field offline-cache validator remains intentionally deferred until the filesystem-backed cache implementation exists, as documented by this design PR. |
Abhash-Chakraborty
left a comment
Abhash-Chakraborty
left a comment
There was a problem hiding this comment.
The scoped design and interfaces match #45, are now formatted and tested, and do not alter existing loaders. Approved after maintainer verification.
Abhash-Chakraborty
added
ready-to-merge
Abhash-Chakraborty
merged commit d4b4332
into
Abhash-Chakraborty:main
|
@macroscope-app review Please review this PR against its linked issue, local-first privacy rules, and the current Find repo instructions. |
Abhash-Chakraborty
added
the
gssoc:approved
2 participants
Summary
Adds the design for installer-mode model pack downloading/caching, plus the
thin interfaces (ModelPack, PackCategory, PackStatus, PackProgress,
PackCache) that a future implementation will build on. This keeps the
installer small while allowing Find to run ML fully locally after models
are downloaded, with explicit size/progress/verification/retry/offline
behavior instead of invisible first-run downloads.
Fixes #45
Type of change
What changed
docs/plans/partial/model-cache-design.md: versioned pack manifest schema (source, license, size, checksum, compatibility), first-run selection/disk-space-preview/progress/cancel/resume/retry UX, atomic install + corrupted-cache recovery, cache-location config, and offline reuse — verified against the actual loader/config names infind_api/ml/andcore/config.py.backend/src/find_api/core/model_pack.py: thinModelPackdataclass,PackCategory/PackStatusenums,PackProgress, and aPackCacheprotocol with aNotImplementedPackCacheplaceholder. No existing ML loader is modified.MODEL_CACHE_DIRandML_OFFLINE_ONLYsettings tobackend/src/find_api/core/config.py, both defaulted so Docker/full/mock/remote dev modes are unaffected..env.example.docs/plans/partial/local-first-roadmap.md's "Follow-up implementation work" list.backend/tests/test_model_pack.pycovering the new thin interfaces.Screenshots / recordings (for UI changes)
N/A — this PR is design + backend interfaces only, no UI changes.
How to test
Also review
docs/plans/partial/model-cache-design.mdagainst the currentfind_api/ml/loaders andcore/config.pyto confirm the documented modelnames/settings still match.
Checklist
GSSoC'26 checklist
Summary by CodeRabbit
MODEL_CACHE_DIRandML_OFFLINE_ONLYsettings to enable local-only model reuse and configurable caching.