feat(update): install requirements when an update changes them
Pulled code but never dependencies, so a release adding one broke the import silently. pip runs against sys.executable, opt-in from the route, off the event loop; failures name the manual command.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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));
|
||||
|
||||
+15
-2
@@ -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)
|
||||
|
||||
@@ -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"}
|
||||
|
||||
Reference in New Issue
Block a user