fix(windows): gate torch.compile on Triton + ASR critical-path smoke (plan-02, closes #65) - #138
Conversation
…65) plan-02. torch.compile(mode="reduce-overhead") needs Triton at runtime; Triton has no Windows wheel, so the old `device=="cuda"`-only guard in model_manager.py failed on Windows+CUDA and surfaced as a confusing "OOM" (#65). Inference-time, hard to diagnose. - engine_env.should_torch_compile(device): requires CUDA + find_spec("triton") + the existing perf.torch_compile_disabled setting being off; logs the skip reason at INFO and falls back to eager. - model_manager.py call site uses it instead of the bare cuda check. - smoke-test.sh INST-02: import torch + ctranslate2 + whisperx (full ASR path) so a missing transitive dep fails the build instead of crashing mid- transcription (#116). Runs in the CI smoke-matrix on Win/macOS/Linux. setuptools>=75.0 (fix-sequence step 1) already pinned (#58). Linux/CUDA+Triton behaviour unchanged. Tests (TDD): tests/test_torch_compile_gate.py (4). Closes #65; addresses #129/#116. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 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 (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR gates in-process torch.compile on CUDA + Triton availability and a user escape-hatch, updates engine subprocess comments, adds an ASR install-time smoke import, and adds spec/plan/tasks and tests covering the gating paths. ChangesWindows Runtime Integrity
Sequence Diagram(s)sequenceDiagram
participant ModelLoader
participant EngineEnv
participant TritonDetector
participant Settings
ModelLoader->>EngineEnv: should_torch_compile(device)
EngineEnv->>TritonDetector: importlib.util.find_spec("triton")
TritonDetector-->>EngineEnv: present|missing
EngineEnv->>Settings: settings_store.get_text("perf.torch_compile_disabled")
Settings-->>EngineEnv: "0"|"1"|error
EngineEnv-->>ModelLoader: True|False
ModelLoader->>ModelLoader: apply torch.compile if True else skip
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 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)
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 |
|
Analysis CompleteGenerated ECC bundle from 1 commits | Confidence: 50% View Pull Request #139Repository Profile
Changed Files (7)
Top hotspots
Top directories
Analysis Depth Readiness (commit-history, 21%)ECC Tools uses this to decide whether recommendations should stay at commit-history/setup guidance or expand into CI, security, harness, reference-set, AI-routing, and team backlog work.
Reference Set Readiness (0/7, 0%)
Generated Instincts (15)
After merging, import with: Files
|
|
| Filename | Overview |
|---|---|
| backend/services/engine_env.py | Adds should_torch_compile(device) gating on CUDA + Triton availability + user setting; updates the build_engine_env comment to explain the intentional subprocess-only design. Module-level docstring still attributes #65 to build_engine_env, which is misleading. |
| backend/services/model_manager.py | Replaces bare device == 'cuda' guard with should_torch_compile(device) call; existing try/except backstop for compile failures is preserved. Change is minimal and correct. |
| scripts/smoke-test.sh | Adds INST-02 check importing torch, ctranslate2, and whisperx to catch missing transitive deps at build time. Consistent with INST-01 pattern. |
| tests/test_torch_compile_gate.py | Four unit tests cover all gate branches: non-CUDA, Triton missing, Triton present+enabled, and user-disabled. Monkeypatching is correct for the lazy-import pattern used in should_torch_compile. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[model_manager._load_model_sync] --> B{should_torch_compile device}
B --> C{device == cuda?}
C -- No --> D[Return False eager mode]
C -- Yes --> E{find_spec triton is None?}
E -- Yes --> F[Return False log: Triton unavailable]
E -- No --> G{perf.torch_compile_disabled == 1?}
G -- Yes --> H[Return False log: disabled in Settings]
G -- No --> I[Return True]
G -- read error --> I
I --> J[torch.compile mode=reduce-overhead]
D & F & H --> K[Eager mode no compile]
Reviews (3): Last reviewed commit: "revert(engine_env): keep subprocess TORC..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_torch_compile_gate.py (1)
28-37: ⚡ Quick winOptional: cover the documented "settings read fails → proceed" path.
The spec/edge-case and the helper's
exceptbranch guarantee that asettings_storeread failure should not block compile (returnsTrue, logged). That behavioral guarantee currently has no test.💚 Proposed test
def test_skips_when_disabled_in_settings(monkeypatch): monkeypatch.setattr(importlib.util, "find_spec", lambda name: object()) monkeypatch.setattr("services.settings_store.get_text", lambda key, default="0": "1") assert engine_env.should_torch_compile("cuda") is False + + +def test_proceeds_when_settings_read_fails(monkeypatch): + monkeypatch.setattr(importlib.util, "find_spec", lambda name: object()) + + def _boom(*_a, **_k): + raise RuntimeError("settings unreadable") + + monkeypatch.setattr("services.settings_store.get_text", _boom) + assert engine_env.should_torch_compile("cuda") is True🤖 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_torch_compile_gate.py` around lines 28 - 37, Add a test in tests/test_torch_compile_gate.py that simulates a failing settings read: patch importlib.util.find_spec to return a non-None value (to simulate Triton present) and monkeypatch services.settings_store.get_text to raise an exception, then assert engine_env.should_torch_compile("cuda") returns True; name the test e.g. test_proceeds_when_settings_read_fails and (optionally) assert that the code logs the fallback behavior. This ensures the except branch in engine_env.should_torch_compile is covered.scripts/smoke-test.sh (1)
337-341: ⚡ Quick winSurface the import error on failure to keep CI actionable.
2>/dev/nulldiscards the traceback, so a failing INST-02 only prints the generic "packaged venv incomplete" message without telling you which oftorch/ctranslate2/whisperxfailed or why. Since the whole point of this step is to catch missing transitive deps early, capturing and echoing stderr on failure makes CI logs self-diagnosing. (INST-01 above has the same pattern and could benefit too.)♻️ Capture and print stderr on failure
TESTS=$((TESTS + 1)) -if uv run python -c "import torch; import ctranslate2; import whisperx" 2>/dev/null; then +INST02_ERR=$(uv run python -c "import torch; import ctranslate2; import whisperx" 2>&1) +if [ $? -eq 0 ]; then pass "INST-02: ASR critical path (torch, ctranslate2, whisperx) imports OK" else fail "INST-02: ASR critical path import failed (torch/ctranslate2/whisperx) — packaged venv incomplete" + echo "$INST02_ERR" | tail -10 | sed 's/^/ /' fiNote: assigning to
INST02_ERRmakes the command's own exit status$?reflect the assignment, but sinceuv runis the only command in the substitution,$?still carries its exit code here. If you prefer to be explicit/robust underset -e, split the capture and the status check.🤖 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 `@scripts/smoke-test.sh` around lines 337 - 341, The INST-02 check currently swallows stderr by using "2>/dev/null" on the "uv run python -c 'import torch; import ctranslate2; import whisperx'" command; remove the redirection and capture the command's stderr (e.g., via command substitution like INST02_ERR="$(uv run python -c '... ' 2>&1')" or run then save "$?"/stderr separately) and on failure print the captured stderr alongside the existing fail message so CI shows which import failed and why; update the INST-02 branch that mentions "ASR critical path" to echo INST02_ERR when the import command fails.
🤖 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 `@scripts/smoke-test.sh`:
- Around line 337-341: The INST-02 check currently swallows stderr by using
"2>/dev/null" on the "uv run python -c 'import torch; import ctranslate2; import
whisperx'" command; remove the redirection and capture the command's stderr
(e.g., via command substitution like INST02_ERR="$(uv run python -c '... '
2>&1')" or run then save "$?"/stderr separately) and on failure print the
captured stderr alongside the existing fail message so CI shows which import
failed and why; update the INST-02 branch that mentions "ASR critical path" to
echo INST02_ERR when the import command fails.
In `@tests/test_torch_compile_gate.py`:
- Around line 28-37: Add a test in tests/test_torch_compile_gate.py that
simulates a failing settings read: patch importlib.util.find_spec to return a
non-None value (to simulate Triton present) and monkeypatch
services.settings_store.get_text to raise an exception, then assert
engine_env.should_torch_compile("cuda") returns True; name the test e.g.
test_proceeds_when_settings_read_fails and (optionally) assert that the code
logs the fallback behavior. This ensures the except branch in
engine_env.should_torch_compile is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6cd006e2-b3f0-4e49-a176-7473ab0ccc9d
📒 Files selected for processing (7)
backend/services/engine_env.pybackend/services/model_manager.pyscripts/smoke-test.shspecs/003-windows-runtime-integrity/plan.mdspecs/003-windows-runtime-integrity/spec.mdspecs/003-windows-runtime-integrity/tasks.mdtests/test_torch_compile_gate.py
…138) Greptile flagged that the in-process gate left a parallel gap: engine subprocesses honour TORCH_COMPILE_DISABLE, but build_engine_env() only set it on the user's Performance toggle — so a Triton-absent host (Windows, or macOS) still exposed subprocess engines to the same crash this PR fixes in-process. - build_engine_env(): set TORCH_COMPILE_DISABLE=1 when the user disabled compile OR Triton is unavailable (find_spec), cross-platform — mirrors should_torch_compile(). Drops the Windows-only scoping (and the now-unused `import sys`). - Refreshed the stale module docstring. - 3 new tests cover the subprocess gate (triton-missing, triton-present, user-opt-out). 7/7 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed both observations in dec971d: |
Reverts the build_engine_env() broadening from the previous commit. Auto- disabling subprocess torch.compile on Triton-absence conflicts with a deliberate, tested contract (test_perf_settings: Windows + flag-off ⇒ no injection; non-Windows ⇒ never inject) — the subprocess var is intentionally under the user's explicit control. The #65 fix is the in-process should_torch_compile() gate (unchanged here), which IS automatic and fully tested. Pushing back on the subprocess auto-gate as a separate, deliberate contract change rather than forcing it through by rewriting established tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Update on the subprocess-gate observation: I implemented it, but it conflicts with a deliberate, tested contract — |
1 participant
plan-02 — Closes #65. Addresses #129, #116.
Two Windows failures that only bite at inference time:
torch.compile(mode="reduce-overhead")needs Triton, which has no Windows wheel. The olddevice == "cuda"-only guard ran it on Windows+CUDA → failed, surfaced as a confusing "OOM".ModuleNotFoundError: pkg_resourcesvia ctranslate2→whisperx, mid-transcription.Fix
engine_env.should_torch_compile(device)— appliestorch.compileonly when device==CUDA andimportlib.util.find_spec("triton")is present and the existingperf.torch_compile_disabledsetting is off. Otherwise → eager mode with an INFO log.model_manager.pyuses it in place of the bare CUDA check.torch,ctranslate2,whisperx) so a missing transitive dep fails the build instead of crashing on the user's machine. Runs in the CIsmoke-matrixon Windows/macOS/Linux.setuptools>=75.0(fix-sequence step 1) was already pinned ([Bug] Transcription produced no segments. No module named 'pkg_resources' #58) — confirmed.Cross-platform parity
The gate is Triton-availability-driven: Linux/CUDA+Triton is unchanged (compile still applied); Windows falls back to eager — same user-visible result (working inference), no divergent default.
Tests (TDD, fail-before/pass-after — Constitution V)
tests/test_torch_compile_gate.py(4): skip on non-CUDA, skip when Triton missing, compile when present+enabled, skip when disabled. Engine/settings/model regression subset: 90 passed, 0 failed.Follow-up (out of scope)
A richer in-app post-bootstrap "setup incomplete" banner — deferred; the build-time smoke gate + ASR
is_available()message cover it for now.Spec/plan/tasks in
specs/003-windows-runtime-integrity/.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests