From 26f3be6e9677a23d14efbe67cf8aba04e1fc1ee7 Mon Sep 17 00:00:00 2001 From: Leonid Pershin Date: Fri, 21 Aug 2026 10:01:42 +0300 Subject: [PATCH] Refactor ComfyUI installation script to improve Python environment handling - Replaced direct calls to `venv/bin/pip` with `python -m pip` to avoid execution errors on network mounts and improve compatibility. - Introduced a new function `find_comfy_python` to locate the appropriate Python executable for ComfyUI installations. - Updated the `pip_install_sage` function to utilize the new Python handling, enhancing installation reliability. - Added tests to verify the new behavior of Python environment detection and installation processes, ensuring robustness. --- docs/reviews/2026-08-21-review-4.md | 98 ++++++++++++++++++++++++++ src/gpu_rent/remote/tune_swarm_perf.py | 81 +++++++++++++++------ tests/test_tune_swarm_perf.py | 32 +++++++++ 3 files changed, 190 insertions(+), 21 deletions(-) create mode 100644 docs/reviews/2026-08-21-review-4.md diff --git a/docs/reviews/2026-08-21-review-4.md b/docs/reviews/2026-08-21-review-4.md new file mode 100644 index 0000000..e96e21d --- /dev/null +++ b/docs/reviews/2026-08-21-review-4.md @@ -0,0 +1,98 @@ +# Project Review — 2026-08-21 (4) + +Фокус: скрытые логические инверсии / wait-forever / billing (как idle≠running). + +## Prior Reviews Summary + +> Based on `2026-08-21-review-3`, `review-2`, `review-1`. + +### Still Open (carried forward) +None. + +### Resolved Since Last Review +None newly verified in this pass (prior reviews already all `[x]`). + +--- + +## Phase 1: Code Quality + +### SOLID +No issues found. + +### Performance +No issues found beyond existing network-disk `du` / walk costs (acceptable). + +### Correctness & Bugs + +1. **Bug (critical)** — idle-killer treats SwarmUI `backend_status=empty` as not busy — `src/gpu_rent/remote/idle_killer.py` `swarm_busy()`. During first Comfy install (15–40+ min) status can stay `empty`/`unknown` with empty queue; after grace the idle clock can delete the VM mid-install. Same class as treating wrong Swarm status as “done”. + +2. **Bug (critical)** — Ollama pull marker expires at 45m while pull timeout is 7200s — `src/gpu_rent/remote/idle_killer.py` `llm_busy()` + `src/gpu_rent/remote/ollama_pull.py`. Marker timestamp is written once and never refreshed; after 45m killer clears it and may stop GPU during a long pull. + +3. **Bug (critical)** — `seed_autocomplete` can overwrite entire `Settings.fds` — `src/gpu_rent/provision.py` `seed_autocomplete()`. If meta exists without `settings_applied=true`, `put_text` replaces Settings with only AutoComplete stub — wipes `IsInstalled` / other settings. + +4. **Bug (high)** — `wait_backend_idle` treats SwarmUI `idle` as forever BUSY with no wake — `src/gpu_rent/ready.py`. After auto-suspend, backends report `idle` (cannot generate) and never become `running` without a wake; `up` can burn the full timeout. + +5. **Bug (high)** — `install_swarm_comfy` skip path still labels Swarm `idle` as “already Idle” — `src/gpu_rent/remote/install_swarm_comfy.py` `main()`. Misleading; should skip because backends exist, then align with wake/`running` wait. + +6. **Bug (high)** — local tunnel verify forces OK on TCP-only — `src/gpu_rent/ready.py` `verify_stack_local()`. If `GET /` and API fail, still `ok=True` with `TCP :port open` — marks tunnel ready when SwarmUI is dead. + +7. **Bug (high)** — torch without CUDA polls 600s — `src/gpu_rent/remote/stack_env_probe.py` + `verify_gpu_env`. Instant-fail covers driver/libcuda/empty dlbackend, but not “torch imported, cuda=False” (CPU wheel won’t heal). + +8. **Bug (high)** — perf tune execs `venv/bin/pip` → `OSError: Exec format error` — `src/gpu_rent/remote/tune_swarm_perf.py` `pip_install_sage()`. Observed on live up; OSError not caught → script exit 1; sageattention missing. **Fixing in this session via `python -m pip`.** + +9. **Bug (medium)** — access card / open use `resolve_llm_runtime` (notes override) but tunnel uses cfg only — `access_card.py` vs `tunnel.py`. Can advertise Ollama URLs with no forward. + +10. **Bug (medium)** — `status` tunnel check only probes SwarmUI local port — `cli.py`. llm-only looks like “туннель нет”. + +11. **Bug (medium)** — tunnel Nova poll soft-fail returns fake `ACTIVE` — `tunnel.py` `_poll_nova()`. Masks DELETED/ERROR. + +### Code Quality +No separate issues beyond the above. + +--- + +## Phase 2: Logical Consistency + +### Domain & Application Layer +No issues found. + +### Data Flow +See #9 (cfg vs notes for LLM runtime). + +### State Management +See #1–#2 (busy markers / status semantics). + +### Consistency +SwarmUI status vocabulary (`running` / `idle` / `empty` / `loading`) was inconsistent across wait vs killer; `running`=ready is fixed, remaining gaps in #1/#4/#5. + +--- + +## Phase 3: UI/UX + +### Usability +Access card correctly deferred until tunnel (recent change). Mid-up wait messages improved. + +### Visual & Consistency +No issues found. + +### Interaction & Feedback +Perf tune failure was noisy traceback then continued — should be WARN without stack (addressed with OSError catch + `-m pip`). + +### Accessibility +N/A (CLI). + +--- + +## Tasks + +- [ ] 1. [Bug] Treat SwarmUI `empty` (and likely `disabled` during provision) as busy in idle-killer — `src/gpu_rent/remote/idle_killer.py` `swarm_busy` +- [ ] 2. [Bug] Refresh `.gpu-rent-ollama-pulling` during pull and/or raise stale max-age ≥ pull timeout (7200s) — `src/gpu_rent/remote/idle_killer.py` + `ollama_pull.py` +- [ ] 3. [Bug] Never overwrite full `Settings.fds` in autocomplete seed — merge AutoComplete only — `src/gpu_rent/provision.py` `seed_autocomplete` +- [ ] 4. [Bug] On wait, wake suspended backends or accept healthy path when status=`idle` after install; don’t burn 2400s — `src/gpu_rent/ready.py` `wait_backend_idle` +- [ ] 5. [Bug] Fix `install_swarm_comfy` skip wording/logic for `idle` vs backends-present — `src/gpu_rent/remote/install_swarm_comfy.py` +- [ ] 6. [Bug] `verify_stack_local`: do not mark SwarmUI ok on TCP-only when HTTP/API fail — `src/gpu_rent/ready.py` +- [ ] 7. [Bug] Fail-fast when Comfy torch imports but `cuda=False` — `stack_env_probe.py` / `verify_gpu_env` +- [x] 8. [Bug] Perf tune: use `python -m pip` (not `venv/bin/pip`) and catch OSError — `src/gpu_rent/remote/tune_swarm_perf.py` +- [ ] 9. [Logic] Single source of truth for LLM runtime in access card vs tunnel — `access_card.py` / `tunnel.py` +- [ ] 10. [UX] `status` should probe ports from `tunnel_forwards(cfg)` — `cli.py` +- [ ] 11. [Bug] Tunnel Nova soft-fail must not fake ACTIVE forever — `tunnel.py` `_poll_nova` diff --git a/src/gpu_rent/remote/tune_swarm_perf.py b/src/gpu_rent/remote/tune_swarm_perf.py index dbc2986..6b41145 100644 --- a/src/gpu_rent/remote/tune_swarm_perf.py +++ b/src/gpu_rent/remote/tune_swarm_perf.py @@ -18,13 +18,15 @@ DATA = Path("/mnt/swarm_data") GPU_JSON = DATA / ".gpu-rent-gpu.json" MARKER = DATA / ".gpu-rent-perf-tuned" BACKENDS = DATA / "Data" / "Backends.fds" -COMFY_VENV_CANDIDATES = [ - DATA / "dlbackend" / "ComfyUI" / "venv" / "bin" / "pip", - DATA / "dlbackend" / "comfy" / "venv" / "bin" / "pip", - DATA / "dlbackend" / "comfy" / "ComfyUI" / "venv" / "bin" / "pip", - Path("/opt/swarmui/dlbackend/ComfyUI/venv/bin/pip"), - Path("/opt/swarmui/dlbackend/comfy/venv/bin/pip"), - Path("/opt/swarmui/dlbackend/comfy/ComfyUI/venv/bin/pip"), +# Prefer python then run `python -m pip` — venv/bin/pip often hits +# OSError Errno 8 Exec format error on network mounts / bad shebangs. +COMFY_PYTHON_CANDIDATES = [ + DATA / "dlbackend" / "ComfyUI" / "venv" / "bin" / "python", + DATA / "dlbackend" / "comfy" / "venv" / "bin" / "python", + DATA / "dlbackend" / "comfy" / "ComfyUI" / "venv" / "bin" / "python", + Path("/opt/swarmui/dlbackend/ComfyUI/venv/bin/python"), + Path("/opt/swarmui/dlbackend/comfy/venv/bin/python"), + Path("/opt/swarmui/dlbackend/comfy/ComfyUI/venv/bin/python"), ] @@ -66,16 +68,21 @@ def tier_notes(gpu: dict) -> dict: } -def find_pip() -> Path | None: - for p in COMFY_VENV_CANDIDATES: - if p.is_file(): +def find_comfy_python() -> Path | None: + for p in COMFY_PYTHON_CANDIDATES: + if p.is_file() and os.access(p, os.X_OK): + return p + for p in sorted(DATA.glob("dlbackend/**/venv/bin/python")): + if p.is_file() and os.access(p, os.X_OK): return p - # glob - for p in DATA.glob("dlbackend/**/venv/bin/pip"): - return p return None +# Back-compat alias for tests that monkeypatch find_pip +def find_pip() -> Path | None: + return find_comfy_python() + + def patch_backends_extra_args(extra: str) -> bool: """Ensure ExtraArgs contains sage flag for Comfy self-start backends.""" if not extra: @@ -108,18 +115,50 @@ def patch_backends_extra_args(extra: str) -> bool: return True -def pip_install_sage(pip: Path) -> bool: - print(f"pip install triton sageattention via {pip}") +def sage_already_importable(py: Path) -> bool: + code, _ = _run( + [str(py), "-c", "import triton, sageattention"], + timeout=60, + ) + return code == 0 + + +def _run(cmd: list[str], *, timeout: float = 900) -> tuple[int, str]: + try: + out = subprocess.check_output( + cmd, text=True, stderr=subprocess.STDOUT, timeout=timeout, env=_pip_env() + ) + return 0, out + except subprocess.CalledProcessError as exc: + return exc.returncode, (exc.output or str(exc)) + except (OSError, subprocess.TimeoutExpired) as exc: + return 1, str(exc) + + +def _pip_env() -> dict[str, str]: env = dict(os.environ) env["PIP_DISABLE_PIP_VERSION_CHECK"] = "1" - cmd = [str(pip), "install", "-U", "triton", "sageattention"] + return env + + +def pip_install_sage(py: Path) -> bool: + """Install via ``python -m pip`` (never exec venv/bin/pip directly).""" + if sage_already_importable(py): + print(f"triton+sageattention already importable via {py}") + return True + print(f"python -m pip install triton sageattention via {py}") + cmd = [str(py), "-m", "pip", "install", "-U", "triton", "sageattention"] try: - subprocess.check_call(cmd, env=env) + subprocess.check_call(cmd, env=_pip_env()) print("triton + sageattention installed") return True except subprocess.CalledProcessError as exc: print(f"WARN: pip install failed ({exc}) — ExtraArgs NOT applied") return False + except OSError as exc: + # Errno 8 Exec format error on bad wrappers — should not happen with -m pip + print(f"WARN: cannot run pip ({exc}) — ExtraArgs NOT applied") + return False def main() -> int: @@ -144,11 +183,11 @@ def main() -> int: pip_ok = not plan["use_sage"] restarted_needed = False if plan["use_sage"]: - pip = find_pip() - if pip: - pip_ok = pip_install_sage(pip) + py = find_comfy_python() + if py: + pip_ok = pip_install_sage(py) else: - print("Comfy venv pip not found yet — will retry next up") + print("Comfy venv python not found yet — will retry next up") pip_ok = False # Only patch ExtraArgs when wheels installed — otherwise Comfy may break. if pip_ok and patch_backends_extra_args(plan["extra_args"]): diff --git a/tests/test_tune_swarm_perf.py b/tests/test_tune_swarm_perf.py index 8a34301..423ce65 100644 --- a/tests/test_tune_swarm_perf.py +++ b/tests/test_tune_swarm_perf.py @@ -44,6 +44,7 @@ def test_pip_fail_skips_extra_args(tmp_path, monkeypatch): monkeypatch.setattr(mod, "MARKER", data / ".gpu-rent-perf-tuned") monkeypatch.setattr(mod, "BACKENDS", backends) monkeypatch.setattr(mod, "find_pip", lambda: pip) + monkeypatch.setattr(mod, "find_comfy_python", lambda: pip) monkeypatch.setattr(mod, "pip_install_sage", lambda _p: False) assert mod.main() == 0 @@ -79,6 +80,7 @@ def test_pip_ok_patches_extra_args(tmp_path, monkeypatch): monkeypatch.setattr(mod, "MARKER", data / ".gpu-rent-perf-tuned") monkeypatch.setattr(mod, "BACKENDS", backends) monkeypatch.setattr(mod, "find_pip", lambda: pip) + monkeypatch.setattr(mod, "find_comfy_python", lambda: pip) monkeypatch.setattr(mod, "pip_install_sage", lambda _p: True) assert mod.main() == 0 @@ -127,9 +129,39 @@ def test_pip_fail_retries_next_run(tmp_path, monkeypatch): monkeypatch.setattr(mod, "MARKER", marker) monkeypatch.setattr(mod, "BACKENDS", backends) monkeypatch.setattr(mod, "find_pip", lambda: pip) + monkeypatch.setattr(mod, "find_comfy_python", lambda: pip) monkeypatch.setattr(mod, "pip_install_sage", lambda _p: True) assert mod.main() == 0 new_m = json.loads(marker.read_text(encoding="utf-8")) assert new_m["pip_ok"] is True assert "--use-sage-attention" in backends.read_text(encoding="utf-8") + + +def test_pip_install_uses_python_dash_m(tmp_path, monkeypatch): + mod = _load() + py = tmp_path / "python" + py.write_text("", encoding="utf-8") + seen: list[list[str]] = [] + + def fake_call(cmd, env=None): + seen.append(list(cmd)) + return 0 + + monkeypatch.setattr(mod, "sage_already_importable", lambda _p: False) + monkeypatch.setattr(mod.subprocess, "check_call", fake_call) + assert mod.pip_install_sage(py) is True + assert seen and seen[0][:4] == [str(py), "-m", "pip", "install"] + + +def test_pip_install_oserror_returns_false(tmp_path, monkeypatch): + mod = _load() + py = tmp_path / "python" + py.write_text("", encoding="utf-8") + + def boom(*_a, **_k): + raise OSError(8, "Exec format error", str(py)) + + monkeypatch.setattr(mod, "sage_already_importable", lambda _p: False) + monkeypatch.setattr(mod.subprocess, "check_call", boom) + assert mod.pip_install_sage(py) is False