fix(windows): port-conflict kill + Triton/torch.compile disable (salvaged from #85) - #156
Conversation
…aged from #85) Salvages the two safe, valuable Windows fixes from community PR #85 without the changes that would regress all users. backend.rs — implement the Windows branch of `kill_orphan_on_port`, which was a no-op (`pub fn kill_orphan_on_port(_port: u16) {}`). It now parses `netstat -ano -p TCP` for the LISTENING socket on exactly `port` (suffix-matched on ":PORT" to avoid e.g. :3900 matching 39000) and kills the owning PID via `taskkill /PID <pid> /F`. Behind `#[cfg(not(unix))]`; the unix branch is untouched. Signature now matches the unix branch. main.py — on Windows (sys.platform == "win32"), default TORCH_COMPILE_DISABLE / TORCHDYNAMO_DISABLE / TORCHINDUCTOR_DISABLE to "1" before torch is imported. Triton has no Windows wheel, so these prevent TritonMissing/dynamo errors. Uses os.environ.setdefault (never overrides an explicit user value) and is win32-guarded, so cross-platform default behavior is unchanged. Intentionally NOT salvaged from #85: - The unconditional `os.environ["HF_HUB_OFFLINE"] = "1"` in main.py and the `local_files_only=True` / HF_HUB_OFFLINE save-restore in model_manager.py. This breaks first-run model downloads for every user (downloading models on first use is the core value prop). Offline mode must stay opt-in — only when the user sets HF_HUB_OFFLINE themselves. - The model_manager.py torch.compile/_get_gpu_pool changes: already present on main in a superior form (`should_torch_compile()` gating from plan-02/#65 and the existing `_get_gpu_pool()` lazy pool), so applying #85's cruder `TORCH_COMPILE_DISABLE` env check would regress. - The 512-line README rewrite, the ~50 frontend .jsx formatting-only diffs, and the personalities.py attrs additions (out of scope). Refs #85. Co-Authored-By: caaaaaleb <caaaaaleb@users.noreply.github.com> 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSets Windows-only torch compile/dynamo/inductor disable environment variables before importing torch, and replaces the non-Unix no-op kill_orphan_on_port with netstat-based PID discovery and taskkill termination for a given TCP port. ChangesWindows Platform Startup and Cleanup Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
|
| Filename | Overview |
|---|---|
| backend/main.py | Adds win32-guarded setdefault calls to disable Triton/dynamo/inductor before torch import; note that these calls land before dotenv loading (already flagged in a previous review thread) |
| frontend/src-tauri/src/backend.rs | Replaces the no-op Windows kill_orphan_on_port stub with a working netstat+taskkill implementation; port-suffix matching, field parsing, and error handling all look correct |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Tauri: kill_orphan_on_port called] --> B{Platform?}
B -- unix --> C[lsof -ti :port]
B -- not unix/Windows --> D[netstat -ano -p TCP]
D --> E{netstat ok?}
E -- err --> F[return early]
E -- ok --> G[iterate lines]
G --> H{line contains LISTENING?}
H -- no --> G
H -- yes --> I{local_addr ends with :port?}
I -- no --> G
I -- yes --> J[parse PID from last field]
J --> K[taskkill /PID x /F]
K --> G
L[main.py startup] --> M{sys.platform == win32?}
M -- yes --> N[setdefault TORCH_COMPILE_DISABLE=1]
N --> O[setdefault TORCHDYNAMO_DISABLE=1]
O --> P[setdefault TORCHINDUCTOR_DISABLE=1]
P --> Q[dotenv.load_dotenv]
M -- no --> Q
Q --> R[torch lazily imported later]
Reviews (2): Last reviewed commit: "fix: move win32 torch-disable block belo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if sys.platform == "win32": | ||
| os.environ.setdefault("TORCH_COMPILE_DISABLE", "1") | ||
| os.environ.setdefault("TORCHDYNAMO_DISABLE", "1") | ||
| os.environ.setdefault("TORCHINDUCTOR_DISABLE", "1") | ||
|
|
||
| # Ensure `backend/` is on sys.path so bare imports like `from core.config` |
There was a problem hiding this comment.
os.environ.setdefault() calls fire before dotenv.load_dotenv() runs. Because load_dotenv() defaults to override=False, any .env file that explicitly sets TORCH_COMPILE_DISABLE=0 (e.g., a power user who managed to install Triton on Windows and wants to opt back in to compilation) will be silently ignored — the setdefault value of "1" is already in os.environ when dotenv tries to apply it. The comment's stated guarantee ("never overrides an explicit user value") does not hold for .env-file values, only for process-environment values set before launch. Moving this block to after the dotenv loading section preserves the intent while letting all three override mechanisms (process env > .env file > default) work correctly.
| if sys.platform == "win32": | |
| os.environ.setdefault("TORCH_COMPILE_DISABLE", "1") | |
| os.environ.setdefault("TORCHDYNAMO_DISABLE", "1") | |
| os.environ.setdefault("TORCHINDUCTOR_DISABLE", "1") | |
| # Ensure `backend/` is on sys.path so bare imports like `from core.config` | |
| # Ensure `backend/` is on sys.path so bare imports like `from core.config` |
test_main_py_bootstrap_adds_backend_dir asserts the first 15 lines of main.py contain sys.path.insert + _backend_dir. The win32 block was placed above the preamble, pushing them out of range. The torch env vars only need to precede torch's (lazy) import, so moving the block below the sys.path bootstrap keeps behavior identical and restores the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/main.py (1)
6-7: 💤 Low valueComment inaccuracy: torch is explicitly imported, not lazy.
The comment states torch "is lazily imported in services/model_manager.py," but Line 108 contains an explicit
import torchstatement in this file. While the timing is still correct (these env vars are set before line 108), the comment is misleading.📝 Suggested clarification
-# to prevent TritonMissing errors at inference time. Must be set before torch -# is imported (it is lazily imported in services/model_manager.py). Uses +# to prevent TritonMissing errors at inference time. Must be set before torch +# is imported (explicit import at line 108; also used in services/model_manager.py). Uses🤖 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/main.py` around lines 6 - 7, The comment claiming "torch is lazily imported in services/model_manager.py" is inaccurate because this file contains an explicit import torch (the import torch statement). Update the comment near the setdefault code to remove the "lazily imported in services/model_manager.py" phrase and instead state that torch is imported explicitly in this module (or that the env vars are set before the import torch occurs), ensuring it correctly references the explicit import and preserves the note about using setdefault so user-set values aren't overridden.frontend/src-tauri/src/backend.rs (1)
111-129: 💤 Low valueMinor optimization: avoid double split.
The code calls
split_whitespace()twice for the same line (Line 117 and Line 121). While functionally correct, collecting once would be slightly more efficient.♻️ Proposed refactor
let port_suffix = format!(":{}", port); for line in stdout.lines() { if !line.to_uppercase().contains("LISTENING") { continue; } - // Local address is the second whitespace-delimited field. - // Format: " TCP 0.0.0.0:3900 0.0.0.0:0 LISTENING 1234" - let local_addr = line.split_whitespace().nth(1).unwrap_or(""); + let parts: Vec<&str> = line.split_whitespace().collect(); + // Local address is the second field (index 1). + // Format: " TCP 0.0.0.0:3900 0.0.0.0:0 LISTENING 1234" + let local_addr = parts.get(1).copied().unwrap_or(""); if !local_addr.ends_with(&port_suffix) { continue; } - let parts: Vec<&str> = line.split_whitespace().collect(); if let Some(pid_str) = parts.last() { if let Ok(pid) = pid_str.parse::<u32>() {🤖 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 `@frontend/src-tauri/src/backend.rs` around lines 111 - 129, Replace the double call to split_whitespace() by splitting once into a Vec (e.g., keep the existing parts: Vec<&str>) and reuse it: for each line, build parts = line.split_whitespace().collect(), check the LISTENING predicate on line as before, get the local address from parts.get(1).unwrap_or(&""), verify it ends_with(&port_suffix), then take the PID from parts.last() and parse/kills as currently done (retain variables pid_str/pid and Command::new("taskkill")). This removes the redundant split_whitespace() call and reuses parts for both local_addr and pid lookup.
🤖 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/main.py`:
- Around line 6-7: The comment claiming "torch is lazily imported in
services/model_manager.py" is inaccurate because this file contains an explicit
import torch (the import torch statement). Update the comment near the
setdefault code to remove the "lazily imported in services/model_manager.py"
phrase and instead state that torch is imported explicitly in this module (or
that the env vars are set before the import torch occurs), ensuring it correctly
references the explicit import and preserves the note about using setdefault so
user-set values aren't overridden.
In `@frontend/src-tauri/src/backend.rs`:
- Around line 111-129: Replace the double call to split_whitespace() by
splitting once into a Vec (e.g., keep the existing parts: Vec<&str>) and reuse
it: for each line, build parts = line.split_whitespace().collect(), check the
LISTENING predicate on line as before, get the local address from
parts.get(1).unwrap_or(&""), verify it ends_with(&port_suffix), then take the
PID from parts.last() and parse/kills as currently done (retain variables
pid_str/pid and Command::new("taskkill")). This removes the redundant
split_whitespace() call and reuses parts for both local_addr and pid lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d9c59a45-48f5-4f63-b9f4-f10d19701233
📒 Files selected for processing (2)
backend/main.pyfrontend/src-tauri/src/backend.rs
1 participant
Summary
Salvages the two safe, valuable Windows fixes from community PR #85 (author: @caaaaaleb), without the parts that would regress all users. The original PR bundled good Windows fixes with a dangerous default-behavior change plus a large amount of unrelated churn, so we are declining it as-is and landing the good parts cleanly here. Full credit to @caaaaaleb (co-authored on the commit).
What was salvaged
frontend/src-tauri/src/backend.rs— real Windowskill_orphan_on_port.The
#[cfg(not(unix))]branch was a no-op (pub fn kill_orphan_on_port(_port: u16) {}). It now parsesnetstat -ano -p TCPfor theLISTENINGsocket on exactlyport(suffix-matched on":PORT"to avoid e.g.:3900matching port39000) and kills the owning PID viataskkill /PID <pid> /F. Behind#[cfg(not(unix))]; the unix branch is untouched and the signature now matches it (pub fn kill_orphan_on_port(port: u16)).backend/main.py— Windows Triton disable block.On
sys.platform == "win32", defaultsTORCH_COMPILE_DISABLE/TORCHDYNAMO_DISABLE/TORCHINDUCTOR_DISABLEto"1"before torch is imported. Triton has no Windows wheel, so this preventsTritonMissing/dynamo errors. Usesos.environ.setdefault(never overrides an explicit user value) and iswin32-guarded — cross-platform default behavior is unchanged.What was intentionally NOT salvaged (why we declined #85 as-is)
os.environ["HF_HUB_OFFLINE"] = "1"inmain.pyand thelocal_files_only=True/HF_HUB_OFFLINEsave-restore inmodel_manager.py. This breaks first-run model downloads for every user — downloading models on first use is the core value prop. Offline mode must stay opt-in, only when the user setsHF_HUB_OFFLINEthemselves. Existing download behavior is left untouched.model_manager.pytorch.compile /_get_gpu_poolchanges — already present onmainin a superior form: theshould_torch_compile()gating (plan-02 / [Bug] torch.compile on Windows causes TTS OOM due to missing Triton #65, which gates on Triton availability and falls back to eager) and the existing_get_gpu_pool()lazy pool. Applying fix: Windows port conflict + HF_HUB_OFFLINE for offline model loading #85's cruderTORCH_COMPILE_DISABLEenv check would be a regression, somodel_manager.pyis not touched..jsxformatting-only diffs, and thepersonalities.pyattrsadditions — all out of scope for a Windows-fix salvage.Verification
cargo check(host/macOS, unix build): passes clean.ring, needs the Windows SDK), so the exact#[cfg(not(unix))]kill_orphan_on_portbody was type-checked in isolation againstx86_64-pc-windows-msvc: compiles clean.python -c "import ast; ast.parse(...)"onbackend/main.pyandbackend/services/model_manager.py: both parse OK.Refs #85.
🤖 Generated with Claude Code
Summary by CodeRabbit