d91beef0fd314250d8d9b94de86dfea019a8bd96
2
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Palash DebnathandClaude Opus 5
|
850ae8e192 |
test: reset model-manager shutdown state between backend tests (#1320)
* test: reset model-manager shutdown state between backend tests Two leaks, one of them mine. 1. `model_manager._shutting_down` is a module-global Event and the GPU pool is a module-global executor. Any test that runs the app lifespan flips both on the way out (begin_shutdown + _reset_gpu_pool) and nothing puts them back — correct in production, where the process is ending; wrong across a combined session. A test arriving with the flag set finds a shut-down executor, so its first run_in_executor raises "cannot schedule new futures after shutdown", which the preload path classifies as benign and swallows. The symptom is a load that silently never starts. Reset before AND after: before so an inherited flag cannot decide the test, after so a test that legitimately shuts down does not hand it on. 2. tests/test_torch_compile_path_gate.py assigned services.settings_store into sys.modules directly instead of via monkeypatch.setitem. That leaks process-wide out of collection and breaks every later import of the real module. backend/tests/test_no_module_stubs.py exists to catch exactly that, and caught it — I introduced it two commits ago while isolating the Settings gate for a review finding. #1269 stays open for its last failure, which is a different root cause: test_lifespan_shutdown_mid_load fails because a reload fixture in tests/ replaces services.model_manager, so the test patches one module object while main's lifespan uses another (verified: `same=False`). That is the duplicate- module class, not a state leak, and needs its own fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: let the shutdown-state reset fail loudly CodeRabbit Major: the broad try/except meant a reset that raised left the next test with stale shutdown or executor state — precisely the order-dependent failure the fixture exists to remove, while looking like it had worked. That is the same silent-fail-open shape as the watermark and ffmpeg bugs fixed earlier in this cycle. If reset_shutdown_flag() or _reset_gpu_pool() can raise, that is a real problem in model_manager and it should be loud. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: assert the shutdown-state reset fixture actually resets (CodeRabbit) Ordered pair: one test leaves the module globals exactly as the lifespan leaves them, the next asserts it arrived clean — delete the fixture and the second fails. Plus a mechanical guard that the reset stays un-swallowed, so a future try/except cannot make the fixture look like it worked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Palash DebnathandClaude Opus 5
|
4f1135ac62 |
fix(compile): skip torch.compile when the torch lib path has whitespace (#1310)
* fix(compile): skip torch.compile when the torch lib path has whitespace Inductor passes the torch library directory to clang++/g++ as an unquoted -L flag, so a path containing a space splits into two arguments and the compile dies with "no such file or directory: 'Support/...'". The quoting bug is inside PyTorch and we cannot fix it — but a path we already know cannot compile is one we should not spend a compile attempt on. The cost was never a broken generation (eager mode is the documented fallback); it was a guaranteed-failing compile on every load, whose clang wreckage got swallowed by `except Exception: logger.info(...)` and then ate a chunk of the captured log tail. That is exactly how it surfaced in #1259, where it was not the actual fault but crowded out the output that was. Not platform-specific: macOS keeps app data under ~/Library/Application Support/ and a Windows profile is routinely C:/Users/First Last. OMNIVOICE_FORCE_TORCH_COMPILE=1 still overrides, consistent with the arch gate. An unreadable torch path fails open — no evidence is not evidence of a problem. Closes #1266 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(compile): redact the torch path in the skip reason; tighten the tests CodeRabbit Major: the reason string embedded the absolute torch lib path, and both log branches print it — so a home directory (and anything secret-shaped in the path) went into omnivoice.log and any pasted bug report. It now goes through core.failure.sanitize(), the same redaction every other user-facing failure text uses, with a basename fallback if that import ever fails. Also per review: the Settings gate is isolated in the test helper (it was reading the real settings_store, so a persisted perf.torch_compile_disabled=1 could have decided these tests instead of the path logic), and two logging contracts are now asserted rather than merely exercised — the forced-override warning, and the skip message naming OMNIVOICE_FORCE_TORCH_COMPILE. A skip the user cannot discover how to override is a dead end. DECLINED: CodeRabbit's Critical asks for torch.compile to be disabled by default on every platform "so the default is uniform". That would make every Linux CUDA user slower to satisfy a rule about USER-VISIBLE default behaviour — and torch.compile is not user-visible, it is an internal optimization whose absence shows up only as speed. The function has always diverged by host by design: device != "cuda" skips, and no-Triton skips (which is every Windows install). This gate adds no new divergence; it declines a compile that is GUARANTEED to fail on that host, which is the same shape as the existing arch gate. Disabling a working optimization everywhere would be the regression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |