From 548d99a39b8b9566433ead809ee618ca3e14e6e6 Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:20:51 +0300 Subject: [PATCH] fix: SLA's H3 gate now matches the qualified class, not a bare name Round 1 of extensive_testing on 79ca8cf found is_h3_model() matched on diffusion_model's bare class name ("MiniMaxH3Model"), inherited unchanged from v4. This codebase's own core/traits.py.has_block() documents exactly why that's unsafe for a name this generic: a same-named class from an unrelated custom node or a future FunPack model would be silently treated as H3 and sparsified at 0.90 with H3-specific audio-prefix pinning -- the identical LTX-collision risk this file's own docstring already warns about, just not guarded against strongly enough. modules/models/minimax_h3 already qualifies its own identical check (MODEL_CLASS = the dotted path); this port carried over the private, weaker duplicate instead of reusing that pattern. Switched to core/traits.py's has_block() with the same qualified dotted name, kept as a local string (not an import of modules/models/minimax_h3 itself, which pulls in comfy_extras at import time -- this file deliberately stays importable without ComfyUI on the path). Added a test exercising the exact collision the bare match would have missed: a class named MiniMaxH3Model from an unrelated module. Co-Authored-By: Claude Sonnet 5 --- modules/loaders/sla_attention.py | 22 +++++++++++++++++---- modules/loaders/tests/test_sla_attention.py | 20 +++++++++++++++++-- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/modules/loaders/sla_attention.py b/modules/loaders/sla_attention.py index 143b863d..2f2eaf49 100644 --- a/modules/loaders/sla_attention.py +++ b/modules/loaders/sla_attention.py @@ -24,10 +24,22 @@ below exists and why a run that never sparsified says so in the log. """ import logging +from .._core import traits as _traits + SLA_NAME = "sla_h3" _H3_HEAD_DIM = 128 -_H3_MODEL_NAMES = ("MiniMaxH3Model",) +# Qualified on purpose, per core/traits.py's has_block: "a bare name matches on +# the class name alone... actively wrong" for a false positive that silently +# sparsifies a model this was never validated on -- exactly the LTX-collision +# risk this file's own docstring already warns about, just not guarded against +# strongly enough before this fix (v4 matched the bare name; found in extensive_ +# testing round 1 on this port). Kept as a local string matching modules/models/ +# minimax_h3's own MODEL_CLASS rather than importing that module -- it pulls in +# comfy_extras at import time, which this file deliberately does not require +# just to be importable on a machine with no ComfyUI on the path. Keep the two +# in sync if the H3 model class is ever renamed. +_H3_MODEL_CLASS = "comfy.ldm.minimax.model.MiniMaxH3Model" # Validated on an RTX 5090 at 768p/15s (ComfyUI-H3-SLA-Attention's own measurements): # 3.7x the attention throughput of stock ComfyUI at sparsity 0.90 / block 64. @@ -67,10 +79,12 @@ def is_h3_model(model): SLA is only sound on H3: the sparsity ratio it runs at is the one the turbo LoRA was distilled to tolerate, and H3's packed [text | cond | audio | video] sequence is what the prefix pinning is for. Head shape alone would also match LTX, and - silently sparsifying LTX attention is a quality loss with no LoRA compensating it. + silently sparsifying LTX attention is a quality loss with no LoRA compensating it + -- which is also why the class name is matched qualified (module + name), not + bare: a bare name is exactly as blind to an unrelated same-named class as it is + to LTX's different head shape. """ - diffusion = getattr(getattr(model, "model", None), "diffusion_model", None) - return type(diffusion).__name__ in _H3_MODEL_NAMES + return _traits.has_block(model, _H3_MODEL_CLASS) def new_state(): diff --git a/modules/loaders/tests/test_sla_attention.py b/modules/loaders/tests/test_sla_attention.py index 22aa6e97..39f5bbfa 100644 --- a/modules/loaders/tests/test_sla_attention.py +++ b/modules/loaders/tests/test_sla_attention.py @@ -190,8 +190,12 @@ def test_a_non_h3_model_is_not_handed_a_minimax_kwarg(): # ── the H3 gate ─────────────────────────────────────────────────────────────── class _Patcher: - def __init__(self, class_name): - inner = type(class_name, (), {})() + def __init__(self, class_name, module="comfy.ldm.minimax.model"): + # has_block (core/traits.py) matches a QUALIFIED class name -- module and + # name both -- so the stub needs a real __module__ and a modules() walk + # the way an nn.Module provides, not just a bare type() with a name. + cls = type(class_name, (), {"__module__": module, "modules": lambda self: [self]}) + inner = cls() self.model = types.SimpleNamespace(diffusion_model=inner) self.model_options = {} self.wrappers = [] @@ -214,6 +218,18 @@ def test_sla_refuses_a_model_that_is_not_h3_and_says_so(): assert "not a MiniMax H3 model" in note +def test_a_same_named_class_from_a_different_module_is_not_treated_as_h3(): + """A bare class-name match would treat any unrelated MiniMaxH3Model -- built by a + different custom node, or a future FunPack model reusing the name by accident -- + as the real thing, and sparsify it at H3's ratio with H3's prefix-pinning logic + on a model that was never validated for either. The qualified check requires the + DEFINING module too, not just the name.""" + model = _Patcher("MiniMaxH3Model", module="some_other_package.model") + out, note, installed = sla.install_sla(model) + assert out is model and installed is False + assert "not a MiniMax H3 model" in note + + def test_sla_reports_when_the_machine_cannot_run_it(): model = _Patcher("MiniMaxH3Model") out, note, installed = sla.install_sla(model)