fix(loaders): name the stage a load failed at; gate the arch override
A bare "'dict' object has no attribute 'startswith'" names neither the step nor the file. Each stage now labels itself in the re-raise. The ComfyUI-GGUF architecture override is opt-in, not default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
78c9625a1a
commit
4bacc9c7d3
+5
-6
@@ -8,12 +8,11 @@
|
||||
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.
|
||||
- **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 fell back to expanding the whole
|
||||
checkpoint. The name is now added to its IMAGE list (never the text one) for a file whose
|
||||
header actually declares it, and the status line says so, because it is an override of
|
||||
another project's own guard.
|
||||
- **`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.
|
||||
- **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
|
||||
|
||||
+13
-3
@@ -314,6 +314,10 @@ 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 read_architecture(path: str) -> Optional[str]:
|
||||
"""`general.architecture` out of a GGUF header, or None. Header only — no tensor data."""
|
||||
try:
|
||||
@@ -342,6 +346,11 @@ 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 ""
|
||||
@@ -364,9 +373,10 @@ 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 FunPack added it "
|
||||
f"so the file loads quantized instead of being expanded — if the weights come out "
|
||||
f"wrong, this is why")
|
||||
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")
|
||||
|
||||
|
||||
def _load_native(path: str) -> tuple[dict, dict, str]:
|
||||
|
||||
+24
@@ -255,6 +255,24 @@ class FunPackDiffusionModelLoader:
|
||||
fp16_accumulation=False, sla_sparsity=None, sla_block_size=None,
|
||||
sla_protect_audio=None, sla_min_seq_len=None, sla_dense_last_steps=None,
|
||||
sla=False):
|
||||
# ComfyUI reports a node failure as "<node>: <message>" with the traceback going to
|
||||
# the console. A bare "'dict' object has no attribute 'startswith'" names neither the
|
||||
# step it came from nor the file it was reading, which is most of what you need. Each
|
||||
# stage sets `stage`, and the re-raise says which one.
|
||||
stage = {"at": "starting"}
|
||||
try:
|
||||
return self._load_model(stage, model_name, weight_dtype, compute_dtype, attention,
|
||||
fp16_accumulation, sla_sparsity, sla_block_size,
|
||||
sla_protect_audio, sla_min_seq_len, sla_dense_last_steps,
|
||||
sla)
|
||||
except Exception as e:
|
||||
raise type(e)(f"{e} [while {stage['at']}: {model_name}]").with_traceback(
|
||||
e.__traceback__) from e
|
||||
|
||||
def _load_model(self, stage, model_name, weight_dtype, compute_dtype, attention,
|
||||
fp16_accumulation=False, sla_sparsity=None, sla_block_size=None,
|
||||
sla_protect_audio=None, sla_min_seq_len=None, sla_dense_last_steps=None,
|
||||
sla=False):
|
||||
notes = [f"FunPack Diffusion Model Loader | {model_name}"]
|
||||
|
||||
accum = set_fp16_accumulation(fp16_accumulation)
|
||||
@@ -272,6 +290,7 @@ class FunPackDiffusionModelLoader:
|
||||
# depend on the file and the dtypes, so those are the cache key and everything else
|
||||
# is applied to a clone. One entry, replaced whenever the key changes: this holds no
|
||||
# more than ComfyUI's own node-output cache already did.
|
||||
stage["at"] = "checking the weight cache"
|
||||
cache_key = _weight_cache_key(model_name, weight_dtype, compute_dtype)
|
||||
cached = _WEIGHT_CACHE.get("model") if _WEIGHT_CACHE.get("key") == cache_key else None
|
||||
if cached is not None:
|
||||
@@ -280,6 +299,7 @@ class FunPackDiffusionModelLoader:
|
||||
return self._finish(model, notes, attention, sla, sla_sparsity, sla_block_size,
|
||||
sla_min_seq_len, sla_dense_last_steps, sla_protect_audio)
|
||||
|
||||
stage["at"] = "locating the file"
|
||||
path = None
|
||||
misnamed = False
|
||||
if gguf_support.is_gguf(model_name):
|
||||
@@ -296,6 +316,7 @@ class FunPackDiffusionModelLoader:
|
||||
if misnamed:
|
||||
notes.append(f"{model_name} is named .safetensors but is a GGUF container — "
|
||||
f"loaded as GGUF")
|
||||
stage["at"] = "reading the GGUF container"
|
||||
state_dict, gguf_options, gguf_note = gguf_support.load_state_dict(path)
|
||||
# The quantized path needs its own torch operations, and they must not be lost to
|
||||
# the dtype options merged above — a GGUF loaded with stock ops would try to matmul
|
||||
@@ -304,7 +325,9 @@ class FunPackDiffusionModelLoader:
|
||||
metadata = None
|
||||
notes.append(gguf_note)
|
||||
else:
|
||||
stage["at"] = "reading the safetensors file"
|
||||
state_dict, metadata = comfy.utils.load_torch_file(path, return_metadata=True)
|
||||
stage["at"] = "handing the state dict to ComfyUI"
|
||||
model = comfy.sd.load_diffusion_model_state_dict(
|
||||
state_dict, model_options=model_options, metadata=metadata)
|
||||
if model is None:
|
||||
@@ -324,6 +347,7 @@ class FunPackDiffusionModelLoader:
|
||||
# below is applied to a clone, which is what lets a sparsity change skip the disk.
|
||||
_WEIGHT_CACHE["key"] = cache_key
|
||||
_WEIGHT_CACHE["model"] = model
|
||||
stage["at"] = "applying attention / SLA"
|
||||
return self._finish(model.clone(), notes, attention, sla, sla_sparsity,
|
||||
sla_block_size, sla_min_seq_len, sla_dense_last_steps,
|
||||
sla_protect_audio)
|
||||
|
||||
@@ -322,21 +322,34 @@ 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 "FunPack added it" in note
|
||||
assert "minimax_h3" in note and gguf_support.ARCH_OVERRIDE_VAR 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") == ""
|
||||
@@ -344,6 +357,7 @@ 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")
|
||||
|
||||
Reference in New Issue
Block a user