diff --git a/.gitignore b/.gitignore index b9607fc3..e36d7f0e 100644 --- a/.gitignore +++ b/.gitignore @@ -36,6 +36,7 @@ app/playwright-report/ /media/ /shortcuts.json /markers.json +/quarantine.json /shortcut_categories.json /shortcut_revolver.json diff --git a/app/legacy/api.js b/app/legacy/api.js index 92b201fd..36f4ec24 100644 --- a/app/legacy/api.js +++ b/app/legacy/api.js @@ -418,6 +418,7 @@ document.body.appendChild(link); link.click(); link.remove(); URL.revokeObjectURL(url); }, + releaseModule: (id) => j("POST", API("/api/control/release"), { id }), deleteRefinementKey: (key) => j("DELETE", API(`/api/m/taste/keys/${encodeURIComponent(key)}`)), absoluteStoreInfo: () => unsupported("refinement keys are not built in v5 yet"), clearAbsoluteStore: () => unsupported("refinement keys are not built in v5 yet"), diff --git a/app/legacy/engine_settings.js b/app/legacy/engine_settings.js index d83bdc63..eb978404 100644 --- a/app/legacy/engine_settings.js +++ b/app/legacy/engine_settings.js @@ -37,7 +37,7 @@ function categoriesWithContent() { const found = new Set(); - Object.values(PS.modulesById()).forEach((m) => { + PS.activeModules().forEach((m) => { if (Object.keys(m.settings || {}).length) found.add(m.category || ""); }); return CATEGORY_ORDER.filter((c) => found.has(c)); @@ -58,7 +58,7 @@ } if (!category || !cats.includes(category)) category = cats[0]; - const modulesInCategory = Object.values(PS.modulesById()) + const modulesInCategory = PS.activeModules() .filter((m) => (m.category || "") === category && Object.keys(m.settings || {}).length); const notes = PS.saveNotes(); diff --git a/app/legacy/index.html b/app/legacy/index.html index 175ba05b..ec07c2cb 100644 --- a/app/legacy/index.html +++ b/app/legacy/index.html @@ -154,6 +154,7 @@ + diff --git a/app/legacy/modules_control.js b/app/legacy/modules_control.js new file mode 100644 index 00000000..da44c40d --- /dev/null +++ b/app/legacy/modules_control.js @@ -0,0 +1,70 @@ +// Settings ▸ Modules: switch a module off for THIS project (it vanishes from Engine settings and does +// not run), and see the ones the server turned off after they failed, with a way to turn them back on. +// Only modules that modify a run are listed: loaders, the sampler and the like are structure. +(function () { + const { el, clear } = window.dom; + const PS = window.PipelineState; + const API = window.MovieEditorAPI; + const SF = window.SettingsField; + + const CATEGORY_LABELS = { + continuity: "Continuity", guidance: "Guidance", conditioning: "Conditioning", + sampling: "Sampling", post: "Post", system: "System", "": "Other", + }; + let _mounted = null; + + function render() { + if (!_mounted) return; + const { content } = _mounted; + clear(content); + const pane = el("div", "models-pane eng-pane"); + content.append(pane); + if (PS.loading()) { pane.append(SF.hintEl("Loading…")); return; } + if (PS.loadError()) { pane.append(SF.hintEl(`Could not load: ${PS.loadError()}`)); return; } + const control = PS.control(); + const mine = Object.values(PS.modulesById()).filter((m) => control[m.id] && control[m.id].controllable); + if (!mine.length) { pane.append(SF.hintEl("No installed module can be switched off.")); return; } + pane.append(SF.hintEl("A module you turn off here is off for this project only, and disappears from Engine " + + "settings. One that failed while generating is turned off for every project until its code is " + + "repaired or you turn it back on.")); + const byCat = {}; + mine.forEach((m) => { (byCat[m.category || ""] = byCat[m.category || ""] || []).push(m); }); + Object.keys(CATEGORY_LABELS).filter((c) => byCat[c]).forEach((c) => { + const g = SF.group(pane, CATEGORY_LABELS[c]); + byCat[c].forEach((m) => { + const held = control[m.id].quarantine; + if (held) { + const back = el("button", "btn ghost tiny", "Turn back on"); + back.onclick = async () => { + try { await API.releaseModule(m.id); } catch (e) { alert(`Could not turn ${m.title || m.id} back on: ${e.message || e}`); } + await PS.refreshControl(); render(); + }; + g.append(SF.field(`${m.title || m.id} — failed`, back, + `Off since ${held.when || "a failure"}: ${held.reason || "an error"}`)); + return; + } + const cb = el("input"); cb.type = "checkbox"; cb.checked = !PS.isOff(m.id); + cb.onchange = () => { PS.setOff(m.id, !cb.checked).then(render); }; + g.append(SF.toggleField(m.title || m.id, cb, PS.isOff(m.id) ? "Off for this project" : undefined)); + }); + }); + } + + function mount(body) { + const content = el("div", "models-mount eng-mount"); + body.append(content); + _mounted = { content }; + render(); + PS.ensureLoaded().then(() => PS.refreshControl()).then(render); + return () => { _mounted = null; }; + } + + window.SettingsWindow.register({ + id: "modules", group: "Generation", order: 3, title: "Modules", + subtitle: "Switch a module off for this project; see and re-enable ones that failed.", + keywords: "modules enable disable off quarantine failed repair switch", + iconBg: "linear-gradient(180deg,#6fd08c,#2f9e5a)", + icon: '', + mount, + }); +})(); diff --git a/app/legacy/pipeline_state.js b/app/legacy/pipeline_state.js index 9c2fc114..95360998 100644 --- a/app/legacy/pipeline_state.js +++ b/app/legacy/pipeline_state.js @@ -58,6 +58,7 @@ const manifest = await API.modules(traits ? traits.join(",") : undefined); modulesById = {}; (manifest.modules || []).forEach((m) => { modulesById[m.id] = m; }); + controlState = manifest.control || controlState; lastProbedFile = file; // only once it worked: a failed probe is tried again } let incomplete = []; @@ -85,7 +86,7 @@ // session already knows is installed, so an unrelated node's incidentally // JSON-shaped string can't inject a bogus entry that round-trips forever. function valuesAlreadyPlaced() { - const known = new Set(Object.keys(modulesById)); + const known = new Set([...Object.keys(modulesById), "_off"]); // _off: the modules this project turned off const merged = {}; (slots || []).forEach((slot) => { Object.values(slot.inputs || {}).forEach((v) => { @@ -231,7 +232,9 @@ Object.keys(pendingValues).forEach((moduleId) => { const pend = pendingValues[moduleId]; const real = already[moduleId] || {}; - Object.keys(pend).forEach((name) => { if (real[name] === pend[name]) delete pend[name]; }); + Object.keys(pend).forEach((name) => { + if (real[name] === pend[name] || (typeof pend[name] === "object" && JSON.stringify(real[name]) === JSON.stringify(pend[name]))) delete pend[name]; + }); if (!Object.keys(pend).length) delete pendingValues[moduleId]; }); } @@ -261,6 +264,27 @@ return merged; } + // Which modules this project turned off (a reserved entry of the same settings tree), and which + // the server has quarantined after a fault. Neither shows in Engine settings or the quick bar. + let controlState = {}; + function isOff(id) { + const off = (currentValues()._off || {}).modules; + return Array.isArray(off) && off.includes(id); + } + function setOff(id, off) { + const cur = new Set(((currentValues()._off || {}).modules) || []); + if (off) cur.add(id); else cur.delete(id); + return setModuleValue("_off", "modules", [...cur]); + } + async function refreshControl() { + try { controlState = (await API.modules()).control || {}; } catch (_) { /* keeps the last answer */ } + return controlState; + } + function isQuarantined(id) { return !!(controlState[id] && controlState[id].quarantine); } + function activeModules() { + return Object.values(modulesById).filter((m) => !isOff(m.id) && !isQuarantined(m.id)); + } + // Patch ONE field and save the WHOLE tree. place() (core/graph.py) writes // values as a single opaque JSON blob with no merge on the server side -- // sending only the module being edited would silently erase every other @@ -449,6 +473,7 @@ }).concat(extras ? saved.filter((s) => sound(s) && !have.has(s.id)) : []); // Replaces whatever pipeline was live: nothing queued for the old one may land on the new. const mine = ++epoch; + pendingValues = {}; // the new project's values win, not an edit made to the old one removed = new Set(gone); unwired = {}; Object.entries(unwiredMap && typeof unwiredMap === "object" ? unwiredMap : {}).forEach(([id, list]) => { @@ -482,6 +507,7 @@ window.PipelineState = { ensureLoaded, save, edit, restore, snapshot, setGroup, adopt, subscribe: (fn) => { listeners.add(fn); return () => listeners.delete(fn); }, valuesAlreadyPlaced, currentValues, setModuleValue, modulesById: () => modulesById, + activeModules, isOff, setOff, refreshControl, control: () => controlState, removedIds: () => [...removed], unwiredMap: () => JSON.parse(JSON.stringify(unwired)), slots: () => slots, diff --git a/app/legacy/pipeline_state.test.js b/app/legacy/pipeline_state.test.js index 43fa16d2..47e096fa 100644 --- a/app/legacy/pipeline_state.test.js +++ b/app/legacy/pipeline_state.test.js @@ -239,3 +239,26 @@ test("a refused restore leaves the pipeline as it is", async () => { assert.deepStrictEqual(res.refused, ["no"]); assert.strictEqual(PS.slots().find((s) => s.id === "model").inputs.file, "changed"); }); + +test("a module switched off is remembered with the settings, listed as off, and can be switched back on", async () => { + const posts = []; + const PS = load(posts); + await PS.ensureLoaded(); + global.window.MovieEditorAPI.modules = async () => ({ modules: [{ id: "sharpen", settings: {} }, { id: "alg", settings: {} }], control: {} }); + await PS.refreshControl(); + await PS.setOff("sharpen", true); + assert.strictEqual(PS.isOff("sharpen"), true); + assert.strictEqual(PS.isOff("alg"), false); + assert.deepStrictEqual(posts.at(-1).values._off, { modules: ["sharpen"] }); + await PS.setOff("alg", true); + await PS.setOff("sharpen", false); + assert.deepStrictEqual(posts.at(-1).values._off, { modules: ["alg"] }); +}); + +test("a project opened after another does not inherit the first one's pending switch", async () => { + const PS = load([]); + await PS.ensureLoaded(); + await PS.setOff("sharpen", true); + await PS.adopt([{ id: "model", node: "Loader", inputs: { file: "x" } }]); + assert.strictEqual(PS.isOff("sharpen"), false); +}); diff --git a/app/legacy/sampler_quickbar.js b/app/legacy/sampler_quickbar.js index ec2505f5..748ddca9 100644 --- a/app/legacy/sampler_quickbar.js +++ b/app/legacy/sampler_quickbar.js @@ -29,7 +29,7 @@ } function samplingModules() { - return Object.values(PS.modulesById()) + return PS.activeModules() .filter((m) => (m.category || "") === "sampling" && Object.keys(m.settings || {}).length); } diff --git a/app/legacy/store.js b/app/legacy/store.js index f239126f..6dd89823 100644 --- a/app/legacy/store.js +++ b/app/legacy/store.js @@ -3684,6 +3684,7 @@ const mediaList = [...(images || []), ...(audio || [])] .map((f) => ({ ...f, kind: _kindForFilename(f.filename) })); if (mediaList.length && _genRunSceneIds.length) _recordSegment(mediaList, _genRunSceneIds, { promptId }); + _announceQuarantine(); if (getEditorSetting("upscaleMode") === "always") upscaleMedia(mediaList.find((m) => m.kind === "videos")); set({ gen: { @@ -3692,6 +3693,7 @@ }, }); } else if (phase === "failed") { + _announceQuarantine(); set({ gen: { state: "error", promptId, media: [], @@ -4695,6 +4697,21 @@ } } + // A module that failed during a run is now off for good (until repaired): say which, once. + const _quarantineSeen = new Set(); + async function _announceQuarantine() { + const PS = window.PipelineState; + if (!PS || !PS.refreshControl) return; + const control = await PS.refreshControl(); + const fresh = Object.entries(control).filter(([id, c]) => c.quarantine && !_quarantineSeen.has(id)); + if (!fresh.length) return; + fresh.forEach(([id]) => _quarantineSeen.add(id)); + const title = (id) => (PS.modulesById()[id] || {}).title || id; + state.notice = fresh.map(([id, c]) => `${title(id)} failed and is now off (${c.quarantine.reason}). ` + + `Turn it back on in Settings ▸ Modules once it is repaired.`).join(" "); + notify(); + } + // ── pluggable models / exposed controls ────────────────────────────────────── // The pipeline lives in PipelineState (one copy shared by every panel); the project keeps a // copy of it (project.models) so a model pick and every node value travel with the project to diff --git a/conftest.py b/conftest.py index 72c7643d..56c66097 100644 --- a/conftest.py +++ b/conftest.py @@ -159,3 +159,10 @@ def packed_av(video, audio): def unpacked(x, latent_shapes): import comfy.utils return comfy.utils.unpack_latents(x, latent_shapes) + + +@pytest.fixture(autouse=True) +def _quarantine_in_tmp(tmp_path, monkeypatch): + """A test that makes a module fail must never write the real quarantine file.""" + from core import config + monkeypatch.setattr(config, "QUARANTINE_FILE", tmp_path / "quarantine.json") diff --git a/core/chain.py b/core/chain.py index 67fe6b49..764bd8b9 100644 --- a/core/chain.py +++ b/core/chain.py @@ -115,6 +115,8 @@ def build(specs, values, accepts: Sequence[str], dropped: patching.Dropped, member = make(values.get(spec.id) or {}, tuple(offered)) except Exception as exc: # noqa: BLE001 log.broke(f"{spec.id}.{capability}", exc, "starting up for this run") + from . import control + control.start_failed(spec, exc) notes.append(f"{spec.id}: failed to start -- {type(exc).__name__}: {exc}") continue diff --git a/core/config.py b/core/config.py index 96b4bc34..054e660b 100644 --- a/core/config.py +++ b/core/config.py @@ -39,3 +39,6 @@ SHORTCUT_CATEGORIES_FILE = ROOT / "shortcut_categories.json" # The words that cut a story into scenes (core/story.py). Global for the same # reason the shortcut library is. MARKERS_FILE = ROOT / "markers.json" + +# Modules that failed and stay off until repaired (core/control.py). State, so beside the library. +QUARANTINE_FILE = ROOT / "quarantine.json" diff --git a/core/control.py b/core/control.py new file mode 100644 index 00000000..fe1096d5 --- /dev/null +++ b/core/control.py @@ -0,0 +1,193 @@ +"""Which modules run: the project's own switch, and a module's persistent quarantine. + +A module that raises mid-run is dropped for that run (core/patching.py). That alone leaves a +faulty module to fail again on every generation, so a fault is also written down here and the +module stays OFF until it is repaired -- its code changed -- or the person turns it back on. +The project's switch is the person's own: a module they turned off vanishes for that project. + +Only modules that MODIFY a run can be switched (they provide `modifier` or `sampler_modifier`). +Loaders, the sampler and the like are structure: a pipeline without them is not a pipeline. +""" + +from __future__ import annotations + +import json +import os +import sys +import threading +import time +from pathlib import Path +from typing import Dict, Iterable, List, Tuple + +from . import config, log, patching + +#: Keys of the settings payload that are not module ids. +RESERVED = frozenset({"_off"}) + +_TRANSIENT = ("Interrupt", "OutOfMemory", "OOM") # not the module's fault: say nothing, remember nothing +_lock = threading.Lock() + + +def controllable(spec) -> bool: + return bool(spec.provides.get("modifier") or spec.provides.get("sampler_modifier")) + + +def off_by_project(settings) -> set: + """Module ids the project turned off, read from the settings payload.""" + raw = settings.get("_off") if isinstance(settings, dict) else None + ids = raw.get("modules") if isinstance(raw, dict) else None + return {i for i in ids if isinstance(i, str)} if isinstance(ids, list) else set() + + +def bad_off(raw) -> str | None: + """Why a payload's `_off` entry is malformed, or None.""" + if raw is None: + return None + ids = raw.get("modules") if isinstance(raw, dict) else None + if not isinstance(ids, list) or not all(isinstance(i, str) for i in ids): + return '"_off" must be {"modules": ["module_id", ...]}' + return None + + +# --- the quarantine file --------------------------------------------------- + + +def _file() -> Path: + return Path(config.QUARANTINE_FILE) + + +def _read() -> Dict[str, dict]: + try: + raw = json.loads(_file().read_text(encoding="utf-8")) + except (OSError, ValueError): + return {} + return {k: v for k, v in raw.items() if isinstance(k, str) and isinstance(v, dict)} if isinstance(raw, dict) else {} + + +def _write(entries: Dict[str, dict]) -> None: + path = _file() + tmp = path.with_suffix(".tmp") + try: + if not entries: + path.unlink(missing_ok=True) + return + tmp.write_text(json.dumps(entries, indent=1), encoding="utf-8") + os.replace(tmp, path) + except OSError as exc: + log.warning("module control", f"could not write {path.name}: {exc}. The module is off for this " + f"session only.") + + +def signature(spec) -> str: + """Changes when the module's code does: that is what "repaired" means.""" + mod = sys.modules.get(spec.source) + base = Path(getattr(mod, "__file__", "") or "").parent + parts = [] + if base.is_dir(): + for p in sorted(base.rglob("*.py")): + if "tests" in p.parts or "__pycache__" in p.parts: + continue + try: + st = p.stat() + except OSError: + continue + parts.append(f"{p.name}:{st.st_mtime_ns}:{st.st_size}") + return "|".join(parts) + + +def quarantined(specs: Iterable = ()) -> Dict[str, dict]: + """{module id: {reason, when}} for modules still quarantined. One whose code has changed since + it failed is released here, and said.""" + by_id = {s.id: s for s in specs} + with _lock: + entries = _read() + kept = {} + for mid, entry in entries.items(): + spec = by_id.get(mid) + if spec is not None and entry.get("sig") and entry["sig"] != signature(spec): + log.info("module control", f"{mid} was changed since it failed: trying it again.") + continue + kept[mid] = entry + if kept != entries: + _write(kept) + return kept + + +def quarantine(spec, reason: str) -> None: + with _lock: + entries = _read() + if spec.id in entries: + return + entries[spec.id] = {"reason": str(reason)[:400], "when": time.strftime("%Y-%m-%d %H:%M"), + "sig": signature(spec)} + _write(entries) + log.alert("module control", f"{spec.id} failed and is now OFF until it is repaired or you turn it " + f"back on (Settings ▸ Modules): {reason}") + + +def release(module_id: str) -> bool: + with _lock: + entries = _read() + if module_id not in entries: + return False + del entries[module_id] + _write(entries) + return True + + +def fault(key: str, exc: BaseException) -> None: + """Called by core/patching.Dropped when a module's hook raised. Faults that are not the module's + (an interrupt, running out of memory) are left alone.""" + if any(t in type(exc).__name__ for t in _TRANSIENT): + return + mid = key[len("funpack."):].split(".")[0] if key.startswith("funpack.") else None + if not mid: + return + from . import registry + spec = registry.current().specs.get(mid) + if spec is not None and controllable(spec): + quarantine(spec, f"{type(exc).__name__}: {exc}") + + +patching.on_fault = fault + + +def start_failed(spec, exc: BaseException) -> None: + """A module that raised while installing or starting up for a run.""" + if controllable(spec) and not any(t in type(exc).__name__ for t in _TRANSIENT): + quarantine(spec, f"{type(exc).__name__}: {exc}") + + +# --- what a run uses ------------------------------------------------------- + + +def partition(specs: Iterable, settings) -> Tuple[List, List[str]]: + """(kept, notes): `specs` without the ones the project turned off or that are quarantined.""" + specs = list(specs) + off = off_by_project(settings) + held = quarantined(specs) + kept, notes = [], [] + for spec in specs: + if controllable(spec): + if spec.id in off: + notes.append(f"{spec.id}: turned off for this project") + continue + if spec.id in held: + notes.append(f"{spec.id}: OFF -- it failed on {held[spec.id].get('when', '?')} " + f"({held[spec.id].get('reason', '')}). Turn it back on in Settings ▸ Modules.") + continue + kept.append(spec) + return kept, notes + + +def fingerprint() -> str: + """Part of a node's cache key: a module going into or out of quarantine changes what a run does.""" + return ",".join(sorted(_read())) + + +def state(specs: Iterable) -> Dict[str, dict]: + """What the Modules panel shows: {id: {controllable, quarantine?}} for every module.""" + specs = list(specs) + held = quarantined(specs) + return {s.id: {"controllable": controllable(s), **({"quarantine": held[s.id]} if s.id in held else {})} + for s in specs} diff --git a/core/patching.py b/core/patching.py index 925a342b..b8a290e9 100644 --- a/core/patching.py +++ b/core/patching.py @@ -111,6 +111,10 @@ def _strip_options(options: dict, prefix: str) -> int: # this" and "this happened", so it is said once, with the traceback, and carried # in the run's status. +#: Set by core/control.py: told when a module's hook raised, so the fault outlives the run. +on_fault = None + + class Dropped: """What was disabled during a run, and why. One per run, not per process.""" @@ -118,9 +122,10 @@ class Dropped: self.reasons: dict = {} def clear(self) -> None: - """Forget what was dropped. Called when a generation starts, because a - modifier that failed on the last one deserves its chance at this one -- - and because ComfyUI's cache hands the same object to both.""" + """Forget what was dropped. Called when a generation starts: this run's record starts + empty (a module that FAILED also stays off across runs: that is + core/control.py's quarantine, not this record) -- and because ComfyUI's cache hands the same + object to both.""" self.reasons.clear() def record(self, key: str, exc: BaseException) -> bool: @@ -128,6 +133,11 @@ class Dropped: if key in self.reasons: return False self.reasons[key] = f"{type(exc).__name__}: {exc}" + if on_fault is not None: + try: + on_fault(key, exc) + except Exception: # noqa: BLE001 -- bookkeeping must not end a run + pass return True def __contains__(self, key) -> bool: diff --git a/core/routes.py b/core/routes.py index 420303a4..33e449b7 100644 --- a/core/routes.py +++ b/core/routes.py @@ -8,7 +8,7 @@ thin adapters over pure functions in `serve`. import asyncio import json -from . import (backend_log, config, graph as graph_mod, log, media, nodes_manager, probe as probe_mod, +from . import (backend_log, config, control as control_mod, graph as graph_mod, log, media, nodes_manager, probe as probe_mod, projects, prompt_build, settings_card, @@ -141,6 +141,8 @@ def manifest(traits=None): "incompatible": [ {"id": spec.id, "requires": spec.requires} for spec in incompatible ], + # Which modules the person can switch, and which are quarantined after a fault. + "control": control_mod.state(ordered), } @@ -173,6 +175,18 @@ def register(routes, prefix=None): traits = [t for t in raw.split(",") if t] if raw is not None else None return web.json_response(manifest(traits)) + @routes.post(P + "/api/control/release") + async def _control_release(req): + """Turn a quarantined module back on: {"id": ""}.""" + try: + body = await req.json() + except ValueError: + body = None + mid = body.get("id") if isinstance(body, dict) else None + if not isinstance(mid, str) or mid not in modules().specs: + return web.json_response({"detail": "Name a module that is installed."}, status=400) + return web.json_response({"released": control_mod.release(mid)}) + def _pipeline(): """Whatever module offers a default pipeline, or nothing. diff --git a/core/tests/test_control.py b/core/tests/test_control.py new file mode 100644 index 00000000..7240f30e --- /dev/null +++ b/core/tests/test_control.py @@ -0,0 +1,73 @@ +"""Module control: the project's switch and the persistent quarantine.""" + +import types + +from core import control, patching + + +def spec(mid, provides=("modifier",), source="nowhere"): + return types.SimpleNamespace(id=mid, provides={p: (lambda *a, **k: None) for p in provides}, source=source) + + +def test_only_modules_that_modify_a_run_can_be_switched(): + assert control.controllable(spec("a", ("modifier",))) + assert control.controllable(spec("a", ("sampler_modifier",))) + assert not control.controllable(spec("loader", ("routes",))) + + +def test_a_module_the_project_turned_off_is_left_out_and_said(): + kept, notes = control.partition([spec("a"), spec("b")], {"_off": {"modules": ["a"]}}) + assert [s.id for s in kept] == ["b"] and "a: turned off for this project" in notes[0] + + +def test_structure_cannot_be_turned_off(): + kept, _ = control.partition([spec("loader", ("routes",))], {"_off": {"modules": ["loader"]}}) + assert [s.id for s in kept] == ["loader"] + + +def test_a_fault_is_remembered_across_runs_until_released(): + a = spec("a") + control.quarantine(a, "RuntimeError: boom") + kept, notes = control.partition([a], {}) + assert kept == [] and "a: OFF" in notes[0] and "boom" in notes[0] and "Settings ▸ Modules" in notes[0] + assert control.fingerprint() == "a" + assert control.release("a") and not control.release("a") + assert [s.id for s in control.partition([a], {})[0]] == ["a"] + + +def test_a_module_whose_code_changed_since_it_failed_gets_another_try(monkeypatch): + a = spec("a") + monkeypatch.setattr(control, "signature", lambda s: "v1") + control.quarantine(a, "boom") + monkeypatch.setattr(control, "signature", lambda s: "v2") + assert [s.id for s in control.partition([a], {})[0]] == ["a"] + assert control.fingerprint() == "" + + +def test_an_interrupt_or_out_of_memory_is_not_the_modules_fault(monkeypatch): + from core import registry + a = spec("a") + monkeypatch.setattr(registry, "current", lambda: types.SimpleNamespace(specs={"a": a})) + + class InterruptProcessingException(Exception): ... + class OutOfMemoryError(Exception): ... + + control.fault("funpack.a", InterruptProcessingException()) + control.fault("funpack.a", OutOfMemoryError()) + assert control.fingerprint() == "" + control.fault("funpack.a", ValueError("bad")) + assert control.fingerprint() == "a" + + +def test_a_hook_that_raised_in_a_run_reaches_the_quarantine(monkeypatch): + from core import registry + a = spec("a") + monkeypatch.setattr(registry, "current", lambda: types.SimpleNamespace(specs={"a": a})) + dropped = patching.Dropped() + assert dropped.record("funpack.a", RuntimeError("step 3")) + assert "a" in control.quarantined([a]) and "step 3" in control.quarantined([a])["a"]["reason"] + + +def test_a_malformed_off_entry_is_named(): + assert control.bad_off({"modules": ["a"]}) is None and control.bad_off(None) is None + assert "modules" in control.bad_off(["a"]) and "modules" in control.bad_off({"modules": [1]}) diff --git a/core/tests/test_routes_pipeline.py b/core/tests/test_routes_pipeline.py index ca7e0cf0..2ac45600 100644 --- a/core/tests/test_routes_pipeline.py +++ b/core/tests/test_routes_pipeline.py @@ -515,3 +515,19 @@ def test_a_module_provided_preset_is_offered(server, registered): preset = next(p for p in body["presets"] if p["id"] == "minimax_h3_reference_to_video") assert preset["module"] == "model_minimax_h3" assert preset["slots"] + + +def test_the_manifest_says_which_modules_can_be_switched_and_a_quarantined_one_can_be_released(server): + from core import control, registry + status, body = _request(server, "GET", "/funpack/api/modules") + assert status == 200 and body["control"] + sharp = registry.current().specs["sharpen"] + assert body["control"]["sharpen"]["controllable"] is True + control.quarantine(sharp, "RuntimeError: boom") + _, body = _request(server, "GET", "/funpack/api/modules") + assert body["control"]["sharpen"]["quarantine"]["reason"].endswith("boom") + status, body = _request(server, "POST", "/funpack/api/control/release", {"id": "sharpen"}) + assert (status, body) == (200, {"released": True}) + _, body = _request(server, "GET", "/funpack/api/modules") + assert "quarantine" not in body["control"]["sharpen"] + assert _request(server, "POST", "/funpack/api/control/release", {"id": "nope"})[0] == 400 diff --git a/modules/_core.py b/modules/_core.py index 2463a519..ce4d2d14 100644 --- a/modules/_core.py +++ b/modules/_core.py @@ -11,8 +11,8 @@ leading underscore keeps it out of the module scan. """ try: # root on sys.path: tests, dev server - from core import chain, config, contract, dit_hooks, input_steer, log, media, patching, projects, rated_dial, registry, relations, run, schema, shortcuts, streams, temp_files, traits + from core import chain, config, contract, control, dit_hooks, input_steer, log, media, patching, projects, rated_dial, registry, relations, run, schema, shortcuts, streams, temp_files, traits except ImportError: # inside ComfyUI: a subpackage of the pack - from ..core import chain, config, contract, dit_hooks, input_steer, log, media, patching, projects, rated_dial, registry, relations, run, schema, shortcuts, streams, temp_files, traits + from ..core import chain, config, contract, control, dit_hooks, input_steer, log, media, patching, projects, rated_dial, registry, relations, run, schema, shortcuts, streams, temp_files, traits -__all__ = ["chain", "config", "contract", "dit_hooks", "input_steer", "log", "media", "patching", "projects", "rated_dial", "registry", "relations", "run", "schema", "shortcuts", "streams", "temp_files", "traits"] +__all__ = ["chain", "config", "contract", "control", "dit_hooks", "input_steer", "log", "media", "patching", "projects", "rated_dial", "registry", "relations", "run", "schema", "shortcuts", "streams", "temp_files", "traits"] diff --git a/modules/sampling/modifiers/nodes.py b/modules/sampling/modifiers/nodes.py index f9034f69..148c1624 100644 --- a/modules/sampling/modifiers/nodes.py +++ b/modules/sampling/modifiers/nodes.py @@ -33,7 +33,7 @@ import json from comfy_api.latest import io -from ..._core import (log, patching, registry as registry_mod, +from ..._core import (control as control_mod, log, patching, registry as registry_mod, relations as relations_mod, run as run_mod, schema as schema_mod) @@ -120,6 +120,11 @@ class FunPackModifierSettings(io.ComfyNode): specs = registry_mod.current().specs problems = [] for module_id, values in raw.items(): + if module_id in control_mod.RESERVED: + said = control_mod.bad_off(values) + if said: + problems.append(said) + continue spec = specs.get(module_id) if spec is None: problems.append(f"no module named {module_id!r} is installed") @@ -152,6 +157,13 @@ class FunPackModifierSettings(io.ComfyNode): guards_off = cls._guards_off(raw) problems, checked = [], {} for module_id, values in raw.items(): + if module_id in control_mod.RESERVED: + said = control_mod.bad_off(values) + if said: + problems.append(said) + else: + checked[module_id] = values + continue spec = specs.get(module_id) if spec is None: # Silently ignoring it is how v4 accumulated settings that meant @@ -192,8 +204,9 @@ class FunPackLoadModifiers(io.ComfyNode): @classmethod def fingerprint_inputs(cls, **_): - return "|".join(f"{spec.id}={answer()}" - for spec, answer in registry_mod.current().providers(CACHE_KEY)) + return "|".join([f"{spec.id}={answer()}" + for spec, answer in registry_mod.current().providers(CACHE_KEY)] + + ["quarantine=" + control_mod.fingerprint()]) @classmethod def execute(cls, model, settings=None) -> io.NodeOutput: @@ -214,6 +227,7 @@ class FunPackLoadModifiers(io.ComfyNode): offering = [spec for spec in specs if spec.provides.get(CAPABILITY)] compatible, incompatible = traits_mod.split(offering, available) + compatible, switched = control_mod.partition(compatible, settings) ordered, rejected = order(compatible) patched = model.clone() @@ -241,6 +255,7 @@ class FunPackLoadModifiers(io.ComfyNode): "these settings are yours.") if stripped: notes.append(f"replaced {stripped} modifier(s) already on this model") + notes.extend(switched) # Checked HERE, not only where the payload was made. A FUNPACK_SETTINGS # socket is typed by a string tag, so a dict can reach this node without @@ -270,6 +285,7 @@ class FunPackLoadModifiers(io.ComfyNode): # Absent, and said out loud. A modifier that half-installed and # carried on is how a run silently stops meaning what it says. log.broke(f"{spec.id}.{CAPABILITY}", exc, "installing itself") + control_mod.start_failed(spec, exc) notes.append(f"{spec.id}: failed to install -- {type(exc).__name__}: {exc}") continue if note is None: diff --git a/modules/sampling/sampler/nodes.py b/modules/sampling/sampler/nodes.py index 47a7e93f..1e7bebf4 100644 --- a/modules/sampling/sampler/nodes.py +++ b/modules/sampling/sampler/nodes.py @@ -27,7 +27,7 @@ import torch from comfy_api.latest import io from ..._core import chain as chain_mod, log, patching, registry as registry_mod, run as run_mod -from ..._core import relations as relations_mod, schema as schema_mod, traits as traits_mod +from ..._core import control as control_mod, relations as relations_mod, schema as schema_mod, traits as traits_mod # The hook points this sampler offers. A modifier asking for anything else is # absent and said, rather than half-running. @@ -201,6 +201,7 @@ class FunPackSampler(io.ComfyNode): offering = [s for s in specs if s.provides.get("sampler_modifier")] compatible, incompatible = traits_mod.split(offering, available) + compatible, switched = control_mod.partition(compatible, settings) ordered, rejected = relations_mod.order(compatible) guards_off = any(answer(settings or {}) for _spec, answer @@ -218,7 +219,7 @@ class FunPackSampler(io.ComfyNode): notes.append(f"{spec.id}: needs {', '.join(traits_mod.missing_for(spec, available))}") for spec, why in rejected: notes.append(f"{spec.id}: {why}") - return chain, notes + return chain, notes + switched @classmethod def _with_chain(cls, sampler, chain, sigmas, run): diff --git a/modules/sampling/tests/test_a_module_cannot_kill_a_run.py b/modules/sampling/tests/test_a_module_cannot_kill_a_run.py index 385528dd..96c6aaed 100644 --- a/modules/sampling/tests/test_a_module_cannot_kill_a_run.py +++ b/modules/sampling/tests/test_a_module_cannot_kill_a_run.py @@ -136,11 +136,32 @@ def test_the_record_is_per_run_not_per_process(patcher): hook(_pre_cfg_args([torch.randn(1, 4, 8, 16, 16), torch.randn(1, 4, 8, 16, 16)])) return patched.funpack_dropped - first, second = run(), run() + from core import control + first = run() + assert control.release("sharpen") # a fault is also remembered across runs: see core/tests/test_control.py + second = run() assert first is not second assert "funpack.sharpen" in first and "funpack.sharpen" in second +def test_a_module_that_failed_stays_off_for_the_next_generation_until_turned_back_on(patcher): + import torch + from core import control + from modules.sampling.modifiers.nodes import FunPackLoadModifiers + + values = {"sharpen": {"enabled": True, "amount": 0.4}} + first, _ = FunPackLoadModifiers.execute(patcher, settings=values).result + first.model_options["sampler_pre_cfg_function"][0]( + _pre_cfg_args([torch.randn(1, 4, 8, 16, 16), torch.randn(1, 4, 8, 16, 16)])) + assert "sharpen" in control.quarantined(__import__("core.registry", fromlist=["x"]).current().specs.values()) + second, status = FunPackLoadModifiers.execute(patcher, settings=values).result + assert "sampler_pre_cfg_function" not in second.model_options + assert "sharpen: OFF" in status and "Settings ▸ Modules" in status + control.release("sharpen") + third, _ = FunPackLoadModifiers.execute(patcher, settings=values).result + assert "sampler_pre_cfg_function" in third.model_options + + def test_a_hook_shape_with_no_known_neutral_result_is_named(patcher): """Honesty about the limit: an unguardable hook is installed and said out loud, rather than counted as protected.""" @@ -207,6 +228,8 @@ def test_the_guards_are_on_unless_asked_otherwise(patcher): original = video[0].clone() out = hook(_pre_cfg_args(video)) # must not raise assert torch.equal(out[0], original) + from core import control + control.release("sharpen") # it faulted on purpose; each case starts with it on def test_is_on_reads_the_switch_the_same_way_everywhere(): diff --git a/modules/sampling/tests/test_modifier_settings.py b/modules/sampling/tests/test_modifier_settings.py index cc1086df..971c85cd 100644 --- a/modules/sampling/tests/test_modifier_settings.py +++ b/modules/sampling/tests/test_modifier_settings.py @@ -156,3 +156,13 @@ def test_validation_catches_the_same_things_execution_would(registry, payload, e with pytest.raises(RuntimeError): _run(payload) + + +def test_the_projects_off_list_passes_through_and_is_not_mistaken_for_a_module(registry): + out, _status = _run('{"alg": {"enabled": true}, "_off": {"modules": ["sharpen"]}}') + assert out["_off"] == {"modules": ["sharpen"]} and out["alg"]["enabled"] is True + + +def test_a_malformed_off_list_is_refused_with_a_reason(registry): + with pytest.raises(RuntimeError, match="modules"): + _run('{"_off": ["sharpen"]}')