sampling: modifiers attach to the model, so no sampler can own them

Two nodes. One carries the values a person picked, as a single validated
payload rather than a socket per setting -- a socket per setting means this
node's shape changes whenever any module gains an option, and every saved
workflow refers to sockets that moved.

The other reads what announced itself, filters it by what the model actually
is, orders it by declared relations, and installs each one through ComfyUI's
own patcher_extension wrappers. The consequence is checked against comfy's
real prepare_model_patcher and WrapperExecutor rather than against our own
dict: a model that has been through this node runs every per-step modifier
inside ComfyUI's own SamplerCustomAdvanced. The sampler is not consulted, so
it cannot own them.

Installing happens on a clone, and clone() copies the wrapper lists per key,
so the model everyone else holds is untouched. Fifty runs leave zero
wrappers behind. The accumulation that once degraded output across a hundred
generations and survived every reset short of a restart is structurally
impossible here rather than something to remember to undo.

The payload is validated against the same declaration that renders the
panel: an undeclared key, a wrong type, a value outside its own bounds and a
bool passed as a number are all refused, and all problems are reported at
once. v4 validated none of it, which is why it needed a hand-mirrored table
of every default and a list of keys that no longer meant anything.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
digital-garbage
2026-08-29 12:35:28 +03:00
co-authored by Claude Opus 5
parent 9e08f4dd0e
commit 8521bdf899
8 changed files with 623 additions and 1 deletions
+64 -1
View File
@@ -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"):
+2
View File
@@ -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",
View File
+10
View File
@@ -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]
+152
View File
@@ -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]))
@@ -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.")]
@@ -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
+194
View File
@@ -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}