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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
79ca8cfb6b
commit
548d99a39b
@@ -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():
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user