From 993e5e4d04157bdb922ad431fa263dafc3757750 Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Sun, 23 Aug 2026 05:26:32 +0300 Subject: [PATCH] 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 --- CHANGELOG.md | 13 ++++++++----- gguf_support.py | 38 +++++++++++++++++++++++++++----------- tests/test_gguf_support.py | 35 ++++++++++++++++++++--------------- 3 files changed, 55 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88ee67da..048a1910 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/gguf_support.py b/gguf_support.py index 860bf434..9260633b 100644 --- a/gguf_support.py +++ b/gguf_support.py @@ -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]: diff --git a/tests/test_gguf_support.py b/tests/test_gguf_support.py index 82eec412..62a83e52 100644 --- a/tests/test_gguf_support.py +++ b/tests/test_gguf_support.py @@ -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")