From 7a226830b161fca496effe95b14ec05d2b32950d Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:14:46 +0300 Subject: [PATCH] feat: GGUF loader support, and fix silent pipeline-editor state loss GGUF: port v4's gguf_support.py (backend selection, content-based .gguf detection, dequantize fallback) and wire it into both the diffusion-model and CLIP loaders. Pipeline editor: reopening the Models-and-pipeline settings window was silently discarding the whole edited pipeline (a loaded preset, every loader file, every sampler setting) because GET /api/pipeline is deliberately stateless server-side and boot.js never fed its own already-saved slots back into a freshly remounted editor -- found live while walking through the MiniMax-H3 setup flow. Fixed by re-validating the saved slots through check() on reopen instead of always replaying the raw stateless default. Also fixes, found via the extensive_testing adversarial loop over both of the above: a stale file-listing cache in the pipeline window (no way to see a newly-added model file without a full page reload), and a chain of races/leaks in its own refresh/commit error-banner handling that surfaced across five review rounds -- a rejection from a superseded request overwriting a fresher success, a failure banner never clearing on success, banners bleeding across groups on navigation, and a commit()'s refusal landing on the wrong view after the user backs out before it answers. Co-Authored-By: Claude Sonnet 5 --- app/boot.js | 13 +- app/shell/pipeline_window.js | 138 +++++++- app/shell/tests/pipeline_window.test.js | 264 +++++++++++++++ modules/loaders/clip/nodes.py | 66 +++- modules/loaders/diffusion_model/nodes.py | 42 ++- modules/loaders/gguf_support.py | 309 ++++++++++++++++++ modules/loaders/tests/test_gguf_support.py | 248 ++++++++++++++ .../loaders/tests/test_loader_behaviour.py | 129 ++++++++ requirements.txt | 7 + 9 files changed, 1191 insertions(+), 25 deletions(-) create mode 100644 modules/loaders/gguf_support.py create mode 100644 modules/loaders/tests/test_gguf_support.py create mode 100644 requirements.txt diff --git a/app/boot.js b/app/boot.js index 96ef6d83..ed0fa9fd 100644 --- a/app/boot.js +++ b/app/boot.js @@ -254,7 +254,18 @@ async function start() { id: "pipeline", title: "Models and pipeline", subtitle: "What the run is made of.", keywords: "pipeline models nodes slots loaders", icon: "▦", tone: "accent", mount: (ctx) => mountPipeline({ - load, describe, check, search, presets, ...ctx, + // load() always answers with the server's stateless default -- it + // holds no session, by design (core/routes.py's _pipeline()). The + // window itself only remembers what was saved for as long as it + // stays open; closing Settings and reopening this section made a + // fresh one every time, so a saved preset and every edit on top of + // it vanished the moment someone closed Settings to look at + // something else, with no error and no way to tell it had happened. + // Re-validating the already-saved `slots` through check() (rather + // than replaying `load()`'s raw answer) keeps `incomplete` and + // `queueable` correct, exactly as a fresh commit() would. + load: () => (slots ? check({ slots }) : load()), + describe, check, search, presets, ...ctx, onApply: (next) => { slots = next; // The boxes on the main window are for inputs of THESE slots. A slot diff --git a/app/shell/pipeline_window.js b/app/shell/pipeline_window.js index 78bb9b57..1ab902b9 100644 --- a/app/shell/pipeline_window.js +++ b/app/shell/pipeline_window.js @@ -92,6 +92,24 @@ export function mount({ load, describe, check, search, onApply, presets, let editing = null; // group name let selected = null; // slot id let notes = { refused: [], incomplete: [] }; + // Kept separate from `notes.refused`: that field is about an EDIT the + // server rejected (commit()), and stays on screen until the user does + // something about it. A rescan() failure is unrelated to any edit -- and + // sharing one field meant a rescan that happened to succeed a moment later + // could silently wipe out a real, still-unaddressed commit() refusal the + // user had not yet seen or acted on. + let scanFailed = null; + let learnGen = 0; // guards against two learn() calls landing out of order + // Bumped by leave() only: every enter() is reached through the index, + // which only ever renders after a leave(), so leave() alone sees every + // group-to-group transition. Which VIEW an in-flight commit() belongs to + // -- a commit started in one group can still answer after the user has + // backed out to the index. Its DATA (slots) must still apply either way, + // an edit that happened does not un-happen because someone stopped + // looking, but its refusal banner belongs to a group that is no longer on + // screen, and painting it on the index is the same misattribution + // rescanGen/scanFailed already exist to prevent for a scan failure. + let viewGen = 0; const body = composer.region.stack({ gap: "md", label: "Pipeline", fill: true }); @@ -100,19 +118,38 @@ export function mount({ load, describe, check, search, onApply, presets, async function refresh() { const answer = await load(); slots = answer.slots; - notes = { refused: [], incomplete: answer.incomplete || [] }; + // `load` can be backed by a plain read (always `refused: []`) or, when a + // previously-saved pipeline is being re-validated on open, by the same + // check() a structural edit uses -- which can refuse. Reading it here + // rather than hardcoding `[]` is what keeps that case from replacing the + // screen with an unexplained empty pipeline. + notes = { refused: answer.refused || [], incomplete: answer.incomplete || [] }; await learn(slots.map((slot) => slot.node)); } - /** Descriptions for classes we have not seen. Asked once per class. */ - async function learn(classes) { - const unknown = [...new Set(classes)].filter((c) => c && !nodes.has(c)); - if (!unknown.length) return; - const described = await describe(unknown); + /** + * Descriptions for classes we have not seen -- or, with `force`, for + * classes we have. A class's combo choices are a file listing (model + * names, LoRAs), and this window's `nodes` cache lives for as long as the + * window is open: without forcing a re-ask on entry, a file added to disk + * mid-session stayed invisible in an already-opened group until the whole + * page reloaded, which throws the in-progress edit away with it. + */ + async function learn(classes, { force = false } = {}) { + const wanted = [...new Set(classes)].filter(Boolean); + const ask = force ? wanted : wanted.filter((c) => !nodes.has(c)); + if (!ask.length) return; + const mine = ++learnGen; + const described = await describe(ask); + // A newer learn() (another force-refresh, or the group being left and + // re-entered) answered first. Applying this older one now would put its + // stale choices back over the fresh ones just drawn -- the exact bug this + // whole mechanism exists to fix, reintroduced by its own race. + if (mine !== learnGen) return; // Every class asked about gets an entry, `null` included: without it an // absent node is asked for again on every redraw, and reads on screen as a // node still being looked up rather than one that is not installed. - for (const name of unknown) nodes.set(name, described[name] ?? null); + for (const name of ask) nodes.set(name, described[name] ?? null); } // Which edit is the current one. Two structural edits can be in flight at @@ -126,12 +163,13 @@ export function mount({ load, describe, check, search, onApply, presets, /** Send the pipeline as it now stands and keep what the server says of it. */ async function commit(next, action = {}) { const mine = ++edits; + const myView = viewGen; let answer; try { answer = await check({ slots: next, ...action }); } catch (err) { if (mine !== edits) return false; - notes = { refused: [err.message], incomplete: notes.incomplete }; + if (myView === viewGen) notes = { refused: [err.message], incomplete: notes.incomplete }; draw(); return false; } @@ -140,15 +178,25 @@ export function mount({ load, describe, check, search, onApply, presets, if (mine !== edits) return false; // A refused edit did not happen, so what is on screen must stay what it // was: showing the refusal beside the change it refused is how someone - // comes to believe an edit landed when it did not. + // comes to believe an edit landed when it did not. But the refusal is + // only shown if the user is still looking at the view that made the + // edit -- otherwise it paints a group's own rejection onto whatever the + // user has since navigated to, the same misattribution scanFailed exists + // to prevent for a failed rescan. if (answer.refused.length) { - notes = { refused: answer.refused, incomplete: notes.incomplete }; + // `incomplete` describes the WHOLE pipeline, not this one edit, so it + // stays live regardless of view -- only `refused`, which is about + // THIS edit specifically, is view-scoped. + notes = { refused: myView === viewGen ? answer.refused : notes.refused, incomplete: notes.incomplete }; draw(); return false; } + // The edit happened server-side regardless of where the user is looking + // now, so the data applies unconditionally -- only the refusal notes are + // view-scoped, same reasoning as the branch above. forgetDraftsFor(slots, answer.slots); slots = answer.slots; - notes = { refused: [], incomplete: answer.incomplete }; + notes = { refused: myView === viewGen ? [] : notes.refused, incomplete: answer.incomplete }; await learn(slots.map((slot) => slot.node)); if (onApply) onApply(slots); draw(); @@ -225,6 +273,7 @@ export function mount({ load, describe, check, search, onApply, presets, function messages({ everything = false } = {}) { return [ ...notes.refused.map((text) => composer.banner.danger({ text })), + ...(scanFailed ? [composer.banner.danger({ text: scanFailed })] : []), // Not an error: a fresh install has no model picked, and colouring that // red says something went wrong when nothing has yet. ...(everything ? notes.incomplete.map((text) => composer.banner.warn({ text })) : []), @@ -315,17 +364,72 @@ export function mount({ load, describe, check, search, onApply, presets, // ---- one group: nodes on the left, the selected node's parameters right - function enter(name) { + let rescanGen = 0; + + /** + * Force-refresh a group's node descriptions and report if the server + * could not be reached -- describe() is a real fetch and a network hiccup + * or a restarted server leaves it rejecting, same as every other server + * call this window makes. Silently doing nothing here would make a refresh + * failure look identical to "nothing new to show", which defeats the point. + * + * `learnGen` inside learn() only guards which answer gets WRITTEN to the + * `nodes` cache; it says nothing about which rescan() call is allowed to + * touch `scanFailed`. Without its own token, an older rescan's late + * rejection could still slap a stale error banner over a newer rescan's + * fresh, successful result. + */ + async function rescan(classes) { + const mine = ++rescanGen; + try { + await learn(classes, { force: true }); + if (mine !== rescanGen) return; + scanFailed = null; + } catch (err) { + if (mine !== rescanGen) return; + scanFailed = err.message; + } + draw(); + } + + async function enter(name) { editing = name; draft = new Map(); - selected = (groupsOf(slots, extraGroups).byGroup.get(name) || [])[0]?.id ?? null; + const mine = groupsOf(slots, extraGroups).byGroup.get(name) || []; + selected = mine[0]?.id ?? null; + // A scan failure belongs to the group whose rescan raised it. Carrying + // it into a different group -- or the index, via leave() -- points + // someone at the wrong place to investigate; this group's own rescan() + // below sets its own, current answer moments later. + scanFailed = null; + // Same reasoning as rescanGen above, for an in-flight commit() started + // in whatever group was open before this one: its refusal belongs there, + // not here. + viewGen += 1; draw(); + // Refetched fresh rather than served from `nodes`: a combo's choices are + // a file listing, and a file added to disk since this window opened must + // show up without forcing a full page reload, which would drop the whole + // in-progress edit along with the stale cache. + await rescan(mine.map((slot) => slot.node)); } function leave() { editing = null; draft = null; selected = null; + scanFailed = null; + // Invalidates any rescan() still in flight for the group being left -- + // without this, its answer lands after leave() (with editing now null) + // and a LATE failure writes scanFailed straight onto the index, since + // rescan()'s own guard only checks whether a NEWER rescan() has started, + // never whether the one it belongs to has since been left. + rescanGen += 1; + // Same for an in-flight commit(): its DATA still applies when it lands + // (an edit that happened does not un-happen because someone stopped + // looking), but its refusal banner belongs to the group just left, not + // to the index this draw() is about to show. + viewGen += 1; draw(); } @@ -347,6 +451,14 @@ export function mount({ load, describe, check, search, onApply, presets, label: "Remove", tone: "ghost", disabled: !selected, onClick: () => remove(selected), }), + composer.button.sm({ + label: "Refresh models", tone: "ghost", + // Same file listing a re-entry already forces -- exposed here too, + // because a file dropped in AFTER this group was opened has no + // other way to appear short of reloading the page, which would + // throw away every edit made in this session. + onClick: () => rescan(mine.map((slot) => slot.node)), + }), ] }), ] }); diff --git a/app/shell/tests/pipeline_window.test.js b/app/shell/tests/pipeline_window.test.js index 70355a1a..c1f94b22 100644 --- a/app/shell/tests/pipeline_window.test.js +++ b/app/shell/tests/pipeline_window.test.js @@ -385,6 +385,270 @@ test("a node is described once, however many slots point at it", async () => { win.close(); }); +test("reopening a group re-asks for its node descriptions, so a file added mid-session shows up", async () => { + // The bug this pins: `nodes` used to cache a class's description for the + // life of the window, keyed only by class name. A model file dropped onto + // disk after the window opened was invisible in an already-known node's + // panel with no way to see it short of reloading the page -- which throws + // the whole in-progress pipeline edit away with the stale cache. + let files = ["a.safetensors"]; + const custom = server({ slots: [{ id: "l", group: "Loaders", node: "Loader", inputs: {} }] }); + custom.describe = async (classes) => { + custom.calls.push({ describe: [...classes] }); + return { Loader: { ...DESCRIPTIONS.Loader, widgets: [ + { name: "model_name", type: "COMBO", choices: [...files] }, + ] } }; + }; + const win = openWindow(custom); + await win.ready; + await win.enter("Loaders"); + const optionsOf = () => [...rowFor(win, "Model name").querySelector("select").options] + .map((o) => o.textContent); + assert.deepEqual(optionsOf().filter((t) => t !== "— not set —"), ["a.safetensors"]); + + files.push("b.safetensors"); + win.leave(); + await win.enter("Loaders"); + assert.deepEqual(optionsOf().filter((t) => t !== "— not set —"), + ["a.safetensors", "b.safetensors"], + "reopening the group did not pick up the file added since it was last open"); + win.close(); +}); + +test("Refresh models re-asks without leaving the group", async () => { + let files = ["a.safetensors"]; + const custom = server({ slots: [{ id: "l", group: "Loaders", node: "Loader", inputs: {} }] }); + custom.describe = async (classes) => { + custom.calls.push({ describe: [...classes] }); + return { Loader: { ...DESCRIPTIONS.Loader, widgets: [ + { name: "model_name", type: "COMBO", choices: [...files] }, + ] } }; + }; + const win = openWindow(custom); + await win.ready; + await win.enter("Loaders"); + + files.push("b.safetensors"); + click(button(win, "Refresh models")); + await new Promise(setImmediate); + + const optionsOf = [...rowFor(win, "Model name").querySelector("select").options] + .map((o) => o.textContent); + assert.deepEqual(optionsOf.filter((t) => t !== "— not set —"), ["a.safetensors", "b.safetensors"]); + win.close(); +}); + +test("a failed Refresh models says so, instead of looking like there was nothing new", async () => { + const custom = server({ slots: [{ id: "l", group: "Loaders", node: "Loader", inputs: {} }] }); + // mount() destructures `describe` once, at open time -- reassigning + // `custom.describe` afterward would not reach it. A stable wrapper that + // reads a mutable flag is what lets a later part of the test change what + // happens on the NEXT call. + let fail = false; + const base = custom.describe; + custom.describe = (classes) => { + if (fail) return Promise.reject(new Error("those nodes could not be described (500)")); + return base(classes); + }; + const win = openWindow(custom); + await win.ready; + await win.enter("Loaders"); + + fail = true; + click(button(win, "Refresh models")); + await new Promise(setImmediate); + + assert.match(bannerTexts(win).join(" "), /could not be described/); + win.close(); +}); + +test("an older learn() answering after a newer one does not put its stale choices back", async () => { + // Two overlapping force-refreshes (e.g. two quick clicks of Refresh models, + // or leave+enter fast enough that the first request is still in flight): + // whichever answer comes back LAST must not be allowed to be the OLDER one. + const custom = server({ slots: [{ id: "l", group: "Loaders", node: "Loader", inputs: {} }] }); + let deferred = false; + let resolvers = []; + const base = custom.describe; + custom.describe = (classes) => { + if (!deferred) return base(classes); + custom.calls.push({ describe: [...classes] }); + return new Promise((resolve) => resolvers.push({ resolve, classes })); + }; + const win = openWindow(custom); + await win.ready; // past the initial, non-force learn() before describe() defers + + deferred = true; + const first = win.enter("Loaders"); // in flight + const second = win.enter("Loaders"); // also in flight, started after + // Answer the newer request first, then the older one. + resolvers[1].resolve({ Loader: { ...DESCRIPTIONS.Loader, widgets: [ + { name: "model_name", type: "COMBO", choices: ["fresh.safetensors"] }, + ] } }); + await second; + resolvers[0].resolve({ Loader: { ...DESCRIPTIONS.Loader, widgets: [ + { name: "model_name", type: "COMBO", choices: ["stale.safetensors"] }, + ] } }); + await first; + + const options = [...rowFor(win, "Model name").querySelector("select").options] + .map((o) => o.textContent); + assert.ok(options.includes("fresh.safetensors"), "the newer answer should have won"); + assert.ok(!options.includes("stale.safetensors"), "the older, late-arriving answer overwrote it"); + win.close(); +}); + +test("a superseded rescan's late failure does not overwrite a newer rescan's success with a false error", async () => { + const custom = server({ slots: [{ id: "l", group: "Loaders", node: "Loader", inputs: {} }] }); + let deferred = false; + let handlers = []; + const base = custom.describe; + custom.describe = (classes) => { + if (!deferred) return base(classes); + return new Promise((resolve, reject) => handlers.push({ resolve, reject })); + }; + const win = openWindow(custom); + await win.ready; + + deferred = true; + const first = win.enter("Loaders"); // will fail, but only after the second has already won + const second = win.enter("Loaders"); // succeeds + handlers[1].resolve({ Loader: DESCRIPTIONS.Loader }); + await second; + handlers[0].reject(new Error("stale network failure from the OLD request")); + await first; + + assert.deepEqual(bannerTexts(win), [], + "a rejection from a superseded request must not overwrite the newer success with an error banner"); + win.close(); +}); + +test("a successful rescan clears a refusal banner an earlier failed one left behind", async () => { + const custom = server({ slots: [{ id: "l", group: "Loaders", node: "Loader", inputs: {} }] }); + let fail = false; + const base = custom.describe; + custom.describe = (classes) => { + if (fail) return Promise.reject(new Error("those nodes could not be described (500)")); + return base(classes); + }; + const win = openWindow(custom); + await win.ready; + await win.enter("Loaders"); + + fail = true; + click(button(win, "Refresh models")); + await new Promise(setImmediate); + assert.match(bannerTexts(win).join(" "), /could not be described/); + + fail = false; + click(button(win, "Refresh models")); + await new Promise(setImmediate); + assert.deepEqual(bannerTexts(win), [], "the earlier failure's banner should be gone after a success"); + win.close(); +}); + +test("a scan failure does not follow the user to the index or into a different group", async () => { + const custom = server({ slots: [ + { id: "l", group: "Loaders", node: "Loader", inputs: {} }, + { id: "s", group: "Sampling", node: "Sampler", + inputs: { model: ["l", 0], steps: 20, sampler_name: "euler" } }, + ] }); + let fail = false; + const base = custom.describe; + custom.describe = (classes) => { + if (fail) return Promise.reject(new Error("could not be described (500)")); + return base(classes); + }; + const win = openWindow(custom); + await win.ready; + + fail = true; + await win.enter("Loaders"); + assert.match(bannerTexts(win).join(" "), /could not be described/, "sanity: the failure shows in Loaders"); + + win.leave(); + assert.deepEqual(bannerTexts(win), [], "Loaders' scan failure bled into the index"); + + fail = false; + await win.enter("Sampling"); + assert.deepEqual(bannerTexts(win), [], "Loaders' scan failure bled into an unrelated group"); + win.close(); +}); + +test("leaving a group before its rescan answers does not let a late failure land on the index", async () => { + const custom = server({ slots: [{ id: "l", group: "Loaders", node: "Loader", inputs: {} }] }); + let deferred = false; + let handlers = []; + const base = custom.describe; + custom.describe = (classes) => { + if (!deferred) return base(classes); + return new Promise((resolve, reject) => handlers.push({ resolve, reject })); + }; + const win = openWindow(custom); + await win.ready; + + deferred = true; + const entering = win.enter("Loaders"); // rescan() now in flight, unresolved + win.leave(); // back out before it answers + assert.deepEqual(bannerTexts(win), [], "sanity: nothing shown right after leaving"); + + handlers[0].reject(new Error("stale network failure from the left group")); + await entering; + + assert.deepEqual(bannerTexts(win), [], + "a rescan answering after its group was left must not paint the index with its failure"); + win.close(); +}); + +test("leaving a group before its commit() answers does not let a late refusal land on the index", async () => { + // The commit()-side counterpart of the rescan race above: an edit (Remove, + // Save, wire...) started in one group can still answer after the user has + // backed out to the index. Its DATA must still apply -- an edit that + // happened does not un-happen -- but its refusal banner belongs to the + // group that made it, not to whatever the user is looking at by the time + // the answer arrives. + const custom = server({ refuse: "removing 'l' is ambiguous", slots: [ + { id: "l", group: "Loaders", node: "Loader", inputs: {} }, + ] }); + let deferred = false; + let handlers = []; + const base = custom.check; + custom.check = (body) => { + if (!deferred) return base(body); + return new Promise((resolve) => handlers.push({ resolve, body })); + }; + const win = openWindow(custom); + await win.ready; + await win.enter("Loaders"); + + deferred = true; + click(button(win, "Remove")); // commit() now in flight, unresolved + win.leave(); // back out before it answers + assert.deepEqual(bannerTexts(win), [], "sanity: nothing shown right after leaving"); + + handlers[0].resolve({ slots: [], refused: ["removing 'l' is ambiguous"], incomplete: [], queueable: false }); + await new Promise(setImmediate); + + assert.deepEqual(bannerTexts(win), [], + "a commit() refusal answering after its group was left must not paint the index with it"); + win.close(); +}); + +test("a load() that comes back refused still shows why, not an empty pipeline", async () => { + // refresh()'s `load` can be backed by a plain stateless read (always + // refused: []) or, when re-validating an already-saved pipeline on open, + // by the same check() a structural edit uses -- which CAN refuse (a real + // network failure/500 from the server mid-request). Silently dropping + // that field would replace the screen with an unexplained empty pipeline. + const win = openWindow({ + load: async () => ({ slots: [], refused: ["the pipeline could not be read (500)"], incomplete: [] }), + describe: async () => ({}), check: async () => ({}), search: async () => ({ nodes: [], total: 0 }), + }); + await win.ready; + assert.match(bannerTexts(win).join(" "), /could not be read/); + win.close(); +}); + test("a pipeline that cannot be read says so instead of showing an empty window", async () => { const win = openWindow({ load: async () => { throw new Error("the pipeline could not be read (500)"); }, diff --git a/modules/loaders/clip/nodes.py b/modules/loaders/clip/nodes.py index 4e1efc91..b9c6d68c 100644 --- a/modules/loaders/clip/nodes.py +++ b/modules/loaders/clip/nodes.py @@ -24,11 +24,13 @@ backwards-compatible with every saved graph. """ import comfy.sd +import comfy.utils import folder_paths import torch from comfy_api.latest import io from ..._core import log +from .. import gguf_support MAX_ENCODERS = 4 @@ -41,7 +43,10 @@ def clip_types(): def encoder_files(): - return folder_paths.get_filename_list("text_encoders") + """Text encoder files, `.gguf` included. Same reasoning as the diffusion + model loader's own file-list helper.""" + return list(folder_paths.get_filename_list("text_encoders")) + \ + gguf_support.gguf_names("text_encoders") class FunPackCLIPLoader(io.ComfyNode): @@ -107,13 +112,57 @@ class FunPackCLIPLoader(io.ComfyNode): if device == "cpu": model_options["load_device"] = model_options["offload_device"] = torch.device("cpu") - paths = [folder_paths.get_full_path_or_raise("text_encoders", name) for name in names] - clip = comfy.sd.load_clip( - ckpt_paths=paths, - embedding_directory=folder_paths.get_folder_paths("embeddings"), - clip_type=clip_type, - model_options=model_options, - ) + # Each name resolved to a path exactly once, whatever happens next -- + # a second lookup per file would double every filesystem stat this + # loader makes. GGUF-ness is then read off that same path: by + # extension, or by container magic for a renamed file. + paths = {} + for n in names: + if gguf_support.is_gguf(n): + p = gguf_support.gguf_path("text_encoders", n) + if not p: + # Listed when the combo was built, gone by execute time -- + # reachable now that the pipeline window can sit open for a + # while and re-list files without reloading. Refuse by name + # here, or the loader below gets None and fails deep inside + # gguf/pack machinery with no mention of which file it was. + raise RuntimeError(f"{n} is no longer where it was listed from.") + paths[n] = p + else: + paths[n] = folder_paths.get_full_path_or_raise("text_encoders", n) + misnamed = {n for n, p in paths.items() + if not gguf_support.is_gguf(n) and gguf_support.has_gguf_magic(p)} + + gguf_notes = [] + if misnamed or any(gguf_support.is_gguf(n) for n in names): + # A .gguf encoder cannot go through load_clip(), which reads files + # itself. Every slot becomes a state dict instead, so a GGUF and a + # .safetensors connector can sit in the same list -- the normal + # LTX-2.3 shape. + state_dicts = [] + for n in names: + if gguf_support.is_gguf(n) or n in misnamed: + if n in misnamed: + gguf_notes.append(f"{n} is named .safetensors but is a GGUF container") + sd, gopts, gnote = gguf_support.load_clip_state_dict(paths[n]) + state_dicts.append(sd) + model_options = {**model_options, **gopts} + gguf_notes.append(f"{n}: {gnote}") + else: + state_dicts.append(comfy.utils.load_torch_file(paths[n])) + clip = comfy.sd.load_text_encoder_state_dicts( + state_dicts, + embedding_directory=folder_paths.get_folder_paths("embeddings"), + clip_type=clip_type, + model_options=model_options, + ) + else: + clip = comfy.sd.load_clip( + ckpt_paths=[paths[n] for n in names], + embedding_directory=folder_paths.get_folder_paths("embeddings"), + clip_type=clip_type, + model_options=model_options, + ) if clip is None: # comfy returns None for an unrecognised encoder rather than raising, # and a None CLIP fails at encode time, far from the mistake. @@ -124,4 +173,5 @@ class FunPackCLIPLoader(io.ComfyNode): f"{len(names)} encoder file(s) loaded as type {type}, device {device}: " + ", ".join(names)) listing = "\n".join(f" {i + 1}. {name}" for i, name in enumerate(names)) + listing += "".join(f"\n {gn}" for gn in gguf_notes) return io.NodeOutput(clip, f"FunPack CLIP Loader | type={type} device={device}\n{listing}") diff --git a/modules/loaders/diffusion_model/nodes.py b/modules/loaders/diffusion_model/nodes.py index 1a0ff298..75ac9461 100644 --- a/modules/loaders/diffusion_model/nodes.py +++ b/modules/loaders/diffusion_model/nodes.py @@ -11,11 +11,24 @@ import folder_paths from comfy_api.latest import io from ..._core import log +from .. import gguf_support from ..common import (COMPUTE_DTYPES, WEIGHT_DTYPES, attention_choices, attention_override, dtype_of, set_fp16_accumulation, weight_model_options) +def _model_file_choices(): + """Diffusion model files, `.gguf` included. + + Core's extension set has no `.gguf`, so those files are on disk and + invisible to every picker. Appended rather than merged in sorted order, so + an existing pipeline's saved choice keeps its position in the list and + nothing a user already picked moves. + """ + return list(folder_paths.get_filename_list("diffusion_models")) + \ + gguf_support.gguf_names("diffusion_models") + + class FunPackDiffusionModelLoader(io.ComfyNode): @classmethod def define_schema(cls) -> io.Schema: @@ -25,7 +38,7 @@ class FunPackDiffusionModelLoader(io.ComfyNode): category="FunPack/Loaders", description="Load a diffusion model, choosing precision and attention backend.", inputs=[ - io.Combo.Input("model_name", options=folder_paths.get_filename_list("diffusion_models")), + io.Combo.Input("model_name", options=_model_file_choices()), io.Combo.Input("weight_dtype", options=WEIGHT_DTYPES, default="default", tooltip="How the weights are stored. 'default' keeps whatever " "the file already is."), @@ -57,8 +70,31 @@ class FunPackDiffusionModelLoader(io.ComfyNode): model_options = weight_model_options(weight_dtype) notes.append(f"weight dtype: {weight_dtype}") - path = folder_paths.get_full_path_or_raise("diffusion_models", model_name) - state_dict, metadata = comfy.utils.load_torch_file(path, return_metadata=True) + if gguf_support.is_gguf(model_name): + path = gguf_support.gguf_path("diffusion_models", model_name) + if not path: + raise RuntimeError(f"{model_name} is no longer where it was listed from.") + misnamed = False + else: + path = folder_paths.get_full_path_or_raise("diffusion_models", model_name) + # The CONTENT decides, not the extension. A .gguf renamed to + # .safetensors used to reach the safetensors parser and fail with + # a UTF-8 decode error from reading a binary header as JSON -- + # true, and no help at all. + misnamed = gguf_support.has_gguf_magic(path) + if gguf_support.is_gguf(model_name) or misnamed: + if misnamed: + notes.append(f"{model_name} is named .safetensors but is a GGUF " + f"container -- loaded as GGUF") + 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 above -- a GGUF loaded with + # stock ops would try to matmul block-quantized storage. + model_options = {**model_options, **gguf_options} + metadata = None + notes.append(gguf_note) + else: + state_dict, metadata = comfy.utils.load_torch_file(path, return_metadata=True) model = comfy.sd.load_diffusion_model_state_dict( state_dict, model_options=model_options, metadata=metadata) if model is None: diff --git a/modules/loaders/gguf_support.py b/modules/loaders/gguf_support.py new file mode 100644 index 00000000..cfcdc0d2 --- /dev/null +++ b/modules/loaders/gguf_support.py @@ -0,0 +1,309 @@ +"""Load .gguf model files through FunPack's own loaders. + +Not a module: see common.py's own note -- a plain file at the domain level is a +helper the loaders import, and ships no node of its own. + +GGUF is two problems, and only the first is small: + +1. **Reading the container.** A header, tensor metadata, then quantized blocks. + The `gguf` package (llama.cpp's own) does this, and ships a reference + dequantizer for every quant type it defines. + +2. **Using the weights without dequantizing them.** The reason people reach for + GGUF is that a Q4 model occupies a quarter of the VRAM -- which only holds + if the weights STAY quantized and are expanded per-layer during the forward + pass. That needs a custom torch ops class, and hand-written kernels per + quant type. + +So this module does not hand-roll (2). It picks the best backend actually +present: + + * **ComfyUI-GGUF installed** -- use it. It is the mature implementation of + (2), and its output feeds `comfy.sd.load_diffusion_model_state_dict` + exactly like ours does, so FunPack's loader stays the only UI while its + runtime does the quantized maths. + * **`gguf` package only** -- read and dequantize at LOAD time, then hand over + an ordinary state dict. Fully native and correct, but the model occupies + its dequantized size, so the VRAM saving is gone. Said out loud rather than + left to be discovered. + * **Neither** -- refuse with the two ways to fix it, instead of a stack trace. + +Nothing here downloads or installs anything. +""" +from __future__ import annotations + +import importlib +import importlib.util +import os +import sys +import types +from typing import Optional + +GGUF_EXT = ".gguf" + + +def is_gguf(name: str) -> bool: + return str(name or "").lower().endswith(GGUF_EXT) + + +GGUF_MAGIC = b"GGUF" + + +def has_gguf_magic(path: str) -> bool: + """True when the FILE is a GGUF container, whatever it is called. + + Renaming a .gguf to .safetensors is an easy mistake to make, and the + result is an unreadable error from the safetensors parser trying to read a + binary header as UTF-8 JSON. Four bytes settle it, so the content decides. + """ + try: + with open(path, "rb") as f: + return f.read(4) == GGUF_MAGIC + except OSError: + return False + + +# ── finding .gguf files ─────────────────────────────────────────────────────── + +def gguf_names(folder: str) -> list[str]: + """`.gguf` files under a ComfyUI model folder, relative-named like get_filename_list(). + + Core's `supported_pt_extensions` has no `.gguf`, so `get_filename_list` + never returns one -- the files are on disk and simply invisible to every + picker. Scanned directly rather than by mutating core's global extension + set, which would change what every other node in the install offers. + """ + try: + import folder_paths + except ImportError: + return [] + out: list[str] = [] + seen: set[str] = set() + roots = list(folder_paths.get_folder_paths(folder) or []) + # ComfyUI-GGUF registers its own folders; include them under the same + # picker so a user who already keeps files there does not have to move them. + for extra in ("unet_gguf" if folder == "diffusion_models" else "clip_gguf",): + try: + roots += list(folder_paths.get_folder_paths(extra) or []) + except Exception: + pass + for root in roots: + if not root or not os.path.isdir(root): + continue + for dirpath, _dirs, files in os.walk(root, followlinks=True): + for f in files: + if not is_gguf(f): + continue + rel = os.path.relpath(os.path.join(dirpath, f), root) + if rel not in seen: + seen.add(rel) + out.append(rel) + return sorted(out) + + +def gguf_path(folder: str, name: str) -> Optional[str]: + """Absolute path of a `.gguf` picked out of `gguf_names`.""" + try: + import folder_paths + except ImportError: + return None + roots = list(folder_paths.get_folder_paths(folder) or []) + for extra in ("unet_gguf" if folder == "diffusion_models" else "clip_gguf",): + try: + roots += list(folder_paths.get_folder_paths(extra) or []) + except Exception: + pass + for root in roots: + cand = os.path.join(root or "", name) + if os.path.isfile(cand): + return cand + return None + + +# ── backends ────────────────────────────────────────────────────────────────── + +_PKG_NAME = "_funpack_gguf_pack" + + +def _ensure_pack_package(dirname: str) -> str: + """Register the pack's directory as an importable package, once. + + Loading `loader.py` as a standalone module by file path fails the moment + it does `from .ops import ...` -- "attempted relative import with no known + parent package", because a module loaded that way has no package to be + relative TO. So a parent package is synthesized with `__path__` pointing at + the directory, and submodules are imported underneath it, which is what + makes relative imports resolve. + + The pack's own `__init__.py` is deliberately NOT executed: it registers + the pack's nodes, and we want its loader, not a second copy of its node + list. + """ + pkg = sys.modules.get(_PKG_NAME) + if pkg is not None and getattr(pkg, "__path__", [None])[0] == dirname: + return _PKG_NAME + # Pointing at a different directory: drop the cached SUBMODULES too. + # Replacing only the parent leaves `.loader` in sys.modules, and + # import_module would hand back the old pack's code from the new path's name. + for name in [m for m in sys.modules if m == _PKG_NAME or m.startswith(_PKG_NAME + ".")]: + del sys.modules[name] + pkg = types.ModuleType(_PKG_NAME) + pkg.__path__ = [dirname] + pkg.__package__ = _PKG_NAME + sys.modules[_PKG_NAME] = pkg + return _PKG_NAME + + +def _load_pack_module(dirname: str, mod: str): + """Import one module out of a custom-node package by path. + + Custom node folders are not importable by name (``ComfyUI-GGUF`` is not an + identifier), so the directory is mounted as a package first and the + submodule imported from it. + """ + pkg = _ensure_pack_package(dirname) + return importlib.import_module(f"{pkg}.{mod}") + + +def _pack_dir() -> Optional[str]: + """ComfyUI-GGUF's directory, identified by what it CONTAINS rather than by + its name -- the folder gets renamed by Manager, by forks, and by hand.""" + try: + import folder_paths + base = os.path.join(folder_paths.base_path, "custom_nodes") + except Exception: + return None + if not os.path.isdir(base): + return None + for entry in sorted(os.listdir(base)): + d = os.path.join(base, entry) + loader = os.path.join(d, "loader.py") + if not os.path.isfile(loader) or not os.path.isfile(os.path.join(d, "ops.py")): + continue + try: + with open(loader, "r", encoding="utf-8", errors="replace") as fh: + if "gguf_sd_loader" in fh.read(): + return d + except OSError: + continue + return None + + +def backend() -> Optional[str]: + """"pack" (quantized in VRAM), "native" (dequantized at load), or None.""" + if _pack_dir(): + return "pack" + if importlib.util.find_spec("gguf") is not None: + return "native" + return None + + +UNAVAILABLE = ( + "No GGUF backend is available. The `gguf` package is one of FunPack's requirements, " + "so an install that predates .gguf support just needs them re-run:\n" + " pip install -r custom_nodes/ComfyUI-FunPack/requirements.txt\n" + "That gives a load-time dequantize: the file loads, but at its full size. To keep the " + "weights quantized in VRAM -- which is the point of GGUF -- install the ComfyUI-GGUF " + "node pack as well; FunPack uses it as the runtime and stays the only loader you wire." +) + + +def load_state_dict(path: str) -> tuple[dict, dict, str]: + """Read one .gguf into (state_dict, model_options, note). + + `model_options` carries the custom operations the quantized path needs; it + is empty for the native path, whose tensors are ordinary ones by the time + they are returned. + """ + return _load(path, clip=False) + + +def load_clip_state_dict(path: str) -> tuple[dict, dict, str]: + """Same as `load_state_dict`, for a text encoder. + + ComfyUI-GGUF reads encoders through a separate entry point: an encoder's + GGUF carries llama.cpp tensor names that have to be mapped back to the + transformer's, which its diffusion-model reader does not do. + """ + return _load(path, clip=True) + + +def _load(path: str, clip: bool) -> tuple[dict, dict, str]: + """Try the quantized backend, fall back to dequantizing, in that order. + + The pack refuses architectures it has no handling for -- "Unexpected + architecture type in GGUF file: 'minimax_h3'" -- which is a reasonable + refusal on its part and a dead end for us. Since the pack DEPENDS on the + `gguf` package, its presence guarantees the fallback is available, so a + refusal costs memory rather than the load. + """ + which = backend() + if which is None: + raise RuntimeError(UNAVAILABLE) + if which == "pack": + d = _pack_dir() + try: + loader = _load_pack_module(d, "loader") + ops = _load_pack_module(d, "ops") + read = loader.gguf_sd_loader + if clip: + read = getattr(loader, "gguf_clip_loader", None) or read + sd = read(path) + options = {} + ggml_ops = getattr(ops, "GGMLOps", None) + if ggml_ops is not None: + options["custom_operations"] = ggml_ops() + return sd, options, f"gguf: quantized (ComfyUI-GGUF at {os.path.basename(d)})" + except Exception as e: # noqa: BLE001 -- any refusal is a reason to fall back + if importlib.util.find_spec("gguf") is None: + raise + sd, options, note = _load_native(path) + return sd, options, (f"gguf: ComfyUI-GGUF could not read this file ({e}) -- " + f"fell back. {note}") + return _load_native(path) + + +def _load_native(path: str) -> tuple[dict, dict, str]: + """Dequantize every tensor to torch at load time using gguf's reference implementation. + + Correct but not thrifty: the model ends up at its dequantized size, so + this buys the ability to LOAD a .gguf, not GGUF's memory saving. The + caller states that. + """ + import gguf + import torch + + reader = gguf.GGUFReader(path) + sd: dict = {} + quantized = 0 + for tensor in reader.tensors: + name = str(tensor.name) + qtype = tensor.tensor_type + if qtype in (gguf.GGMLQuantizationType.F32, gguf.GGMLQuantizationType.F16): + arr = tensor.data + else: + # gguf.quants.dequantize is the reference implementation for every + # type the package knows -- do not reimplement it per quant type. + arr = gguf.quants.dequantize(tensor.data, qtype) + quantized += 1 + t = torch.from_numpy(arr.copy()) + # GGUF stores dimensions in the opposite order to torch. + shape = tuple(int(d) for d in reversed(tensor.shape)) + expected = 1 + for d in shape: + expected *= d + if shape and t.numel() != expected: + # Loading the tensor unshaped would hand the model a weight of the + # wrong rank and fail somewhere far from here, so stop with the + # name of the culprit. + raise RuntimeError( + f"{os.path.basename(path)}: tensor {name!r} dequantized to {t.numel()} " + f"elements but its header declares {shape} ({expected}). This container " + f"is not laid out the way the gguf package describes it.") + if shape: + t = t.reshape(shape) + sd[name] = t.to(torch.float16) if t.dtype == torch.float32 else t + note = (f"gguf: dequantized at load ({quantized} quantized tensors expanded) -- the " + f"file loads, but it occupies its full size in VRAM. Install ComfyUI-GGUF to " + f"keep it quantized.") + return sd, {}, note diff --git a/modules/loaders/tests/test_gguf_support.py b/modules/loaders/tests/test_gguf_support.py new file mode 100644 index 00000000..9705b82b --- /dev/null +++ b/modules/loaders/tests/test_gguf_support.py @@ -0,0 +1,248 @@ +"""GGUF backend selection and file discovery. + +Nothing here needs a .gguf file or the gguf package: what is testable is the +routing -- which backend gets picked, what happens when none is available, and +whether .gguf files are found at all (core's extension set excludes them, +which is the whole reason this module exists). +""" +import sys +import types + +import pytest + +from modules.loaders import gguf_support + + +@pytest.fixture +def fake_folders(tmp_path, monkeypatch): + """A stand-in folder_paths with two model roots on disk.""" + diff = tmp_path / "diffusion_models" + enc = tmp_path / "text_encoders" + sub = diff / "nested" + for d in (diff, enc, sub): + d.mkdir(parents=True, exist_ok=True) + (diff / "ltx.safetensors").write_bytes(b"x") + (diff / "ltx-Q4_K_M.gguf").write_bytes(b"x") + (sub / "h3-Q8_0.gguf").write_bytes(b"x") + (enc / "gemma.safetensors").write_bytes(b"x") + (enc / "gemma-Q5_K.gguf").write_bytes(b"x") + + mod = types.ModuleType("folder_paths") + mod.base_path = str(tmp_path) + roots = {"diffusion_models": [str(diff)], "text_encoders": [str(enc)]} + mod.get_folder_paths = lambda name: roots.get(name, []) + monkeypatch.setitem(sys.modules, "folder_paths", mod) + return tmp_path + + +def test_is_gguf_is_case_insensitive(): + assert gguf_support.is_gguf("model.gguf") + assert gguf_support.is_gguf("MODEL.GGUF") + assert not gguf_support.is_gguf("model.safetensors") + assert not gguf_support.is_gguf("") + assert not gguf_support.is_gguf(None) + + +def test_finds_gguf_files_core_cannot_list(fake_folders): + names = gguf_support.gguf_names("diffusion_models") + assert "ltx-Q4_K_M.gguf" in names + # Nested, because model folders are routinely organised into subdirectories. + assert any(n.endswith("h3-Q8_0.gguf") for n in names) + # Only .gguf -- the ordinary files still come from core's own listing. + assert not any(n.endswith(".safetensors") for n in names) + + +def test_encoders_are_listed_separately(fake_folders): + assert gguf_support.gguf_names("text_encoders") == ["gemma-Q5_K.gguf"] + + +def test_missing_folder_is_empty_not_an_error(fake_folders): + assert gguf_support.gguf_names("vae") == [] + + +def test_path_resolves_a_listed_name(fake_folders): + p = gguf_support.gguf_path("diffusion_models", "ltx-Q4_K_M.gguf") + assert p and p.endswith("ltx-Q4_K_M.gguf") + assert gguf_support.gguf_path("diffusion_models", "not-there.gguf") is None + + +def test_no_backend_refuses_with_both_remedies(monkeypatch): + monkeypatch.setattr(gguf_support, "_pack_dir", lambda: None) + monkeypatch.setattr(gguf_support.importlib.util, "find_spec", lambda name: None) + assert gguf_support.backend() is None + with pytest.raises(RuntimeError) as e: + gguf_support.load_state_dict("/nope.gguf") + msg = str(e.value) + # Both ways out must be named: they have different consequences for VRAM. + # `gguf` is a FunPack requirement now, so the first is a re-run of them + # rather than a lone package. + assert "ComfyUI-GGUF" in msg + assert "requirements.txt" in msg + assert "quantized in VRAM" in msg + + +def test_the_pack_wins_when_both_are_available(monkeypatch): + monkeypatch.setattr(gguf_support, "_pack_dir", lambda: "/fake/ComfyUI-GGUF") + monkeypatch.setattr(gguf_support.importlib.util, "find_spec", lambda name: object()) + # Quantized-in-VRAM is the point of GGUF; the dequantizing fallback is second best. + assert gguf_support.backend() == "pack" + + +def test_native_is_used_when_only_the_library_is_present(monkeypatch): + monkeypatch.setattr(gguf_support, "_pack_dir", lambda: None) + monkeypatch.setattr(gguf_support.importlib.util, "find_spec", + lambda name: object() if name == "gguf" else None) + assert gguf_support.backend() == "native" + + +def test_pack_is_found_by_content_not_by_folder_name(tmp_path, monkeypatch): + """Manager, forks and hand-installs all rename the folder; the marker is the code.""" + cn = tmp_path / "custom_nodes" + good = cn / "some-renamed-fork" + decoy = cn / "ComfyUI-GGUF-Themed-Nodes" # named right, contains nothing + for d in (good, decoy): + d.mkdir(parents=True) + (good / "loader.py").write_text("def gguf_sd_loader(path):\n return {}\n") + (good / "ops.py").write_text("class GGMLOps:\n pass\n") + (decoy / "loader.py").write_text("# nothing useful\n") + + mod = types.ModuleType("folder_paths") + mod.base_path = str(tmp_path) + mod.get_folder_paths = lambda name: [] + monkeypatch.setitem(sys.modules, "folder_paths", mod) + + assert gguf_support._pack_dir() == str(good) + + +def test_pack_load_passes_custom_operations_through(tmp_path, monkeypatch): + """The quantized weights are unusable without their ops -- losing them + would produce a matmul against block-quantized storage rather than a clear + failure.""" + d = tmp_path / "pack" + d.mkdir() + (d / "loader.py").write_text( + "def gguf_sd_loader(path):\n return {'w': 'quantized'}\n") + (d / "ops.py").write_text("class GGMLOps:\n pass\n") + monkeypatch.setattr(gguf_support, "_pack_dir", lambda: str(d)) + + sd, options, note = gguf_support.load_state_dict("/whatever.gguf") + assert sd == {"w": "quantized"} + assert "custom_operations" in options + assert type(options["custom_operations"]).__name__ == "GGMLOps" + assert "quantized" in note + + +def test_container_magic_beats_the_extension(tmp_path): + """A .gguf renamed to .safetensors reached the safetensors parser, which + read its binary header as UTF-8 JSON and failed with a decode error -- + true, and no help at all.""" + renamed = tmp_path / "ltx-Q4_K_M.safetensors" + renamed.write_bytes(b"GGUF\x03\x00\x00\x00" + b"\x80" * 200) + assert gguf_support.has_gguf_magic(str(renamed)) + # ...and the name alone still says nothing. + assert not gguf_support.is_gguf(renamed.name) + + +def test_a_real_safetensors_is_not_mistaken_for_gguf(tmp_path): + st = tmp_path / "real.safetensors" + st.write_bytes((8).to_bytes(8, "little") + b'{"a":{}}') + assert not gguf_support.has_gguf_magic(str(st)) + + +def test_magic_check_survives_a_missing_file(): + assert gguf_support.has_gguf_magic("/definitely/not/here.safetensors") is False + + +def test_pack_modules_can_use_relative_imports(tmp_path, monkeypatch): + """ComfyUI-GGUF's loader.py does `from .ops import ...`. Loading it as a + standalone module by file path raised "attempted relative import with no + known parent package"; mounting the directory as a package is what makes + it resolve.""" + d = tmp_path / "ComfyUI-GGUF" + d.mkdir() + (d / "__init__.py").write_text("raise AssertionError('the pack __init__ must not run')") + (d / "ops.py").write_text("class GGMLOps:\n pass\n") + (d / "dequant.py").write_text("MARKER = 'dequant'\n") + (d / "loader.py").write_text( + "from .ops import GGMLOps\n" + "from .dequant import MARKER\n" + "def gguf_sd_loader(path):\n" + " return {'w': MARKER}\n" + ) + monkeypatch.setattr(gguf_support, "_pack_dir", lambda: str(d)) + monkeypatch.delitem(sys.modules, gguf_support._PKG_NAME, raising=False) + + sd, options, note = gguf_support.load_state_dict("/whatever.gguf") + assert sd == {"w": "dequant"} + assert type(options["custom_operations"]).__name__ == "GGMLOps" + + +def test_switching_pack_directories_remounts(tmp_path, monkeypatch): + """The synthesized package is cached in sys.modules; a different directory + must not keep resolving to the first one.""" + for name, marker in (("a", "first"), ("b", "second")): + d = tmp_path / name + d.mkdir() + (d / "ops.py").write_text("class GGMLOps:\n pass\n") + (d / "loader.py").write_text( + f"def gguf_sd_loader(path):\n return {{'w': '{marker}'}}\n") + monkeypatch.delitem(sys.modules, gguf_support._PKG_NAME, raising=False) + + monkeypatch.setattr(gguf_support, "_pack_dir", lambda: str(tmp_path / "a")) + assert gguf_support.load_state_dict("/x.gguf")[0] == {"w": "first"} + + # No manual cache clearing: remounting a different directory must purge it + # by itself. + monkeypatch.setattr(gguf_support, "_pack_dir", lambda: str(tmp_path / "b")) + assert gguf_support.load_state_dict("/x.gguf")[0] == {"w": "second"} + + +def test_pack_refusing_an_architecture_falls_back(tmp_path, monkeypatch): + """ComfyUI-GGUF raises "Unexpected architecture type in GGUF file: + 'minimax_h3'" for architectures it has no handling for. That is a fair + refusal on its part and a dead end for us -- but the pack DEPENDS on the + gguf package, so the fallback is always there.""" + d = tmp_path / "pack" + d.mkdir() + (d / "ops.py").write_text("class GGMLOps:\n pass\n") + (d / "loader.py").write_text( + "def gguf_sd_loader(path):\n" + " raise ValueError(\"Unexpected architecture type in GGUF file: 'minimax_h3'\")\n") + monkeypatch.setattr(gguf_support, "_pack_dir", lambda: str(d)) + monkeypatch.delitem(sys.modules, gguf_support._PKG_NAME, raising=False) + monkeypatch.setattr(gguf_support.importlib.util, "find_spec", + lambda name: object() if name == "gguf" else None) + monkeypatch.setattr(gguf_support, "_load_native", + lambda p: ({"w": 1}, {}, "gguf: dequantized at load")) + + sd, options, note = gguf_support.load_state_dict("/h3.gguf") + assert sd == {"w": 1} + # Both halves must be in the note: what refused, and what happened instead. + assert "minimax_h3" in note + assert "dequantized at load" in note + # The quantized ops must NOT be carried over -- these tensors are plain. + assert "custom_operations" not in options + + +def test_a_pack_refusal_without_the_library_still_raises(tmp_path, monkeypatch): + """No silent success: with nothing to fall back to, the original refusal stands.""" + d = tmp_path / "pack" + d.mkdir() + (d / "ops.py").write_text("class GGMLOps:\n pass\n") + (d / "loader.py").write_text( + "def gguf_sd_loader(path):\n raise ValueError('nope')\n") + monkeypatch.setattr(gguf_support, "_pack_dir", lambda: str(d)) + monkeypatch.delitem(sys.modules, gguf_support._PKG_NAME, raising=False) + monkeypatch.setattr(gguf_support.importlib.util, "find_spec", lambda name: None) + with pytest.raises(ValueError, match="nope"): + gguf_support.load_state_dict("/h3.gguf") + + +def test_gguf_is_a_declared_requirement(): + """The advice in UNAVAILABLE is "re-run FunPack's requirements", which is + only true while gguf is actually in them.""" + import pathlib + reqs = pathlib.Path(__file__).resolve().parents[3] / "requirements.txt" + lines = [ln.strip() for ln in reqs.read_text(encoding="utf-8").splitlines() + if ln.strip() and not ln.startswith("#")] + assert any(ln == "gguf" or ln.startswith("gguf") for ln in lines) diff --git a/modules/loaders/tests/test_loader_behaviour.py b/modules/loaders/tests/test_loader_behaviour.py index 35756b3c..8b69ddee 100644 --- a/modules/loaders/tests/test_loader_behaviour.py +++ b/modules/loaders/tests/test_loader_behaviour.py @@ -200,3 +200,132 @@ def test_a_torch_build_without_the_flag_reports_that_rather_than_pretending(): from modules.loaders import common if getattr(getattr(torch.backends, "cuda", None), "matmul", None) is None: assert common.set_fp16_accumulation(True) is None + + +# --- GGUF routing: does the WIRING actually reach gguf_support.py, not just --- +# --- whether gguf_support.py itself works in isolation ------------------------- + +def test_a_gguf_model_name_is_routed_through_gguf_support(monkeypatch): + """A .gguf model file must never reach load_torch_file -- that parser + reads a safetensors header, and a GGUF container is not one.""" + import comfy.sd + from modules.loaders import gguf_support + from modules.loaders.diffusion_model import nodes + + monkeypatch.setattr(gguf_support, "gguf_path", lambda folder, name: f"/fake/{name}") + monkeypatch.setattr(gguf_support, "load_state_dict", + lambda path: ({"w": 1}, {"custom_operations": "ops"}, "gguf: quantized")) + monkeypatch.setattr(comfy.sd, "load_diffusion_model_state_dict", + lambda sd, model_options, metadata: object() if sd == {"w": 1} + and model_options.get("custom_operations") == "ops" else None) + + out = nodes.FunPackDiffusionModelLoader.execute( + model_name="model-Q4_K_M.gguf", weight_dtype="default", + compute_dtype="default", attention="default") + + assert out.result[0] is not None + assert "gguf: quantized" in out.result[1] + + +def test_a_safetensors_named_file_that_is_actually_gguf_is_still_routed(monkeypatch): + """The CONTENT decides, not the extension -- a .gguf renamed to + .safetensors must not reach the safetensors parser and fail with a + decode error that names nothing useful.""" + import comfy.sd + import folder_paths + from modules.loaders import gguf_support + from modules.loaders.diffusion_model import nodes + + monkeypatch.setattr(folder_paths, "get_full_path_or_raise", lambda kind, name: "/fake/path") + monkeypatch.setattr(gguf_support, "has_gguf_magic", lambda path: True) + monkeypatch.setattr(gguf_support, "load_state_dict", + lambda path: ({"w": 1}, {}, "gguf: dequantized at load")) + monkeypatch.setattr(comfy.sd, "load_diffusion_model_state_dict", + lambda sd, model_options, metadata: object()) + + out = nodes.FunPackDiffusionModelLoader.execute( + model_name="model.safetensors", weight_dtype="default", + compute_dtype="default", attention="default") + + assert "named .safetensors but is a GGUF container" in out.result[1] + + +def test_an_ordinary_safetensors_model_never_touches_gguf_support(monkeypatch): + """The common case must not pay for the uncommon one: a real + .safetensors file goes through load_torch_file, not gguf_support.""" + import comfy.sd + import comfy.utils + import folder_paths + from modules.loaders import gguf_support + from modules.loaders.diffusion_model import nodes + + monkeypatch.setattr(folder_paths, "get_full_path_or_raise", lambda kind, name: "/fake/path") + monkeypatch.setattr(gguf_support, "has_gguf_magic", lambda path: False) + monkeypatch.setattr(gguf_support, "load_state_dict", + lambda path: (_ for _ in ()).throw(AssertionError("gguf path taken"))) + monkeypatch.setattr(comfy.utils, "load_torch_file", + lambda path, return_metadata: ({"w": 1}, {})) + monkeypatch.setattr(comfy.sd, "load_diffusion_model_state_dict", + lambda sd, model_options, metadata: object()) + + nodes.FunPackDiffusionModelLoader.execute( + model_name="model.safetensors", weight_dtype="default", + compute_dtype="default", attention="default") + + +def test_a_gguf_text_encoder_is_routed_through_gguf_support(monkeypatch): + """A .gguf encoder cannot go through load_clip -- it reads files itself + and does not understand the container.""" + import comfy.sd + from modules.loaders import gguf_support + from modules.loaders.clip import nodes + + monkeypatch.setattr(gguf_support, "gguf_path", lambda folder, name: f"/fake/{name}") + monkeypatch.setattr(gguf_support, "load_clip_state_dict", + lambda path: ({"w": 1}, {}, "gguf: quantized")) + monkeypatch.setattr(comfy.sd, "load_text_encoder_state_dicts", + lambda state_dicts, **kw: object() if state_dicts == [{"w": 1}] else None) + + out = nodes.FunPackCLIPLoader.execute(clip_name1="encoder-Q5_K.gguf", type="ltxv") + + assert out.result[0] is not None + assert "gguf: quantized" in out.result[1] + + +def test_a_gguf_encoder_alongside_a_plain_connector_loads_both(monkeypatch): + """LTX-2.3's normal shape: a GGUF encoder plus a .safetensors connector + in the same list. Each slot must be read the way ITS file needs, not + however the first slot in the list happened to be.""" + import comfy.sd + import comfy.utils + import folder_paths + from modules.loaders import gguf_support + from modules.loaders.clip import nodes + + monkeypatch.setattr(gguf_support, "gguf_path", lambda folder, name: f"/fake/{name}") + monkeypatch.setattr(gguf_support, "load_clip_state_dict", + lambda path: ({"gguf": 1}, {}, "gguf: quantized")) + monkeypatch.setattr(folder_paths, "get_full_path_or_raise", lambda kind, name: f"/fake/{name}") + monkeypatch.setattr(comfy.utils, "load_torch_file", lambda path: {"plain": 1}) + seen = {} + monkeypatch.setattr(comfy.sd, "load_text_encoder_state_dicts", + lambda state_dicts, **kw: seen.update(sd=state_dicts) or object()) + + nodes.FunPackCLIPLoader.execute( + clip_name1="encoder-Q5_K.gguf", clip_name2="connector.safetensors", type="ltxv") + + assert seen["sd"] == [{"gguf": 1}, {"plain": 1}] + + +def test_a_gguf_encoder_that_vanished_since_being_listed_is_refused_by_name(monkeypatch): + """gguf_path() returns None once a listed file is gone. Without a check, + that None reaches load_clip_state_dict and fails deep inside gguf/pack + machinery instead of naming the missing file, the way the diffusion model + loader already refuses the identical condition.""" + from modules.loaders import gguf_support + from modules.loaders.clip import nodes + + monkeypatch.setattr(gguf_support, "gguf_path", lambda folder, name: None) + + with pytest.raises(RuntimeError, match="encoder-Q5_K.gguf.*no longer where it was listed"): + nodes.FunPackCLIPLoader.execute(clip_name1="encoder-Q5_K.gguf", type="ltxv") diff --git a/requirements.txt b/requirements.txt new file mode 100644 index 00000000..c4cac9e6 --- /dev/null +++ b/requirements.txt @@ -0,0 +1,7 @@ +# Version floors ONLY where FunPack needs one, and never a bare upgrade of +# anything the rest of ComfyUI compiles against. +# +# Reads .gguf containers in the FunPack loaders. Pure python + numpy; it does +# not pull CUDA or any model. Without it a .gguf needs the ComfyUI-GGUF node +# pack instead. +gguf