feat(settings): configurable models directory (#64) - #149
Conversation
Let users pick where model weights download (the HuggingFace / Torch cache) instead of being pinned to ~/.cache/huggingface — useful when the system drive is small or slow. Backend: - core/user_env.py: durable per-user env file (~/.config/omnivoice/env) helper with upsert/unset that preserves other keys and writes 0600. main.py already loads this at startup before importing torch/HF, so the value takes effect on the next launch. Path resolves at call time via an OMNIVOICE_ENV_FILE override so it's robust to module re-import in tests. - settings.py: GET/PUT /api/settings/storage/models-dir — validates the dir is writable (mkdir + write-probe → 400 if not), persists the choice, and writes OMNIVOICE_CACHE_DIR to the durable env. Empty path clears → reverts to default. Returns restart_required since an in-use cache can't be safely moved mid-process. Loopback-gated like the other settings. Frontend: - StoragePanel: Models tab panel to view/set/reset the directory, shows effective vs configured vs default + a restart note. Cross-platform default parity preserved (default cache path is the HF default on every OS); local-first (no network); backward-compatible (absent setting → existing behavior). No version bump. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a durable per-user env helper, backend GET/PUT settings endpoints to configure the HuggingFace/Torch models directory (validation, creation, writability probe, durable OMNIVOICE_CACHE_DIR), a React StoragePanel UI, and tests covering behavior and edge cases. ChangesConfigurable Models Directory Storage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| backend/api/routers/settings.py | Adds GET/PUT /api/settings/storage/models-dir endpoints; _effective_models_dir() and _default_models_dir() both report paths one directory level above where HuggingFace actually stores model files in the HF_HOME/default cases. |
| backend/core/user_env.py | New durable per-user env file helper; addresses the previous permission race with _opener_0600, but _write_lines still does a non-atomic truncate-then-write that could corrupt the file (losing HF_TOKEN) on a crash mid-write. |
| frontend/src/components/settings/StoragePanel.jsx | New React component for the Models directory panel; correctly handles loading, saving, error display, and restart prompt with clean separation of concerns. |
| frontend/src/components/settings/StoragePanel.css | New CSS for StoragePanel; uses CSS variables for theming and is self-contained. |
| frontend/src/pages/Settings.jsx | Wires StoragePanel into the existing models tab; minimal change wrapped in a fragment. |
| tests/test_models_dir_setting.py | Good coverage of the PUT/GET endpoints including unwritable dir, NUL byte, clear-to-default, and XDG_CACHE_HOME awareness. |
| tests/test_user_env.py | Covers upsert, key preservation, unset, bare-filename edge case, and 0600 permission; comprehensive for the helper. |
Sequence Diagram
sequenceDiagram
participant UI as StoragePanel (React)
participant API as PUT /api/settings/storage/models-dir
participant FS as Filesystem
participant Env as user_env.py (~/.config/omnivoice/env)
participant Main as main.py (next launch)
UI->>API: "PUT {path: "/data/models"}"
API->>API: validate control chars
API->>FS: makedirs + write-probe + cleanup
FS-->>API: ok / OSError 400
API->>Env: set_user_env(OMNIVOICE_CACHE_DIR, path)
API-->>UI: "{configured, effective, restart_required: true}"
UI->>UI: show restart notice
Note over Main: On next backend start
Main->>Env: dotenv.load_dotenv(~/.config/omnivoice/env)
Main->>Main: "os.environ[HF_HOME] = OMNIVOICE_CACHE_DIR"
Main->>Main: "os.environ[HF_HUB_CACHE] = OMNIVOICE_CACHE_DIR"
Main->>Main: "os.environ[TORCH_HOME] = OMNIVOICE_CACHE_DIR"
Reviews (3): Last reviewed commit: "refactor(#64): single source of truth fo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/api/routers/settings.py`:
- Around line 252-275: The code currently swallows exceptions from
settings_store.set_text(...) (and continues after
user_env.set_user_env()/unset_user_env), which can mislead the UI into believing
the change persisted; update the handlers around settings_store.set_text and
user_env.set_user_env/unset_user_env (references: _MODELS_DIR_KEY,
settings_store.set_text, user_env.set_user_env, user_env.unset_user_env,
_MODELS_DIR_ENV) so that if either persisting to settings_store OR updating the
durable user_env fails you raise a 500 (e.g., throw
HTTPException(status_code=500)) instead of logging and returning success; ensure
both operations succeed before returning {"configured":..., "restart_required":
True} and on failure, include the original error context in the raised
exception/log.
- Around line 209-219: _default_models_dir currently hard-codes
"~/.cache/huggingface" which disagrees with huggingface_hub's XDG-aware default;
replace its logic to use huggingface_hub's canonical default (e.g., call
huggingface_hub.utils.default_cache_path or equivalent XDG-aware helper) so GET
/storage/models-dir reports the true default, update _effective_models_dir to
prefer that canonical default when HF env vars are unset, and in set_models_dir
ensure failures from settings_store.set_text (both clear and set branches) are
not swallowed — surface the error and return a failure response; only update
OMNIVOICE_CACHE_DIR/env vars after successfully persisting via
settings_store.set_text so persisted `configured` cannot diverge from
`effective`.
In `@backend/core/user_env.py`:
- Around line 20-25: The helper _read_lines currently swallows all OSError
subclasses which can hide genuine read failures and cause
set_user_env/unset_user_env to rebuild from an empty baseline; change the
exception handling so only FileNotFoundError is treated as "file missing" and
returns [], while any other OSError is re-raised (i.e., replace the broad except
OSError with an except FileNotFoundError: return [] and let other errors
propagate) so callers like set_user_env and unset_user_env will abort on real
read errors.
- Around line 28-29: In _write_lines, guard the os.makedirs call so it does not
run when os.path.dirname(path) is empty (e.g., path="env"); compute parent =
os.path.dirname(path) and only call os.makedirs(parent, exist_ok=True) if parent
is truthy, then proceed to open and write the file as before; this prevents
os.makedirs("") from raising while keeping behavior for nested paths.
In `@frontend/src/components/settings/StoragePanel.jsx`:
- Around line 81-97: The models-directory text input (className
"storagepanel__input", data-testid "models-dir-input", controlled by state
setInput/input) lacks an accessible label; add programmatic labeling by either
adding a visible <label> tied to that input via htmlFor/id or by referencing
existing heading/help text with aria-labelledby or aria-describedby attributes
on the input (ensure you add a matching id to the heading/paragraph). Update the
input element to include the chosen aria attribute or id reference so screen
readers receive a descriptive label while preserving the existing disabled,
value, placeholder and event handlers.
In `@tests/test_models_dir_setting.py`:
- Around line 39-42: The test test_rejects_unwritable_dir currently uses a
Unix-specific path ("/dev/null/...") which fails on Windows; change the test to
simulate an unwritable directory instead of relying on OS paths by mocking the
filesystem operations used by s.set_models_dir (e.g., mock os.makedirs and
builtins.open or the specific helper that creates/validates the directory) to
raise OSError, then call
s.set_models_dir(s._ModelsDirBody(path="some/any/path")) and assert the raised
fastapi.HTTPException has status_code 400; this keeps the test OS-neutral while
still exercising the rejection logic in s.set_models_dir.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aa84980c-07b9-4b49-a396-1ff17d6f1687
📒 Files selected for processing (7)
backend/api/routers/settings.pybackend/core/user_env.pyfrontend/src/components/settings/StoragePanel.cssfrontend/src/components/settings/StoragePanel.jsxfrontend/src/pages/Settings.jsxtests/test_models_dir_setting.pytests/test_user_env.py
- settings.py: reject control/NUL chars in the path with a 400 before any filesystem call (an embedded NUL otherwise raised ValueError → 500). Also serves as the explicit input-validation barrier for the user-chosen path (loopback-gated same-user local file picker — no cross-privilege boundary). - test_user_env.py: use `with open(...)` so the file is closed and the assert has no side effects. - user_env.py: comment the best-effort chmod except clause. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_models_dir_setting.py (1)
45-50: 💤 Low valueUse a non-
/tmppath to silence Ruff S108 and clarify intent.The path here is rejected by the control-char check before any filesystem access, so the
/tmp/prefix is meaningless and only trips Ruff S108 (hardcoded temp path). A relative path makes the intent clearer and keeps the linter quiet.🧹 Proposed tweak
- s.set_models_dir(s._ModelsDirBody(path="/tmp/mo\x00dels")) + s.set_models_dir(s._ModelsDirBody(path="models\x00dir"))🤖 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 `@tests/test_models_dir_setting.py` around lines 45 - 50, Change the hardcoded /tmp prefix in the failing test to a non-absolute/relative path so Ruff S108 is not triggered: in test_rejects_path_with_null_byte update the ModelsDirBody path argument (used in s.set_models_dir and the test function name) to a relative path (e.g., "mo\x00dels" or "./mo\x00dels") instead of "/tmp/mo\x00dels" since the control-char check rejects the path before any filesystem access and the prefix is unnecessary.
🤖 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 `@tests/test_models_dir_setting.py`:
- Around line 45-50: Change the hardcoded /tmp prefix in the failing test to a
non-absolute/relative path so Ruff S108 is not triggered: in
test_rejects_path_with_null_byte update the ModelsDirBody path argument (used in
s.set_models_dir and the test function name) to a relative path (e.g.,
"mo\x00dels" or "./mo\x00dels") instead of "/tmp/mo\x00dels" since the
control-char check rejects the path before any filesystem access and the prefix
is unnecessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 39d2ea95-6f10-4ed3-bab1-dad714bc4649
📒 Files selected for processing (4)
backend/api/routers/settings.pybackend/core/user_env.pytests/test_models_dir_setting.pytests/test_user_env.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/core/user_env.py
- tests/test_user_env.py
Address CodeRabbit + Greptile review on PR #149: - P1 (both bots): the settings_store copy of the models dir was only ever read by this GET endpoint, so it was a redundant cache that could diverge from the durable env file (the value main.py actually reads). Drop it — the per-user env file (OMNIVOICE_CACHE_DIR) is now the single source of truth: PUT writes it, GET reads it back. No divergence possible. - XDG-aware default (CodeRabbit): _default_models_dir now honors XDG_CACHE_HOME, matching huggingface_hub's real default on Linux. - Atomic 0600 write (Greptile, security): user_env writes via an os.open opener that creates the file 0600 from the start — no world-readable window before chmod for a file that can hold HF_TOKEN. - _read_lines only swallows FileNotFoundError; other OSErrors propagate so an upsert can't silently drop existing keys on a transient read failure. - Guard makedirs("") when the env path is a bare filename (no parent). - Best-effort write-probe cleanup in a finally; raise ... from e. - a11y: label the models-dir input via aria-labelledby/aria-describedby. - OS-neutral unwritable-dir test (mock makedirs) instead of Unix-only /dev/null path semantics. 12 tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the review in Store/env divergence (CodeRabbit XDG-aware default (CodeRabbit Non-atomic secret exposure (Greptile P2, security) —
CodeQL "user-provided value in path" (3× high, |
| try: | ||
| os.makedirs(path, exist_ok=True) | ||
| probe = os.path.join(path, ".omnivoice_write_test") | ||
| with open(probe, "w", encoding="utf-8") as f: |
| # Best-effort cleanup; a failed remove (concurrent process, perm change) | ||
| # must not leave the request hanging or mask the real error. | ||
| try: | ||
| os.remove(os.path.join(path, ".omnivoice_write_test")) |
| # must not leave the request hanging or mask the real error. | ||
| try: | ||
| os.remove(os.path.join(path, ".omnivoice_write_test")) | ||
| except OSError: |
2 participants
Closes #64.
What
Lets users choose where model weights download (the HuggingFace / Torch cache) from Settings → Models, instead of being pinned to
~/.cache/huggingface. Useful when the system drive is small/slow and the user wants weights on a bigger or faster volume.How
Backend
core/user_env.py— durable per-user env file (~/.config/omnivoice/env) helper withget/set/unset. Upsert preserves other keys (e.g. a persistedHF_TOKEN) and writes the file0600.main.pyalready loads this at startup before importing torch/HF, so a written value takes effect on the next launch. Path resolves at call time (honoring anOMNIVOICE_ENV_FILEoverride) so it's robust to module re-import.api/routers/settings.py—GET/PUT /api/settings/storage/models-dir:PUTvalidates the directory is writable (mkdir+ write-probe →400if not), persists the choice in the settings store, and writesOMNIVOICE_CACHE_DIRto the durable env. Empty path clears → reverts to default.restart_required: true(an in-use cache can't be safely relocated mid-process). Loopback-gated like the other settings endpoints.Frontend
StoragePanelin the Models tab — view / set / reset the directory; shows effective-vs-configured-vs-default and a restart note.Constraints
Tests
tests/test_user_env.py(5) — upsert/unset/preserve/0600.tests/test_models_dir_setting.py(4) — persist+durable-env write, unwritable→400, clear→default, GET shape.All 9 green. (The unrelated
test_supertonic3::test_license_gatefails locally only becausesupertonicisn't in my local venv; it's a runtime dep present in CI.)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests