diff --git a/core/schema.py b/core/schema.py index 2dac4fb8..082c4310 100644 --- a/core/schema.py +++ b/core/schema.py @@ -5,7 +5,7 @@ does not get a best-effort panel -- it does not load at all, and says why once. That is the same rule the UI follows: absent, not approximate. """ -from typing import Any, Dict +from typing import Any, Dict, List, Tuple from .contract import NUMERIC, STAGES, TYPES, UI_HINTS, ModuleSpec @@ -186,6 +186,69 @@ def validate_provides(value: Any, module_id: str) -> dict: return dict(value) +def check_values(spec, values: Any) -> Tuple[Dict[str, Any], List[str]]: + """One module's live values, checked against its own declaration. + + Returns (values, problems). Anything missing falls back to the declared + default, so a partial payload is fine; anything WRONG is reported rather than + coerced. v4 carried these as JSON in a string widget that nothing validated, + which is why it needed a hand-maintained mirror of every default and a list of + keys that no longer meant anything. + """ + problems: List[str] = [] + if values is None: + return spec.defaults(), problems + if not isinstance(values, dict): + return spec.defaults(), [f"{spec.id}: settings must be an object, got {type(values).__name__}."] + + clean = spec.defaults() + for key, value in values.items(): + declared = spec.settings.get(key) + if declared is None: + # Not repaired by dropping it silently: a key nobody declares is + # either a typo or a leftover, and both are worth saying once. + problems.append(f"{spec.id}: no setting named {key!r}.") + continue + problem = _value_problem(declared, key, value, spec.id) + if problem: + problems.append(problem) + continue + clean[key] = value + return clean, problems + + +def _value_problem(declared: dict, key: str, value: Any, module_id: str): + kind = declared["type"] + where = f"{module_id}.{key}" + + if kind == "bool": + if not isinstance(value, bool): + return f"{where} is a bool, got {value!r}." + return None + + if kind == "enum": + allowed = [option["value"] for option in declared["options"]] + if value not in allowed: + return f"{where} must be one of {allowed}, got {value!r}." + return None + + if kind in NUMERIC: + if isinstance(value, bool) or not isinstance(value, (int, float)): + return f"{where} is a {kind}, got {value!r}." + if kind == "int" and not isinstance(value, int): + return f"{where} is an int, got {value!r}." + low, high = declared.get("min"), declared.get("max") + if low is not None and value < low: + return f"{where} is below its minimum {low}: {value}." + if high is not None and value > high: + return f"{where} is above its maximum {high}: {value}." + return None + + if not isinstance(value, str): + return f"{where} is a {kind}, got {value!r}." + return None + + def validate(announcement: Dict[str, Any], source: str = "") -> ModuleSpec: """A validated ModuleSpec, or SchemaError naming what is wrong.""" for required in ("id", "title"): diff --git a/core/tests/test_shipped_nodes.py b/core/tests/test_shipped_nodes.py index bb29733f..25fae92e 100644 --- a/core/tests/test_shipped_nodes.py +++ b/core/tests/test_shipped_nodes.py @@ -46,6 +46,8 @@ FROZEN = { "FunPackCLIPLoader": [ "clip_name1", "clip_name2", "clip_name3", "clip_name4", "type", "device", ], + "FunPackLoadModifiers": ["model", "settings"], + "FunPackModifierSettings": ["settings"], "FunPackEmptyLatent": ["model", "width", "height", "length", "batch_size"], "FunPackDiffusionModelLoader": [ "model_name", "weight_dtype", "compute_dtype", "attention", "fp16_accumulation", diff --git a/modules/sampling/__init__.py b/modules/sampling/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/modules/sampling/modifiers/__init__.py b/modules/sampling/modifiers/__init__.py new file mode 100644 index 00000000..da5e318f --- /dev/null +++ b/modules/sampling/modifiers/__init__.py @@ -0,0 +1,10 @@ +"""The modifier nodes. A node-only module.""" + +from .nodes import FunPackLoadModifiers, FunPackModifierSettings + +ID = "sampling_modifiers" +TITLE = "Modifiers" +STAGE = "sampling" +STATUS = "proven" + +NODES = [FunPackModifierSettings, FunPackLoadModifiers] diff --git a/modules/sampling/modifiers/nodes.py b/modules/sampling/modifiers/nodes.py new file mode 100644 index 00000000..f5dd2b65 --- /dev/null +++ b/modules/sampling/modifiers/nodes.py @@ -0,0 +1,152 @@ +"""Modifiers: what the user chose, and putting it on the model. + +Two nodes, because the two jobs are genuinely different. One carries the values a +person picked; the other reads what is installed and applies whatever announced +itself. Neither names a modifier, and no sampler is involved in either. + +**Modifiers attach to the MODEL, not to a sampler.** They install through +ComfyUI's own `patcher_extension` wrappers on a CLONE of the patcher, which has +two consequences worth stating: + +* Wire the resulting model into ComfyUI's own SamplerCustomAdvanced and every + per-step modifier still runs. The sampler's cooperation is not required, so no + sampler can own them -- which is the whole architectural claim, made physical. +* `ModelPatcher.clone()` copies the wrapper lists per key, so installing on the + clone cannot touch the model everyone else is holding. Accumulation across runs + -- the fault that once degraded output for a hundred generations and survived + every reset short of a restart -- is structurally impossible here rather than + something to remember to clean up. +""" + +import json + +from comfy_api.latest import io + +from ..._core import log, registry as registry_mod, schema as schema_mod + +CAPABILITY = "modifier" + +# Namespaced so removing ours never touches a wrapper somebody else installed. +KEY_PREFIX = "funpack." + + +class FunPackModifierSettings(io.ComfyNode): + """The values a person picked, on their way to the graph. + + One widget, not one per setting. A socket per setting would mean this node's + shape changing every time any module gained an option, and every saved + workflow referring to sockets that had moved. + """ + + @classmethod + def define_schema(cls) -> io.Schema: + return io.Schema( + node_id="FunPackModifierSettings", + display_name="FunPack Modifier Settings", + category="FunPack/Sampling", + description="Settings for every FunPack modifier, as one payload.", + inputs=[ + io.String.Input( + "settings", multiline=True, default="{}", + tooltip='{"module_id": {"setting": value}}. The app writes this; ' + 'editing it by hand is fine, and mistakes are reported.'), + ], + outputs=[ + io.Custom("FUNPACK_SETTINGS").Output(display_name="settings"), + io.String.Output(display_name="status"), + ], + ) + + @classmethod + def execute(cls, settings: str) -> io.NodeOutput: + text = (settings or "").strip() or "{}" + try: + raw = json.loads(text) + except ValueError as exc: + raise RuntimeError(f"FunPack Modifier Settings: this is not valid JSON -- {exc}") + if not isinstance(raw, dict): + raise RuntimeError( + f"FunPack Modifier Settings: expected an object keyed by module id, " + f"got {type(raw).__name__}.") + + specs = registry_mod.current().specs + problems, checked = [], {} + for module_id, values in raw.items(): + spec = specs.get(module_id) + if spec is None: + # Silently ignoring it is how v4 accumulated settings that meant + # nothing and a list of dead keys to remember. + problems.append(f"no module named {module_id!r} is installed.") + continue + clean, said = schema_mod.check_values(spec, values) + checked[module_id] = clean + problems.extend(said) + + if problems: + raise RuntimeError("FunPack Modifier Settings:\n " + "\n ".join(problems)) + + named = ", ".join(sorted(checked)) or "nothing set" + return io.NodeOutput(checked, f"settings for {named}") + + +class FunPackLoadModifiers(io.ComfyNode): + """Whatever announced itself, filtered by this model, installed in order.""" + + @classmethod + def define_schema(cls) -> io.Schema: + return io.Schema( + node_id="FunPackLoadModifiers", + display_name="FunPack Load Modifiers", + category="FunPack/Sampling", + description="Apply the enabled modifiers to a model. Works with any sampler.", + inputs=[ + io.Model.Input("model"), + io.Custom("FUNPACK_SETTINGS").Input("settings", optional=True), + ], + outputs=[ + io.Model.Output(display_name="model"), + io.String.Output(display_name="status"), + ], + ) + + @classmethod + def execute(cls, model, settings=None) -> io.NodeOutput: + from ..._core import traits as traits_mod + from core.relations import order + + registry = registry_mod.current() + specs = list(registry.specs.values()) + + # What this model actually is, read off the model itself. + available = traits_mod.traits_of(model, specs) + + offering = [spec for spec in specs if spec.provides.get(CAPABILITY)] + compatible, incompatible = traits_mod.split(offering, available) + ordered, rejected = order(compatible) + + patched = model.clone() + applied, notes = [], [] + + for spec in ordered: + values = (settings or {}).get(spec.id) or spec.defaults() + install = spec.provides[CAPABILITY] + try: + note = install(patched, values, key=KEY_PREFIX + spec.id) + except Exception as exc: # noqa: BLE001 + # Absent, and said out loud. A modifier that half-installed and + # carried on is how a run silently stops meaning what it says. + log.failed(f"{spec.id}.{CAPABILITY}", exc) + notes.append(f"{spec.id}: failed to install -- {type(exc).__name__}: {exc}") + continue + if note is None: + continue # the module decided it is off + applied.append(spec.id) + notes.append(f"{spec.id}: {note}") + + for spec in incompatible: + notes.append(f"{spec.id}: needs {', '.join(traits_mod.missing_for(spec, available))}") + for spec, why in rejected: + notes.append(f"{spec.id}: {why}") + + 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/tests/test_modifier_reaches_sampling.py b/modules/sampling/tests/test_modifier_reaches_sampling.py new file mode 100644 index 00000000..1232d3c4 --- /dev/null +++ b/modules/sampling/tests/test_modifier_reaches_sampling.py @@ -0,0 +1,100 @@ +"""A modifier installed on the model is reached by ComfyUI's own sampling path. + +This is the architectural claim made checkable: modifiers attach to the MODEL, so +no sampler has to cooperate and therefore no sampler can own them. If this test +holds, wiring the modified model into ComfyUI's own SamplerCustomAdvanced runs +every per-step modifier. + +It uses comfy's real functions -- prepare_model_patcher, get_all_wrappers, +WrapperExecutor -- rather than asserting on our own dict. Checking that we wrote +a key we chose would prove nothing about whether anything reads it. +""" + +import pytest + + +@pytest.fixture(autouse=True) +def _needs_comfy(comfyui): + """Imports comfy.""" + + +@pytest.fixture +def registry(monkeypatch): + from core import registry as registry_mod + from core.contract import ModuleSpec + + class Fake: + specs = {} + failed = [] + + fake = Fake() + fake.specs = {} + monkeypatch.setattr(registry_mod, "current", lambda rescan=False: fake) + return fake, ModuleSpec + + +def test_the_wrapper_a_modifier_installs_is_executed_by_comfys_sampler_path(registry): + import torch + import comfy.patcher_extension as ext + import comfy.sampler_helpers + from comfy.model_patcher import ModelPatcher + from comfy.patcher_extension import WrappersMP + from modules.sampling.modifiers.nodes import FunPackLoadModifiers + + fake, ModuleSpec = registry + ran = [] + + def install(target, values, key): + def wrapper(executor, *args, **kwargs): + ran.append(key) + return executor(*args, **kwargs) + + target.add_wrapper_with_key(WrappersMP.SAMPLER_SAMPLE, key, wrapper) + return "on" + + fake.specs["m"] = ModuleSpec(id="m", title="m", mount="", + provides={"modifier": install}) + + class Stub(torch.nn.Module): + pass + + patcher = ModelPatcher(Stub(), load_device=torch.device("cpu"), + offload_device=torch.device("cpu")) + patched, _status = FunPackLoadModifiers.execute(patcher).result + + # Exactly what CFGGuider.inner_sample does: the patcher's wrappers are merged + # into model_options, then the sampler's own sample() is wrapped in them. + model_options = {"transformer_options": {}} + comfy.sampler_helpers.prepare_model_patcher(patched, {}, model_options) + + def sample(*args, **kwargs): + return "sampled" + + executor = ext.WrapperExecutor.new_class_executor( + sample, None, + ext.get_all_wrappers(WrappersMP.SAMPLER_SAMPLE, model_options, is_model_options=True)) + + assert executor.execute() == "sampled" + assert ran == ["funpack.m"], ( + "the modifier was installed somewhere ComfyUI's sampling path does not read") + + +def test_an_unmodified_model_carries_none_of_our_wrappers(registry): + """The control. Without it the test above could pass on a machine where + something else installs a wrapper under any key at all.""" + import torch + import comfy.sampler_helpers + from comfy.model_patcher import ModelPatcher + from comfy.patcher_extension import WrappersMP + + class Stub(torch.nn.Module): + pass + + patcher = ModelPatcher(Stub(), load_device=torch.device("cpu"), + offload_device=torch.device("cpu")) + model_options = {"transformer_options": {}} + comfy.sampler_helpers.prepare_model_patcher(patcher, {}, model_options) + + installed = model_options["transformer_options"].get("wrappers", {}).get( + WrappersMP.SAMPLER_SAMPLE, {}) + assert not [k for k in installed if str(k).startswith("funpack.")] diff --git a/modules/sampling/tests/test_modifier_settings.py b/modules/sampling/tests/test_modifier_settings.py new file mode 100644 index 00000000..66841f00 --- /dev/null +++ b/modules/sampling/tests/test_modifier_settings.py @@ -0,0 +1,101 @@ +"""The settings payload, checked rather than trusted. + +v4 carried these as JSON in a string widget that nothing validated. The cost was +visible in the code: a hand-mirrored table of every default, "regenerated from +engine_settings.js by a test", plus a list of keys that no longer meant anything +and had to be filtered out so they could not reach the render. Every test here +is a thing that silently did nothing in v4. +""" + +import pytest + + +@pytest.fixture(autouse=True) +def _needs_comfy(comfyui): + """Imports comfy.""" + + +@pytest.fixture +def registry(monkeypatch): + from core import registry as registry_mod + from core.contract import ModuleSpec + + class Fake: + specs = { + "alg": ModuleSpec(id="alg", title="ALG", mount="", settings={ + "enabled": {"type": "bool", "default": False, "label": "On"}, + "strength": {"type": "float", "default": 0.5, "min": 0.0, "max": 1.0, "label": "S"}, + "steps": {"type": "int", "default": 2, "min": 0, "max": 8, "label": "N"}, + "mode": {"type": "enum", "default": "beat", "label": "M", "options": [ + {"value": "beat", "label": "Beat"}, {"value": "flat", "label": "Flat"}]}, + }), + } + failed = [] + + fake = Fake() + monkeypatch.setattr(registry_mod, "current", lambda rescan=False: fake) + return fake + + +def _run(text): + from modules.sampling.modifiers.nodes import FunPackModifierSettings + return FunPackModifierSettings.execute(text).result + + +def test_a_valid_payload_comes_back_merged_over_the_defaults(registry): + values, _status = _run('{"alg": {"enabled": true}}') + assert values["alg"] == {"enabled": True, "strength": 0.5, "steps": 2, "mode": "beat"} + + +def test_an_empty_payload_is_fine(registry): + values, _status = _run("") + assert values == {} + + +def test_malformed_json_says_so_instead_of_meaning_nothing(registry): + with pytest.raises(RuntimeError, match="not valid JSON"): + _run("{not json") + + +def test_a_setting_no_module_declares_is_reported(registry): + """In v4 this was simply ignored, which is how dead keys accumulated.""" + with pytest.raises(RuntimeError, match="no setting named 'stength'"): + _run('{"alg": {"stength": 0.5}}') + + +def test_a_module_that_is_not_installed_is_reported(registry): + with pytest.raises(RuntimeError, match="no module named 'ghost'"): + _run('{"ghost": {"x": 1}}') + + +@pytest.mark.parametrize("payload,expected", [ + ('{"alg": {"strength": "loud"}}', "float"), + ('{"alg": {"strength": 2.0}}', "above its maximum"), + ('{"alg": {"strength": -1.0}}', "below its minimum"), + ('{"alg": {"steps": 1.5}}', "int"), + ('{"alg": {"enabled": "yes"}}', "bool"), + ('{"alg": {"mode": "swing"}}', "must be one of"), +]) +def test_a_wrong_value_is_refused_not_coerced(registry, payload, expected): + with pytest.raises(RuntimeError, match=expected): + _run(payload) + + +def test_a_bool_is_not_accepted_as_a_number(registry): + """True == 1 in Python, so an unguarded numeric check lets a checkbox through + as a strength.""" + with pytest.raises(RuntimeError, match="float"): + _run('{"alg": {"strength": true}}') + + +def test_a_payload_that_is_not_an_object_is_refused(registry): + with pytest.raises(RuntimeError, match="keyed by module id"): + _run("[1, 2, 3]") + + +def test_every_problem_is_reported_at_once(registry): + """One error per run means finding them one at a time.""" + with pytest.raises(RuntimeError) as raised: + _run('{"alg": {"strength": 9.0, "steps": 99}}') + message = str(raised.value) + assert "above its maximum 1.0" in message and "above its maximum 8" in message diff --git a/modules/sampling/tests/test_modifiers.py b/modules/sampling/tests/test_modifiers.py new file mode 100644 index 00000000..1c5233e4 --- /dev/null +++ b/modules/sampling/tests/test_modifiers.py @@ -0,0 +1,194 @@ +"""Modifiers: chosen, filtered, installed, and never accumulating. + +Run against real ComfyUI ModelPatcher objects. No weights are needed: a patcher +around an empty nn.Module carries the same wrapper machinery a real one does. +""" + +import pytest + + +@pytest.fixture(autouse=True) +def _needs_comfy(comfyui): + """Everything here touches comfy.""" + + +@pytest.fixture +def patcher(): + import torch + from comfy.model_patcher import ModelPatcher + + class Stub(torch.nn.Module): + pass + + return ModelPatcher(Stub(), load_device=torch.device("cpu"), + offload_device=torch.device("cpu")) + + +def _spec(module_id, install, requires=(), settings=None, stage="sampling"): + from core.contract import ModuleSpec + return ModuleSpec(id=module_id, title=module_id, mount="", stage=stage, + requires=list(requires), settings=settings or {}, + provides={"modifier": install}) + + +@pytest.fixture +def registry(monkeypatch): + """A registry holding only what a test puts in it.""" + from core import registry as registry_mod + + class Fake: + def __init__(self): + self.specs = {} + self.failed = [] + + fake = Fake() + monkeypatch.setattr(registry_mod, "current", lambda rescan=False: fake) + return fake + + +# --- installing ------------------------------------------------------------ + +def test_a_modifier_installs_on_the_model_and_is_reported(registry, patcher): + from comfy.patcher_extension import WrappersMP + from modules.sampling.modifiers.nodes import FunPackLoadModifiers + + def install(target, values, key): + target.add_wrapper_with_key(WrappersMP.SAMPLER_SAMPLE, key, lambda e, *a, **k: e(*a, **k)) + return "on" + + registry.specs["m"] = _spec("m", install) + out = FunPackLoadModifiers.execute(patcher) + model, status = out.result + + assert "m: on" in status + assert any("funpack.m" in keys for keys in model.wrappers.get(WrappersMP.SAMPLER_SAMPLE, {})) + + +def test_installing_never_touches_the_model_everyone_else_holds(registry, patcher): + """The fault this makes impossible: v4 registered hooks on shared blocks and + removed them only on a scene change, so every run stacked another set and + only a restart cleared them. Installing on a clone means the original is + untouched, so there is nothing to accumulate and nothing to clean up.""" + from comfy.patcher_extension import WrappersMP + from modules.sampling.modifiers.nodes import FunPackLoadModifiers + + def install(target, values, key): + target.add_wrapper_with_key(WrappersMP.SAMPLER_SAMPLE, key, lambda e, *a, **k: e(*a, **k)) + return "on" + + registry.specs["m"] = _spec("m", install) + + before = len(patcher.wrappers.get(WrappersMP.SAMPLER_SAMPLE, {})) + for _ in range(50): + FunPackLoadModifiers.execute(patcher) + + after = patcher.wrappers.get(WrappersMP.SAMPLER_SAMPLE, {}) + assert len(after) == before == 0, ( + f"fifty runs left {len(after)} wrapper(s) on the shared model") + + +def test_a_modifier_that_says_it_is_off_installs_nothing(registry, patcher): + from modules.sampling.modifiers.nodes import FunPackLoadModifiers + + def install(target, values, key): + return None # the module's own decision, not core's + + registry.specs["m"] = _spec("m", install) + _model, status = FunPackLoadModifiers.execute(patcher).result + assert "0 modifier(s) applied" in status + + +def test_one_broken_modifier_does_not_stop_the_others(registry, patcher): + from modules.sampling.modifiers.nodes import FunPackLoadModifiers + + def explodes(target, values, key): + raise RuntimeError("boom") + + def works(target, values, key): + return "on" + + registry.specs["a_broken"] = _spec("a_broken", explodes) + registry.specs["b_works"] = _spec("b_works", works) + + _model, status = FunPackLoadModifiers.execute(patcher).result + assert "b_works: on" in status + assert "failed to install" in status and "boom" in status + + +def test_modifiers_install_in_declared_order(registry, patcher): + from modules.sampling.modifiers.nodes import FunPackLoadModifiers + from core.contract import ModuleSpec + + seen = [] + + def make(name): + def install(target, values, key): + seen.append(name) + return "on" + return install + + # "first" declares that it runs before "second", so filesystem/alphabetical + # order must not decide this. + registry.specs["zzz_first"] = ModuleSpec( + id="zzz_first", title="", mount="", before=["aaa_second"], + provides={"modifier": make("zzz_first")}) + registry.specs["aaa_second"] = ModuleSpec( + id="aaa_second", title="", mount="", provides={"modifier": make("aaa_second")}) + + FunPackLoadModifiers.execute(patcher) + assert seen == ["zzz_first", "aaa_second"] + + +# --- filtering by what the model actually is ------------------------------- + +def test_a_modifier_needing_an_absent_trait_is_absent_and_says_why(registry, patcher): + from modules.sampling.modifiers.nodes import FunPackLoadModifiers + + def install(target, values, key): + raise AssertionError("an incompatible modifier was installed") + + registry.specs["needs_audio"] = _spec("needs_audio", install, requires=["audio_stream"]) + _model, status = FunPackLoadModifiers.execute(patcher).result + + assert "needs_audio: needs audio_stream" in status + assert "0 modifier(s) applied" in status + + +def test_a_modifier_declaring_no_traits_runs_on_anything(registry, patcher): + """Narrowing, not opting in -- the property that lets a modifier reach models + FunPack has never heard of.""" + from modules.sampling.modifiers.nodes import FunPackLoadModifiers + registry.specs["plain"] = _spec("plain", lambda t, v, key: "on") + _model, status = FunPackLoadModifiers.execute(patcher).result + assert "plain: on" in status + + +# --- the settings payload -------------------------------------------------- + +def test_values_reach_the_modifier_that_declared_them(registry, patcher): + from modules.sampling.modifiers.nodes import FunPackLoadModifiers + + got = {} + + def install(target, values, key): + got.update(values) + return "on" + + registry.specs["m"] = _spec("m", install, settings={ + "strength": {"type": "float", "default": 0.5, "min": 0.0, "max": 1.0, "label": "S"}, + }) + FunPackLoadModifiers.execute(patcher, settings={"m": {"strength": 0.8}}) + assert got == {"strength": 0.8} + + +def test_a_modifier_with_no_values_gets_its_declared_defaults(registry, patcher): + """A headless run must match a run with the panel open, because both read the + same declaration.""" + from modules.sampling.modifiers.nodes import FunPackLoadModifiers + + got = {} + registry.specs["m"] = _spec("m", lambda t, v, key: got.update(v) or "on", settings={ + "strength": {"type": "float", "default": 0.5, "min": 0.0, "max": 1.0, "label": "S"}, + }) + FunPackLoadModifiers.execute(patcher) + assert got == {"strength": 0.5}