Repository navigation
[Fix] comfyui: Fix GGUF detection, import guard and VRAM accounting in SGLDiffusion plugin - #43536
Conversation
|
hi @niehen6174, Planning to add support for a few minor features.
Wanted to get your view on this. Thank you! |
|
built these as part of #43673 |
|
Thanks for the audit, there are real fixes here. To keep it reviewable, could you narrow this PR to the integrated-mode fixes (#1, #2, #5, #8, and #12 if you want)?
|
…l nodes silently executors/minimax_h3.py imported sglang.multimodal_gen unguarded at module scope, breaking the "keep nodes importable without the runtime" pattern base.py/flux.py/zimage.py already follow. core/generator.py imports every executor unconditionally, so this took Flux/Z-Image down with it whenever the diffusion extras weren't installed. __init__.py's except branch then swallowed the resulting ImportError with no logging, so every node just vanished from ComfyUI with no indication why. Guard the import the same way base.py does, raise clearly on first use instead, and log the failure in __init__.py.
…chitecture detect _h3_detect_companion always looked for a MiniMax-H3 BF16 companion regardless of what GGUF was actually being loaded. A Flux/Qwen/Z-Image GGUF either failed with an H3-specific error, or, if an H3 BF16 file happened to exist in diffusion_models, silently built the H3 model around the wrong weights. Infer the real architecture from an explicit SGLDOptions.model_type hint or the GGUF's filename, look up a companion from a per-architecture table, and raise a clear error when the architecture can't be determined instead of defaulting to H3.
… diffusion_models registry SGLDUNETLoader enabled .gguf by mutating folder_paths.folder_names_and_paths["diffusion_models"] at import time, a global registry every other node reads too. This made ComfyUI's own stock "Load Diffusion Model" node offer GGUF files it cannot load. Build SGLDUNETLoader's own file list locally (stock get_filename_list plus a local .gguf scan of the same folders) instead of mutating shared state.
…onfusing TypeError get_comfyui_model returned a bare None on two model-config-build failure paths, but load_model always unpacks the result into comfyui_model, model_config, model_type. A user hitting either failure saw "cannot unpack non-iterable NoneType object" instead of the real error. Raise ValueError with the model path and detected type instead.
SGLDModelPatcher.model_size() returned a hardcoded guess for flux (27GB) and lumina2 (8GB) and 0 for everything else (qwen_image, minimax_h3), so ComfyUI freed VRAM for some SGLD models and not others even though the weights live in the worker process, not in this one. Compute the real byte size from the checkpoint header at load time (parameter count * dtype size) and pass it into the patcher, falling back to the old table only when a caller builds a patcher without that. For a GGUF load, size from the actual (much smaller, quantized) GGUF file rather than the BF16 companion used only for ComfyUI's architecture detect.
3dcefc9 to
a0030b7
Compare
# Conflicts: # python/sglang/multimodal_gen/apps/ComfyUI_SGLDiffusion/executors/minimax_h3.py
… merging main main added VDNH3Executor, whose class body computes its sigma grid from the runtime at import time, which broke the import guard. Only compute it when the runtime imported. Also update the new H3 generator test stub for get_comfyui_model's four-value return.
|
Thanks for narrowing it down. Bugs 2, 3 and 4 look good. Two changes:
|
…model types" This reverts commit 7f5e9f3.
Read tensor names and logical shapes with read_gguf_tensor_meta() and run ComfyUI's detection on them, as is done for safetensors. Drops the filename hints and the BF16 companion table.
|
hi @niehen6174 , made the changes. |
… loader With header-based detection the .gguf itself is the worker's model path, but is_comfyui_single_file only accepted .safetensors, so the worker skipped load_comfyui_transformer. Accept .gguf there, use model_path as the GGUF file when no transformer_weights_path is set, and read the Flux guidance tensor name from the GGUF header instead of opening it as safetensors.
|
Tested the latest head on RTX 5090 with real ComfyUI workflows. BF16 and H3 GGUF both work, but Z-Image GGUF (
The checkpoint contains Could you normalize the names consistently for both GGUF metadata lookup and weight loading, including packed QKV handling? Please also add a small regression test for this mapping and rerun Z-Image GGUF end to end, while keeping the working BF16 and H3 GGUF paths intact. |
Map GGUF tensor names through the spec's param mapping for both the quant lookup and weight loading: qkv -> to_qkv, and w1/w3 shards are concatenated into the fused w13. Packed tensors whose layer builds without GGUF (adaLN ReplicatedLinear) are dequantized on load.
Drop test_sgld_bugfixes.py and its ComfyUI stub harness; move each case into the existing unit file that already has the fixture. List GGUF DiTs with ComfyUI's recursive_search/filter_files_extensions, and rely on the GGUF reader's own error for missing files.
|
hi @niehen6174, thanks for testing. Fixed in the latest push. The GGUF loader now maps tensor names through the spec mapping for both quant lookup and weight loading. For Z-Image, ComfyUI v0.39.2, 1x L40S, 1024x1024, 8 steps, seed 42:
The GGUF image keeps the same composition; the gap is Q4 detail drift. H3 GGUF: the new mapping leaves all 532 tensor names of |
# Conflicts: # python/sglang/multimodal_gen/test/unit/test_comfyui_checkpoint_spec.py
|
/tag-and-rerun-ci |
Fixes #43874
Summary
Integrated-mode fixes for
python/sglang/multimodal_gen/apps/ComfyUI_SGLDiffusion/, narrowed per review. Bug numbers match the issue.read_gguf_tensor_meta()and runs ComfyUI's detection on them, like safetensors, so no BF16 companion file is needed. A GGUF picked inSGLDUNETLoaderis loaded as the DiT. The GGUF loader maps tensor names through the spec's param mapping for both quant lookup and weight loading: Z-Imageqkv->to_qkv,w1/w3packed shards are concatenated into the fusedw13, and packed tensors whose layer builds without GGUF (adaLNReplicatedLinear) are dequantized on load..ggufleaked into ComfyUI's globaldiffusion_modelsregistry.SGLDUNETLoadernow builds its own list with ComfyUI'srecursive_search/filter_files_extensions, so the stock loader no longer offers GGUF files.None("cannot unpack non-iterable NoneType"). It now raisesValueErrorwith the model path.executors/minimax_h3.pyguards its runtime imports likebase.py, and__init__.pylogs why registration failed.Dropped from this PR: Cache-DiT (#43696), worker sleep/wake (#44086), server-mode items (#44082), and VRAM accounting (moved to #44086).
Tests
Regression tests live in the existing unit files and reuse their fixtures (
test_comfyui_checkpoint_spec.py,test_comfyui_generator.py,test_comfyui_lora_node.py); no new test file. Targeted CPU run: 52 passed.ComfyUI v0.39.2 headless, 1x L40S, Z-Image Turbo, 1024x1024, 8 steps, seed 42:
z-image-turbo-Q4_K_M.ggufThe GGUF image keeps the same composition; the lower PSNR is Q4 drift in fine detail against a BF16 reference.
H3 GGUF: checked the real
minimax_h3_fl2va_pruned-Q4_K.ggufheader against the new name mapping; 0 of 532 tensor names change, so the H3 path loads as before.Known gap: Flux ComfyUI GGUF likely still needs its
_convert_weightsreshapes on the GGUF path; left as a follow-up.CI States
Latest PR Test (Base): ✅ Run #39834374029
Latest PR Test (Extra): ❌ Run #39834374072
Latest PR Test (AMD ROCm 10): ❌ Run #39834374024