feat(gguf): read an unlisted architecture by default
No env var. Stepping around the pack's guard means doing its job here: the returned state dict is validated, and anything unexpected raises into the existing fallback rather than reaching ComfyUI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
4bacc9c7d3
commit
993e5e4d04
+8
-5
@@ -8,11 +8,14 @@
|
||||
minute of dequantizing a GGUF to alter a sparsity ratio. The weights depend only on the
|
||||
file (identity, size, mtime) and the dtypes; everything else is applied to a clone. One
|
||||
entry, replaced whenever the key changes.
|
||||
- **`FUNPACK_GGUF_ALLOW_ANY_ARCH=1` lets ComfyUI-GGUF read an architecture off its tested
|
||||
list** — `Unexpected architecture type in GGUF file: 'minimax_h3'` — so the file loads
|
||||
quantized instead of being expanded. OFF by default: overriding another project's guard
|
||||
turns a clean refusal into an untested path, which can fail deeper or produce a subtly
|
||||
wrong state dict. The status line names the variable when it is in effect.
|
||||
- **A GGUF architecture ComfyUI-GGUF has not been told about now loads quantized anyway.**
|
||||
The pack refuses anything off its tested list — `Unexpected architecture type in GGUF file:
|
||||
'minimax_h3'` — which cost the quantized path entirely and expanded the whole checkpoint
|
||||
instead. The name is added to its IMAGE list (never the text one) for a file whose header
|
||||
declares it. Since that steps around the pack's own guard, what the backend hands back is
|
||||
checked here instead: anything that is not a state dict of named tensors raises, and the
|
||||
existing fallback turns that into the slow-but-working path. The status line says the
|
||||
override is in effect, because a wrong generation should be traceable to it.
|
||||
- **When it does have to expand a GGUF, it expands ONCE.** The dequantized weights are
|
||||
written beside the file as an ordinary safetensors and reused on every later load, which
|
||||
then runs at safetensors speed. It costs the model's dequantized size in disk — the same
|
||||
|
||||
+27
-11
@@ -239,6 +239,13 @@ def _load(path: str, clip: bool) -> tuple[dict, dict, str]:
|
||||
if clip:
|
||||
read = getattr(loader, "gguf_clip_loader", None) or read
|
||||
sd = read(path)
|
||||
# The pack's own guard is what we just stepped around, so the check it was doing
|
||||
# has to happen here instead: an untested architecture can return something that
|
||||
# is not a state dict at all, and handing that to ComfyUI fails deep inside core
|
||||
# with a message that makes no sense out here. Anything unexpected raises, which
|
||||
# the fallback below turns into the slow-but-working path.
|
||||
if allowed:
|
||||
_assert_state_dict(sd, path)
|
||||
options = {}
|
||||
ggml_ops = getattr(ops, "GGMLOps", None)
|
||||
if ggml_ops is not None:
|
||||
@@ -314,8 +321,23 @@ def _load_native_cached(path: str) -> tuple[dict, dict, str]:
|
||||
return sd, options, note
|
||||
|
||||
|
||||
ARCH_OVERRIDE_VAR = "FUNPACK_GGUF_ALLOW_ANY_ARCH"
|
||||
_ARCH_ON = {"1", "true", "yes", "on"}
|
||||
def _assert_state_dict(sd, path: str) -> None:
|
||||
"""Raise unless `sd` is a usable state dict. Cheap, and only shape — nothing here can
|
||||
tell whether the NUMBERS are right, which is why the status line says to suspect this
|
||||
first if a generation comes out wrong."""
|
||||
if not isinstance(sd, dict) or not sd:
|
||||
raise RuntimeError(
|
||||
f"{os.path.basename(path)}: the GGUF backend returned "
|
||||
f"{type(sd).__name__} instead of a state dict")
|
||||
for key, value in sd.items():
|
||||
if not isinstance(key, str):
|
||||
raise RuntimeError(
|
||||
f"{os.path.basename(path)}: the GGUF backend returned a state dict keyed by "
|
||||
f"{type(key).__name__}, not by tensor name")
|
||||
if not hasattr(value, "shape"):
|
||||
raise RuntimeError(
|
||||
f"{os.path.basename(path)}: {key!r} came back as {type(value).__name__}, "
|
||||
f"which is not a tensor")
|
||||
|
||||
|
||||
def read_architecture(path: str) -> Optional[str]:
|
||||
@@ -346,11 +368,6 @@ def _allow_architectures(loader, path: str) -> str:
|
||||
declares it. Announced, because it is an override of another project's own guard and a
|
||||
wrong result here should be traceable to this line.
|
||||
"""
|
||||
# OPT-IN. Overriding another project's safety guard changes a clean refusal (which falls
|
||||
# back to a slower path that works) into an untested code path that may produce a subtly
|
||||
# wrong state dict — a worse failure, and a harder one to attribute. Off unless asked.
|
||||
if str(os.environ.get(ARCH_OVERRIDE_VAR, "")).strip().lower() not in _ARCH_ON:
|
||||
return ""
|
||||
arch = read_architecture(path)
|
||||
if not arch:
|
||||
return ""
|
||||
@@ -373,10 +390,9 @@ def _allow_architectures(loader, path: str) -> str:
|
||||
continue
|
||||
if not added:
|
||||
return ""
|
||||
return (f"architecture {arch!r} is not on ComfyUI-GGUF's tested list and "
|
||||
f"{ARCH_OVERRIDE_VAR} added it so the file loads quantized instead of being "
|
||||
f"expanded — if the weights come out wrong, or the load fails somewhere inside "
|
||||
f"the pack, this is why: unset it to go back to the refusal and the slower path")
|
||||
return (f"architecture {arch!r} is not on ComfyUI-GGUF's tested list and FunPack added "
|
||||
f"it, so the file loads quantized instead of being expanded — if the weights come "
|
||||
f"out wrong, this is the first thing to suspect")
|
||||
|
||||
|
||||
def _load_native(path: str) -> tuple[dict, dict, str]:
|
||||
|
||||
+20
-15
@@ -322,34 +322,21 @@ class _Loader:
|
||||
self.IMG_ARCH_LIST = set(arches)
|
||||
|
||||
|
||||
def test_the_override_is_off_unless_asked(monkeypatch):
|
||||
"""Turning another project's clean refusal into an untested code path is a WORSE failure
|
||||
than the slow fallback it avoids — it can produce a subtly wrong state dict instead."""
|
||||
monkeypatch.delenv(gguf_support.ARCH_OVERRIDE_VAR, raising=False)
|
||||
monkeypatch.setattr(gguf_support, "read_architecture", lambda p: "minimax_h3")
|
||||
loader = _Loader({"flux"})
|
||||
assert gguf_support._allow_architectures(loader, "x.gguf") == ""
|
||||
assert loader.IMG_ARCH_LIST == {"flux"}
|
||||
|
||||
|
||||
def test_an_unknown_architecture_is_added_so_the_file_loads_quantized(monkeypatch):
|
||||
monkeypatch.setenv(gguf_support.ARCH_OVERRIDE_VAR, "1")
|
||||
monkeypatch.setattr(gguf_support, "read_architecture", lambda p: "minimax_h3")
|
||||
loader = _Loader({"flux", "sd3"})
|
||||
note = gguf_support._allow_architectures(loader, "x.gguf")
|
||||
assert "minimax_h3" in loader.IMG_ARCH_LIST
|
||||
assert "minimax_h3" in note and gguf_support.ARCH_OVERRIDE_VAR in note
|
||||
assert "minimax_h3" in note and "FunPack added" in note
|
||||
|
||||
|
||||
def test_a_known_architecture_is_left_alone_and_says_nothing(monkeypatch):
|
||||
monkeypatch.setenv(gguf_support.ARCH_OVERRIDE_VAR, "1")
|
||||
monkeypatch.setattr(gguf_support, "read_architecture", lambda p: "flux")
|
||||
loader = _Loader({"flux"})
|
||||
assert gguf_support._allow_architectures(loader, "x.gguf") == ""
|
||||
|
||||
|
||||
def test_an_unreadable_architecture_changes_nothing(monkeypatch):
|
||||
monkeypatch.setenv(gguf_support.ARCH_OVERRIDE_VAR, "1")
|
||||
monkeypatch.setattr(gguf_support, "read_architecture", lambda p: None)
|
||||
loader = _Loader({"flux"})
|
||||
assert gguf_support._allow_architectures(loader, "x.gguf") == ""
|
||||
@@ -357,7 +344,6 @@ def test_an_unreadable_architecture_changes_nothing(monkeypatch):
|
||||
|
||||
|
||||
def test_the_text_encoder_list_is_never_touched(monkeypatch):
|
||||
monkeypatch.setenv(gguf_support.ARCH_OVERRIDE_VAR, "1")
|
||||
"""Image and text architectures steer different key renaming in the pack; guessing a
|
||||
text arch into the image list is one thing, the reverse is another."""
|
||||
monkeypatch.setattr(gguf_support, "read_architecture", lambda p: "minimax_h3")
|
||||
@@ -365,3 +351,22 @@ def test_the_text_encoder_list_is_never_touched(monkeypatch):
|
||||
loader.TXT_ARCH_LIST = {"t5"}
|
||||
gguf_support._allow_architectures(loader, "x.gguf")
|
||||
assert loader.TXT_ARCH_LIST == {"t5"}
|
||||
|
||||
|
||||
def test_a_backend_that_returns_something_else_is_refused(tmp_path):
|
||||
"""The pack's own guard is what we stepped around, so the check it was doing happens
|
||||
here: handing a non-state-dict to ComfyUI fails deep in core with a message that makes no
|
||||
sense out here. Raising instead drops to the slow path that works."""
|
||||
for bad in (None, [], "sd", {}, {1: object()}):
|
||||
with pytest.raises(RuntimeError):
|
||||
gguf_support._assert_state_dict(bad, "m.gguf")
|
||||
|
||||
|
||||
def test_a_non_tensor_value_is_named(tmp_path):
|
||||
with pytest.raises(RuntimeError, match="not a tensor"):
|
||||
gguf_support._assert_state_dict({"w": {"nested": 1}}, "m.gguf")
|
||||
|
||||
|
||||
def test_a_real_state_dict_passes():
|
||||
import torch
|
||||
gguf_support._assert_state_dict({"w": torch.zeros(2)}, "m.gguf")
|
||||
|
||||
Reference in New Issue
Block a user