diff --git a/CHANGELOG.md b/CHANGELOG.md index dd0b9f1e..869655de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -122,6 +122,14 @@ looked exactly like one where the feature was switched off. ### Fixed +- **Updating FunPack in the app installs the dependencies the update needs.** It pulled new + code and never new requirements, so a release that added one left the node pack unable to + import with nothing said. When an update touches `requirements.txt`, pip runs against the + interpreter ComfyUI is using — never a bare `pip`, which in a venv installs somewhere else + entirely — before the restart, and a failure names the command to run by hand rather than + turning a completed update into an error. Update and branch switch also moved off the event + loop; a fetch over a tunnel plus an install was freezing every stream in the meantime. + - **A LoRA that matches by name but not by shape now says so.** Key matching proves a LoRA is *for* a model, not that its weights fit it; a mismatched pair is dropped during the merge, which was visible only as one generic warning per key while FunPack's own status line still diff --git a/movie_editor/backend/git_update.py b/movie_editor/backend/git_update.py index b5768214..f3ccfb7d 100644 --- a/movie_editor/backend/git_update.py +++ b/movie_editor/backend/git_update.py @@ -4,6 +4,7 @@ from __future__ import annotations import re import shutil import subprocess +import sys from pathlib import Path REPO_ROOT = Path(__file__).resolve().parents[2] @@ -154,8 +155,65 @@ def status() -> dict: "codename": funpack_codename(), "detail": str(e)} -def pull(branch: str | None = None) -> dict: - """Fast-forward pull from origin on the given branch (current if omitted).""" +REQUIREMENTS = "requirements.txt" + + +def requirements_changed(before: str, after: str) -> bool: + """Did this update touch requirements.txt? + + The only honest trigger for installing: running pip on every pull would be slow and + surprising, and running it on none leaves an update that added a dependency looking + like a broken build instead of an unfinished install. + """ + if not before or not after or before == after: + return False + proc = _run_git("diff", "--name-only", f"{before}..{after}", "--", REQUIREMENTS) + if proc.returncode != 0: + # Cannot tell — install rather than skip. A redundant pip run costs seconds; a + # skipped one costs a broken node pack and a confusing error. + return True + return bool((proc.stdout or "").strip()) + + +def install_requirements(timeout: int = 900) -> dict: + """`pip install -r requirements.txt` into the interpreter ComfyUI is running. + + `sys.executable`, never a bare `pip`: ComfyUI is usually in a venv, and the pip on PATH + belongs to whatever else is on it. Installing into the wrong environment succeeds + loudly and changes nothing. + + Never raises. A failed install must not turn a completed update into an error — the + code IS updated by this point, and the remedy is a command the user can run. + """ + req = REPO_ROOT / REQUIREMENTS + if not req.is_file(): + return {"ran": False, "ok": True, "detail": "no requirements.txt in this checkout"} + cmd = [sys.executable, "-m", "pip", "install", "--disable-pip-version-check", + "-r", str(req)] + try: + proc = subprocess.run(cmd, cwd=REPO_ROOT, capture_output=True, text=True, + timeout=timeout) + except subprocess.TimeoutExpired: + return {"ran": True, "ok": False, + "detail": f"pip did not finish within {timeout}s — run it yourself: " + f"pip install -r {req}"} + except OSError as e: + return {"ran": True, "ok": False, "detail": f"could not run pip: {e}"} + if proc.returncode != 0: + tail = ((proc.stderr or proc.stdout or "").strip() or "pip failed")[-800:] + return {"ran": True, "ok": False, + "detail": f"pip install failed — run it yourself:\n" + f" {sys.executable} -m pip install -r {req}\n\n{tail}"} + return {"ran": True, "ok": True, "detail": (proc.stdout or "").strip()[-800:]} + + +def pull(branch: str | None = None, *, install_deps: bool = False) -> dict: + """Fast-forward pull from origin on the given branch (current if omitted). + + `install_deps` is OFF by default and turned on by the update route. Running pip is a + side effect no caller should acquire by accident just for asking git to move a branch — + it has to be asked for at the point that means "the user pressed Update". + """ branch = (branch or _current_branch()).strip() if not branch: raise GitUpdateError("Could not determine current branch.") @@ -188,11 +246,17 @@ def pull(branch: str | None = None) -> dict: raise GitUpdateError(msg) realigned = True after = _current_commit() + # Dependencies are part of the update. Done BEFORE the response, so the restart the + # caller schedules cannot race an install that is still running. + deps = None + if install_deps and before != after and requirements_changed(before, after): + deps = install_requirements() return { "branch": branch, "before": before, "after": after, "updated": before != after, + "requirements": deps, # Surfaced so the update reads as what it was, not a silent jump to another commit. "realigned": realigned, "output": ((pull_proc.stdout or "").strip() if not realigned @@ -201,7 +265,7 @@ def pull(branch: str | None = None) -> dict: } -def checkout(branch: str, *, pull_after: bool = True) -> dict: +def checkout(branch: str, *, pull_after: bool = True, install_deps: bool = False) -> dict: """Switch branch, optionally pull, return combined result.""" branch = (branch or "").strip() if not branch: @@ -219,9 +283,14 @@ def checkout(branch: str, *, pull_after: bool = True) -> dict: raise GitUpdateError((co.stderr or co.stdout or "git checkout failed").strip()) result = {"branch": branch, "before_branch": before_branch, "before": before_commit} if pull_after: - pulled = pull(branch) + pulled = pull(branch, install_deps=install_deps) result.update(pulled) else: result["after"] = _current_commit() result["updated"] = result["before"] != result["after"] + # A branch switch alone can cross a requirements change just as a pull can — the + # checkout above already moved the working tree onto the other branch's files. + if install_deps and result["updated"] and requirements_changed(before_commit, + result["after"]): + result["requirements"] = install_requirements() return result diff --git a/movie_editor/frontend/git_actions.js b/movie_editor/frontend/git_actions.js index fee4117a..483a7d6b 100644 --- a/movie_editor/frontend/git_actions.js +++ b/movie_editor/frontend/git_actions.js @@ -53,9 +53,10 @@ const msg = _restartOverlay(`Pulling origin/${branch}…\nComfyUI will restart when the pull finishes.`); try { const res = await API().gitUpdate(branch); - msg.textContent = res.updated - ? `Updated ${res.before} → ${res.after}.\nRestarting ComfyUI…` - : "Already up to date.\nRestarting ComfyUI…"; + const deps = _reportRequirements(res); + msg.textContent = (res.updated + ? `Updated ${res.before} → ${res.after}.${deps}\nRestarting ComfyUI…` + : "Already up to date.\nRestarting ComfyUI…"); } catch (e) { window.FunPackRestart?.removeOverlay?.(); alert("Update failed: " + (e.message || e)); @@ -64,6 +65,19 @@ _waitForComfyReload(msg, Date.now()); } + // An update that changed requirements.txt installs them before restarting. A FAILED + // install is the one outcome the user has to act on — the code is updated and the node + // pack will not import — so it interrupts rather than scrolling past in an overlay. + function _reportRequirements(res) { + const r = res && res.requirements; + if (!r || !r.ran) return ""; + if (r.ok) return "\nDependencies installed."; + alert("FunPack updated, but installing its dependencies failed.\n\n" + + (r.detail || "") + + "\n\nFunPack may not load until this is run."); + return "\nDependency install FAILED — see the message above."; + } + async function switchBranch() { const gs = await _ensureStatus(); if (!gs) return; @@ -85,9 +99,10 @@ const msg = _restartOverlay(`Switching to ${branch}…\nComfyUI will restart when ready.`); try { const res = await API().gitCheckout(branch); - msg.textContent = res.updated - ? `Switched to ${branch} (${res.before} → ${res.after}).\nRestarting ComfyUI…` - : `On ${branch}, already up to date.\nRestarting ComfyUI…`; + const deps = _reportRequirements(res); + msg.textContent = (res.updated + ? `Switched to ${branch} (${res.before} → ${res.after}).${deps}\nRestarting ComfyUI…` + : `On ${branch}, already up to date.\nRestarting ComfyUI…`); } catch (e) { window.FunPackRestart?.removeOverlay?.(); alert("Branch switch failed: " + (e.message || e)); diff --git a/movie_editor/server.py b/movie_editor/server.py index 3c7220c4..120612b8 100644 --- a/movie_editor/server.py +++ b/movie_editor/server.py @@ -2496,13 +2496,26 @@ if web is not None and PromptServer is not None: async def _git_status(_req): return web.json_response(git_update.status()) + # install_deps is opt-in on the library side: pressing Update is what means "install + # what this update needs", and no other caller of pull()/checkout() should acquire a + # pip run by accident. + def _pull_with_deps(branch): + return git_update.pull(branch, install_deps=True) + + def _checkout_with_deps(branch): + return git_update.checkout(branch, pull_after=True, install_deps=True) + @routes.post(UI_PREFIX + "/api/git/update") async def _git_update(req): import asyncio body = await req.json() if req.can_read_body else {} branch = body.get("branch") try: - result = git_update.pull(str(branch).strip() if branch else None) + # to_thread: git fetch over a tunnel takes seconds, and a requirements install + # can take a minute. Inline, both freeze the one event loop ComfyUI has — + # stalling every playing /result stream and the whole editor API meanwhile. + result = await asyncio.to_thread( + _pull_with_deps, str(branch).strip() if branch else None) except git_update.GitUpdateError as e: return web.json_response({"detail": str(e)}, status=400) asyncio.get_event_loop().call_later(0.7, _restart_comfy) @@ -2516,7 +2529,7 @@ if web is not None and PromptServer is not None: if not branch: return web.json_response({"detail": "Branch name is required."}, status=400) try: - result = git_update.checkout(branch, pull_after=True) + result = await asyncio.to_thread(_checkout_with_deps, branch) except git_update.GitUpdateError as e: return web.json_response({"detail": str(e)}, status=400) asyncio.get_event_loop().call_later(0.7, _restart_comfy) diff --git a/movie_editor/tests/test_git_update.py b/movie_editor/tests/test_git_update.py index 281d5079..290e21a5 100644 --- a/movie_editor/tests/test_git_update.py +++ b/movie_editor/tests/test_git_update.py @@ -1,5 +1,7 @@ """Git update helpers for the Movie Editor.""" +import sys +import types from types import SimpleNamespace import pytest @@ -169,3 +171,117 @@ def test_pull_keeps_the_original_error_when_it_cannot_prove_safety(monkeypatch): monkeypatch.setattr(git_update, "_run_git", fake_run) with pytest.raises(git_update.GitUpdateError): git_update.pull("dev") + + +# ── requirements installed as part of an update ─────────────────────────────── +# The gap this closes: an in-app update pulled new code but never new dependencies, so a +# release that added one left the node pack unable to import, with nothing said. + +def test_requirements_change_is_detected(monkeypatch): + from movie_editor.backend import git_update as gu + calls = [] + + def fake_git(*args, **kw): + calls.append(args) + return types.SimpleNamespace(returncode=0, stdout="requirements.txt\n", stderr="") + + monkeypatch.setattr(gu, "_run_git", fake_git) + assert gu.requirements_changed("aaa", "bbb") is True + assert any("diff" in a for a in calls[0]) + + +def test_untouched_requirements_are_not_reinstalled(monkeypatch): + from movie_editor.backend import git_update as gu + monkeypatch.setattr(gu, "_run_git", lambda *a, **k: types.SimpleNamespace( + returncode=0, stdout="", stderr="")) + assert gu.requirements_changed("aaa", "bbb") is False + + +def test_no_update_means_no_install(monkeypatch): + """Same commit before and after: nothing changed, so nothing to install.""" + from movie_editor.backend import git_update as gu + monkeypatch.setattr(gu, "_run_git", lambda *a, **k: pytest.fail("git should not run")) + assert gu.requirements_changed("aaa", "aaa") is False + assert gu.requirements_changed("", "bbb") is False + + +def test_an_undecidable_diff_installs_rather_than_skips(monkeypatch): + """A redundant pip run costs seconds; a skipped one costs a node pack that will not + import and an error that does not name its cause.""" + from movie_editor.backend import git_update as gu + monkeypatch.setattr(gu, "_run_git", lambda *a, **k: types.SimpleNamespace( + returncode=128, stdout="", stderr="bad object")) + assert gu.requirements_changed("aaa", "bbb") is True + + +def test_install_uses_the_running_interpreter(monkeypatch, tmp_path): + """Never a bare `pip`: ComfyUI is usually in a venv, and the pip on PATH belongs to + whatever else is on it — installing into the wrong environment succeeds and changes + nothing.""" + from movie_editor.backend import git_update as gu + seen = {} + + def fake_run(cmd, **kw): + seen["cmd"] = cmd + return types.SimpleNamespace(returncode=0, stdout="ok", stderr="") + + monkeypatch.setattr(gu.subprocess, "run", fake_run) + out = gu.install_requirements() + assert out["ran"] and out["ok"] + assert seen["cmd"][0] == sys.executable + assert seen["cmd"][1:4] == ["-m", "pip", "install"] + + +def test_a_failed_install_reports_the_command_and_never_raises(monkeypatch): + """The code IS updated by this point — turning that into an exception would leave the + user with a half-finished update and no instructions.""" + from movie_editor.backend import git_update as gu + monkeypatch.setattr(gu.subprocess, "run", lambda cmd, **kw: types.SimpleNamespace( + returncode=1, stdout="", stderr="No matching distribution found for gguf")) + out = gu.install_requirements() + assert out["ran"] is True and out["ok"] is False + assert "pip install -r" in out["detail"] + assert "No matching distribution" in out["detail"] + + +def test_a_pip_timeout_is_reported_not_raised(monkeypatch): + from movie_editor.backend import git_update as gu + + def boom(cmd, **kw): + raise gu.subprocess.TimeoutExpired(cmd, 900) + + monkeypatch.setattr(gu.subprocess, "run", boom) + out = gu.install_requirements() + assert out["ok"] is False and "did not finish" in out["detail"] + + +def test_pull_does_not_install_unless_asked(monkeypatch): + """Running pip is a side effect no caller should acquire just for moving a branch. + Turning this on by default made the test suite itself shell out to a real pip install + and hang — the default has to be off, with the update route opting in.""" + from movie_editor.backend import git_update as gu + monkeypatch.setattr(gu, "install_requirements", + lambda *a, **k: pytest.fail("pip must not run by default")) + monkeypatch.setattr(gu, "requirements_changed", lambda *a, **k: True) + monkeypatch.setattr(gu, "_is_dirty", lambda: False) + monkeypatch.setattr(gu, "_current_branch", lambda: "v4") + commits = iter(["aaa", "bbb"]) + monkeypatch.setattr(gu, "_current_commit", lambda: next(commits)) + monkeypatch.setattr(gu, "_run_git", lambda *a, **k: _proc(0, "")) + out = gu.pull("v4") + assert out["updated"] is True + assert out["requirements"] is None + + +def test_pull_installs_when_the_route_asks(monkeypatch): + from movie_editor.backend import git_update as gu + monkeypatch.setattr(gu, "install_requirements", + lambda *a, **k: {"ran": True, "ok": True, "detail": "done"}) + monkeypatch.setattr(gu, "requirements_changed", lambda *a, **k: True) + monkeypatch.setattr(gu, "_is_dirty", lambda: False) + monkeypatch.setattr(gu, "_current_branch", lambda: "v4") + commits = iter(["aaa", "bbb"]) + monkeypatch.setattr(gu, "_current_commit", lambda: next(commits)) + monkeypatch.setattr(gu, "_run_git", lambda *a, **k: _proc(0, "")) + out = gu.pull("v4", install_deps=True) + assert out["requirements"] == {"ran": True, "ok": True, "detail": "done"}