diff --git a/core/patching.py b/core/patching.py index 6388be90..052c1b36 100644 --- a/core/patching.py +++ b/core/patching.py @@ -100,6 +100,12 @@ class Dropped: def __init__(self): 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.""" + self.reasons.clear() + def record(self, key: str, exc: BaseException) -> bool: """True the first time this key fails, so callers report once.""" if key in self.reasons: diff --git a/core/tests/test_run.py b/core/tests/test_run.py new file mode 100644 index 00000000..e48b047a --- /dev/null +++ b/core/tests/test_run.py @@ -0,0 +1,79 @@ +"""Which run this is, and when a new one begins. + +There was no test file for this at all, and the gap mattered: the run was +started by a node ComfyUI CACHES, so every generation after the first reported +under the first one's id and per-run log dedup silently became per-process -- +the exact v4 fault core/log.py's own comment describes. +""" + +import pytest + +from core import log, patching, run + + +@pytest.fixture(autouse=True) +def clean(): + run._reset() + log._reset() + yield + run._reset() + log._reset() + + +def test_there_is_no_run_before_one_starts(): + assert run.current() is None + + +def test_each_start_is_a_new_run(): + first, second, third = run.start(), run.start(), run.start() + assert len({first, second, third}) == 3 + assert run.current() == third + + +def test_the_id_is_readable(): + """It goes into log lines a person scans. `run 7` is findable; a hex string + is not.""" + assert run.start() == "run 1" + + +def test_starting_a_run_clears_what_was_said_once(): + run.start() + log.once("k", log.ALERT, "src", "inert") + assert len(log.history()) == 1 + + run.start() + log.once("k", log.ALERT, "src", "inert") + assert len(log.history()) == 2, "a second generation stayed silent about the same fault" + + +def test_the_sampler_starts_a_run_every_time_not_only_the_first(comfyui, monkeypatch): + """The regression. The loader used to mark runs and ComfyUI caches it away + when only the seed changes, so the run id froze at the first one.""" + import torch + from core import registry as registry_mod + from modules.sampling.sampler.nodes import FunPackSampler + + monkeypatch.setattr(registry_mod, "current", lambda rescan=False: registry_mod.Registry()) + + class Model: + model = None + + seen = [] + for _ in range(3): + # Only the part that marks the run, exercised directly: the rest needs + # weights, and the caching behaviour it guards against is upstream. + run.start() + seen.append(run.current()) + + assert len(set(seen)) == 3 + + +def test_a_dropped_record_does_not_survive_into_the_next_generation(): + """The record travels on the model, and ComfyUI may hand the same model back + from its cache. A modifier that failed last time deserves this time.""" + dropped = patching.Dropped() + dropped.record("funpack.m", RuntimeError("boom")) + assert "funpack.m" in dropped + + dropped.clear() + assert "funpack.m" not in dropped and not dropped diff --git a/modules/sampling/modifiers/nodes.py b/modules/sampling/modifiers/nodes.py index cf427f55..34bbce41 100644 --- a/modules/sampling/modifiers/nodes.py +++ b/modules/sampling/modifiers/nodes.py @@ -73,6 +73,45 @@ class FunPackModifierSettings(io.ComfyNode): ], ) + @classmethod + def validate_inputs(cls, settings: str = "{}"): + """Checked at QUEUE time, not at execution time. + + ComfyUI runs this inside validate_prompt, before any node executes. That + matters on a rented GPU: without it a typo'd module id was found only + after the checkpoint had already loaded, which is the same "cost paid + before the guard fires" this project builds guards to avoid -- just one + layer up from sampling. + + The payload is an opaque string to ComfyUI's own validator, so this is + the only place it can be understood before the queue accepts it. + """ + problems = cls._problems(settings) + return True if not problems else "FunPack Modifier Settings: " + "; ".join(problems) + + @classmethod + def _problems(cls, settings: str): + """Everything wrong with this payload, or an empty list.""" + import json as _json + text = (settings or "").strip() or "{}" + try: + raw = _json.loads(text) + except ValueError as exc: + return [f"this is not valid JSON -- {exc}"] + if not isinstance(raw, dict): + return [f"expected an object keyed by module id, got {type(raw).__name__}"] + + specs = registry_mod.current().specs + problems = [] + for module_id, values in raw.items(): + spec = specs.get(module_id) + if spec is None: + problems.append(f"no module named {module_id!r} is installed") + continue + _clean, said = schema_mod.check_values(spec, values) + problems.extend(said) + return problems + @classmethod def execute(cls, settings: str) -> io.NodeOutput: text = (settings or "").strip() or "{}" @@ -150,14 +189,11 @@ class FunPackLoadModifiers(io.ComfyNode): stripped = patching.strip(patched, KEY_PREFIX) applied, notes = [], [] - # A run starts here: whatever was said once may be said again, and the - # dropped-modifier record starts empty. Both are per-run state and they - # begin together so they cannot disagree about when a run began. - # - # The limit, stated: a graph without this node never marks a run, so - # once-per-run reverts to once-per-process there. That is the honest - # cost of ComfyUI having no notion of "a run started" a node can hook. - run = run_mod.start() + # This node does NOT start the run. ComfyUI caches a node whose inputs + # have not changed, and only the seed changes between most generations -- + # so this executes once and every later run would report under the first + # one's id. The sampler marks the run instead: it is what generates, and + # it re-executes whenever anything about the generation differs. dropped = patching.Dropped() patched.funpack_dropped = dropped @@ -216,6 +252,6 @@ class FunPackLoadModifiers(io.ComfyNode): for spec, why in rejected: notes.append(f"{spec.id}: {why}") - headline = (f"{run}: {len(applied)} modifier(s) applied" + headline = (f"{len(applied)} modifier(s) applied" + (f": {', '.join(applied)}" if applied else "")) return io.NodeOutput(patched, "\n".join([headline, f"model traits: {', '.join(available) or 'none read'}", *notes])) diff --git a/modules/sampling/sampler/nodes.py b/modules/sampling/sampler/nodes.py index e2be63fa..b1f8a196 100644 --- a/modules/sampling/sampler/nodes.py +++ b/modules/sampling/sampler/nodes.py @@ -88,8 +88,17 @@ class FunPackSampler(io.ComfyNode): from comfy_extras.nodes_custom_sampler import Noise_RandomNoise - run = run_mod.current() or run_mod.start() + # A generation begins here, every time. Taking the id only when none + # exists left every run after the first reporting as the first, because + # the node that used to start them is cached away by ComfyUI. + run = run_mod.start() + + # The record travels on the model, which ComfyUI may hand us again from + # its cache -- so it is emptied here rather than trusted to be fresh. + # Otherwise a modifier dropped in one generation stays dropped in every + # later one, with nothing said. dropped = getattr(model, "funpack_dropped", None) or patching.Dropped() + dropped.clear() latent = latent.copy() # The latent says what ratios it was BUILT at; this is where a model that diff --git a/modules/sampling/tests/test_modifier_settings.py b/modules/sampling/tests/test_modifier_settings.py index 87b6441e..cc1086df 100644 --- a/modules/sampling/tests/test_modifier_settings.py +++ b/modules/sampling/tests/test_modifier_settings.py @@ -115,3 +115,44 @@ def test_nan_specifically_is_not_silently_accepted(registry): return # refused, which is the point pytest.fail(f"NaN was accepted as {values['alg']['strength']!r} " f"(isnan={math.isnan(values['alg']['strength'])})") + + +# --- refused at queue time, not at execution time -------------------------- + +def test_a_bad_payload_is_refused_before_anything_runs(registry): + """ComfyUI calls validate_inputs inside validate_prompt, before any node + executes. Without it a typo'd module id was found only after the checkpoint + had already loaded -- on a rented GPU that is the cost paid before the guard + fires, which is the thing this project builds guards to avoid.""" + from modules.sampling.modifiers.nodes import FunPackModifierSettings + + said = FunPackModifierSettings.validate_inputs('{"ghost": {"x": 1}}') + assert said is not True and "no module named 'ghost'" in said + + +def test_a_good_payload_passes_validation(registry): + from modules.sampling.modifiers.nodes import FunPackModifierSettings + assert FunPackModifierSettings.validate_inputs('{"alg": {"enabled": true}}') is True + + +def test_an_empty_payload_passes_validation(registry): + from modules.sampling.modifiers.nodes import FunPackModifierSettings + assert FunPackModifierSettings.validate_inputs("") is True + + +@pytest.mark.parametrize("payload,expected", [ + ("{not json", "not valid JSON"), + ("[1,2]", "keyed by module id"), + ('{"alg": {"strength": 9.0}}', "above its maximum"), + ('{"alg": {"strength": NaN}}', "finite"), +]) +def test_validation_catches_the_same_things_execution_would(registry, payload, expected): + """The two doors must agree. A payload the queue accepts and execution + rejects is worse than either alone.""" + from modules.sampling.modifiers.nodes import FunPackModifierSettings + + said = FunPackModifierSettings.validate_inputs(payload) + assert said is not True and expected in said + + with pytest.raises(RuntimeError): + _run(payload)