foundation: two the whole-tree pass found, both against the real engine
The run id froze after the first generation. Runs were marked by the modifiers node, and ComfyUI CACHES a node whose inputs have not changed -- only the seed changes between most generations, so that node executed once and every later run reported under the first one's id. Per-run log dedup degraded to per-process with it, which is precisely the v4 fault log.py's own comment describes, reintroduced through a caching interaction the fix did not account for. The sampler marks the run now. It is what generates, and it re-executes whenever anything about the generation differs. Verified through the real PromptExecutor with caching on: three seeds, three run ids. The dropped-modifier record is also emptied there, because it travels on the model and ComfyUI may hand the same model back -- a modifier that failed last time deserves this time. And a bad settings payload was only caught during execution, after the checkpoint had already loaded. On a rented GPU that is the cost paid before the guard fires, one layer up from the sampling case this project already guards. ComfyUI calls validate_inputs inside validate_prompt, so the payload is now understood before the queue accepts it: a typo'd module id is refused with nothing loaded. Both doors check the same things, and a test pins that they agree. There was no test file for runs at all, which is why the first fault could be introduced by a fix and survive a green suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
cf318c8c0d
commit
0f31547e1e
@@ -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:
|
||||
|
||||
@@ -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
|
||||
@@ -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]))
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user