diff --git a/app/shell/tests/transport.test.js b/app/shell/tests/transport.test.js index 2b3e190a..a756d7a1 100644 --- a/app/shell/tests/transport.test.js +++ b/app/shell/tests/transport.test.js @@ -94,3 +94,56 @@ test("every phase says something, including ones added later", () => { assert.ok(text && text.trim(), `${phase} draws an empty status`); } }); + +// --- a refusal from the queue, which names the nodes ------------------------ + +test("a graph the queue refused says which node and why", () => { + // run.js captured ComfyUI's node_errors from the start; describe() only ever + // read the singular `node`, which is set by the WebSocket crash path and + // never by this one. So every value refusal showed ComfyUI's top-level + // string -- "Prompt outputs failed validation" -- over a pipeline that can + // hold a dozen loaders, with nothing saying which to fix. + const said = describe({ + phase: FAILED, images: [], + error: { + message: "Prompt outputs failed validation", + nodes: { + vae: { + class_type: "FunPackVAELoader", + errors: [{ type: "value_not_in_list", message: "Value not in list", + details: "vae_name: 'gone.safetensors' not in []" }], + }, + }, + }, + }); + assert.match(said, /FunPackVAELoader/); + assert.match(said, /vae/); + assert.match(said, /Value not in list/); + assert.match(said, /gone\.safetensors/); +}); + +test("a crash during execution still names its one node", () => { + const said = describe({ + phase: FAILED, images: [], + error: { node: "sampler", message: "shape mismatch" }, + }); + assert.equal(said, "sampler: shape mismatch"); +}); + +test("a refusal with no node errors falls back to what it did say", () => { + const said = describe({ + phase: FAILED, images: [], + error: { message: "the queue could not be reached: fetch failed", nodes: {} }, + }); + assert.match(said, /could not be reached/); +}); + +test("a node error carrying nothing readable does not swallow the message", () => { + // An entry with an empty errors array would otherwise return "" and read as + // a run that failed for no reason at all. + const said = describe({ + phase: FAILED, images: [], + error: { message: "refused", nodes: { "4": { class_type: "X", errors: [] } } }, + }); + assert.equal(said, "refused"); +}); diff --git a/app/shell/transport.js b/app/shell/transport.js index 3141e34f..7222d1a2 100644 --- a/app/shell/transport.js +++ b/app/shell/transport.js @@ -96,6 +96,43 @@ export function createTransport({ onGenerate, onCancel } = {}) { return { node: bar.node, draw, say, hold, release, generate, cancel, progress, status }; } +/** + * Why a run failed, in one line, from whichever of the two shapes it arrived in. + * + * A crash during execution names one node -- `node`. A graph the queue refused + * names as many as were wrong -- `nodes`, an object keyed by node id, each with + * its own reasons. Only the first was ever read, so the whole of the second was + * captured and thrown away: the bar showed ComfyUI's top-level string, which is + * "Prompt outputs failed validation" and says nothing about which of a dozen + * loaders to go and fix. + */ +export function failure(error) { + if (!error) return "the run failed"; + + const first = firstNodeError(error.nodes); + if (first) return first; + const where = error.node ? `${error.node}: ` : ""; + return `${where}${error.message || "the run failed"}`; +} + +function firstNodeError(nodes) { + if (!nodes || typeof nodes !== "object") return null; + for (const [id, entry] of Object.entries(nodes)) { + const reasons = entry && Array.isArray(entry.errors) ? entry.errors : []; + for (const reason of reasons) { + const what = reason && (reason.message || reason.type); + if (!what) continue; + // The class type as well as the id: a node id is a number in ComfyUI's + // own graphs and a slot name in ours, and neither on its own is much to + // go on when the message is "Value not in list". + const which = entry.class_type ? `${entry.class_type} (${id})` : id; + const detail = reason.details ? ` — ${reason.details}` : ""; + return `${which}: ${what}${detail}`; + } + } + return null; +} + /** One line saying where the run is. The only place these words are decided. */ export function describe(state) { switch (state.phase) { @@ -114,10 +151,8 @@ export function describe(state) { return state.images.length ? `Done — ${state.images.length} result${state.images.length === 1 ? "" : "s"}` : "Done, but nothing was saved"; - case FAILED: { - const where = state.error && state.error.node ? `${state.error.node}: ` : ""; - return `${where}${(state.error && state.error.message) || "the run failed"}`; - } + case FAILED: + return failure(state.error); case CANCELLED: return "Cancelled"; default: diff --git a/core/graph.py b/core/graph.py index 593717c8..660470e8 100644 --- a/core/graph.py +++ b/core/graph.py @@ -62,6 +62,15 @@ class Schemas: found = self.of(class_type) return list(found.get("required", [])) if found else [] + def limits(self, class_type: str) -> Dict[str, dict]: + """What each input will ACCEPT: a combo's choices, a number's bounds. + + Absent from a schema means unknown, which is checked as no limit -- + a schema that does not say is not a schema that says no. + """ + found = self.of(class_type) + return dict(found.get("limits", {})) if found else {} + def from_comfyui() -> Schemas: """Schemas read from whatever ComfyUI has registered, FunPack's and everyone @@ -72,7 +81,7 @@ def from_comfyui() -> Schemas: if node is None: return None spec = node.INPUT_TYPES() - inputs, required = {}, [] + inputs, required, limits = {}, [], {} for section in ("required", "optional"): for name, declared in (spec.get(section) or {}).items(): kind, options = comfy_types.declared(declared) @@ -86,8 +95,19 @@ def from_comfyui() -> Schemas: # thing that says a slot is incomplete -- and a slot missing # a required input built clean and failed once queued. required.append(name) + + bounds = {} + if inputs[name] == comfy_types.COMBO: + bounds["choices"] = (list(kind) if isinstance(kind, (list, tuple)) + else comfy_types.choices(options)) + for edge in ("min", "max"): + if edge in options: + bounds[edge] = options[edge] + if bounds: + limits[name] = bounds + return {"inputs": inputs, "outputs": list(node.RETURN_TYPES), - "required": required} + "required": required, "limits": limits} return Schemas(lookup) @@ -175,6 +195,7 @@ def build(slots: Sequence[dict], schemas: Optional[Schemas] = None) -> Tuple[dic continue declared = schemas.inputs(class_type) + limits = schemas.limits(class_type) inputs = {} for name, value in (slot.get("inputs") or {}).items(): if name not in declared: @@ -200,6 +221,10 @@ def build(slots: Sequence[dict], schemas: Optional[Schemas] = None) -> Tuple[dic continue inputs[name] = [source, index] else: + problem = unacceptable(name, value, limits.get(name)) + if problem: + problems.append(f"{slot_id}.{problem}") + continue inputs[name] = value for name in schemas.required(class_type): if name not in inputs: @@ -212,6 +237,42 @@ def build(slots: Sequence[dict], schemas: Optional[Schemas] = None) -> Tuple[dic return prompt, problems +def unacceptable(name: str, value: Any, bounds: Optional[dict]) -> Optional[str]: + """Why this literal will not do, or None. + + ComfyUI checks these too, at /prompt -- and it refuses the WHOLE graph when + one value is wrong, which v4 hit with a LoRA file that had been deleted: a + feature that was switched off still stopped every generation. Refusing here + means the reason arrives naming the slot and the input, before anything is + queued, instead of as "Prompt outputs failed validation". + + No bounds means unknown, and unknown is not a refusal. + """ + if not bounds: + return None + + choices = bounds.get("choices") + if choices is not None: + # An empty list is a machine with no files of that kind, not a value + # that was checked and rejected -- the slot being unfilled is what gets + # reported, and saying "x is not one of: " helps nobody. + if choices and value not in choices: + offered = ", ".join(repr(c) for c in choices[:6]) + more = "" if len(choices) <= 6 else f", and {len(choices) - 6} more" + return (f"{name} is {value!r}, which is not one of: {offered}{more}") + return None + + # Only a number has bounds, and only a number is compared to them. A bool is + # an int in Python and would sail through a naive comparison as 0 or 1. + if isinstance(value, bool) or not isinstance(value, (int, float)): + return None + if "min" in bounds and value < bounds["min"]: + return f"{name} is {value}, below the smallest {bounds['min']} it takes" + if "max" in bounds and value > bounds["max"]: + return f"{name} is {value}, above the largest {bounds['max']} it takes" + return None + + def cycles(prompt: Dict[str, dict]) -> List[str]: """Slots that feed each other, named in the order the loop runs. diff --git a/core/tests/test_graph.py b/core/tests/test_graph.py index e62bcd57..5d96f49f 100644 --- a/core/tests/test_graph.py +++ b/core/tests/test_graph.py @@ -240,3 +240,84 @@ def test_a_diamond_is_not_a_cycle(): {"id": "join", "node": "TwoIn", "inputs": {"a": ["left", 0], "b": ["right", 0]}}, ], SCHEMAS) assert problems == [] + + +# --- a value ComfyUI would refuse ------------------------------------------- +# +# ComfyUI checks these too, at /prompt, and it refuses the WHOLE graph when one +# value is wrong. v4 hit exactly that with a LoRA file that had been deleted: a +# feature that was switched OFF still stopped every generation, and the message +# was "Prompt outputs failed validation" over a graph of a dozen loaders. + +def _schemas_with_limits(): + return graph.Schemas(lambda ct: { + "Loader": { + "inputs": {"ckpt_name": "COMBO", "steps": "INT", "on": "BOOLEAN"}, + "outputs": ["MODEL"], "required": ["ckpt_name"], + "limits": {"ckpt_name": {"choices": ["a.safetensors", "b.safetensors"]}, + "steps": {"min": 1, "max": 100}}, + }, + "Empty": {"inputs": {"pick": "COMBO"}, "outputs": [], "required": [], + "limits": {"pick": {"choices": []}}}, + }.get(ct)) + + +def test_a_combo_value_that_is_no_longer_on_disk_is_refused_before_queueing(): + _prompt, problems = graph.build( + [{"id": "model", "node": "Loader", "inputs": {"ckpt_name": "gone.safetensors"}}], + _schemas_with_limits()) + assert any("gone.safetensors" in p and "not one of" in p for p in problems), problems + assert any("model.ckpt_name" in p for p in problems), problems + + +def test_a_combo_value_that_is_on_disk_is_accepted(): + prompt, problems = graph.build( + [{"id": "model", "node": "Loader", "inputs": {"ckpt_name": "b.safetensors"}}], + _schemas_with_limits()) + assert problems == [] + assert prompt["model"]["inputs"]["ckpt_name"] == "b.safetensors" + + +def test_a_number_outside_what_the_node_takes_is_refused(): + _prompt, problems = graph.build( + [{"id": "model", "node": "Loader", + "inputs": {"ckpt_name": "a.safetensors", "steps": 0}}], + _schemas_with_limits()) + assert any("below the smallest" in p for p in problems), problems + + _prompt, problems = graph.build( + [{"id": "model", "node": "Loader", + "inputs": {"ckpt_name": "a.safetensors", "steps": 1000}}], + _schemas_with_limits()) + assert any("above the largest" in p for p in problems), problems + + +def test_a_bool_is_not_measured_against_a_number_bound(): + """True is 1 in Python, so a naive comparison lets a checkbox be judged + against a step count -- and quietly passes it.""" + prompt, problems = graph.build( + [{"id": "model", "node": "Loader", + "inputs": {"ckpt_name": "a.safetensors", "on": True}}], + _schemas_with_limits()) + assert problems == [] + assert prompt["model"]["inputs"]["on"] is True + + +def test_an_empty_choice_list_is_reported_as_unfilled_not_as_a_wrong_value(): + """A machine with no files of that kind has not rejected anything. Saying + "x is not one of: " helps nobody, and the slot being unfilled is already + reported.""" + _prompt, problems = graph.build( + [{"id": "e", "node": "Empty", "inputs": {"pick": "whatever"}}], + _schemas_with_limits()) + assert not any("not one of" in p for p in problems), problems + + +def test_a_schema_that_says_nothing_about_a_value_does_not_refuse_it(): + """Most injected schemas carry no limits at all. Unknown is not a refusal: + a schema that does not say is not a schema that says no.""" + schemas = graph.Schemas(lambda ct: {"inputs": {"anything": "STRING"}, + "outputs": [], "required": []}) + _prompt, problems = graph.build( + [{"id": "x", "node": "Whatever", "inputs": {"anything": "hello"}}], schemas) + assert problems == [] diff --git a/core/tests/test_routes_pipeline.py b/core/tests/test_routes_pipeline.py index 535faea8..3f1de8e9 100644 --- a/core/tests/test_routes_pipeline.py +++ b/core/tests/test_routes_pipeline.py @@ -302,3 +302,48 @@ def test_every_dropdown_in_the_registry_is_offered_as_one(registered): if w["type"] == comfy_types.COMBO) assert dropdowns > 100, f"only {dropdowns} dropdowns found; the sweep is not seeing them" + + +def test_a_file_that_is_no_longer_there_is_refused_here_not_at_the_queue(server, registered): + """v4's failure, reproduced against real nodes. + + A LoRA file that had been deleted made ComfyUI refuse the WHOLE prompt -- + a feature that was switched off still stopped every generation, and the + message was "Prompt outputs failed validation" over a graph of a dozen + loaders. ComfyUI checks this at /prompt, which is one step after our own + check says the pipeline is queueable. + """ + _status, body = _request(server, "GET", "/funpack/api/pipeline") + slots = body["slots"] + for slot in slots: + if slot["id"] == "model": + slot["inputs"]["model_name"] = "deleted-yesterday.safetensors" + + _status, checked = _request(server, "POST", "/funpack/api/pipeline", + {"action": "check", "slots": slots}) + assert checked["queueable"] is False + assert any("deleted-yesterday.safetensors" in problem and "not one of" in problem + for problem in checked["incomplete"]), checked["incomplete"] + assert checked["prompt"] is None, "a graph the queue would refuse was handed over" + + +def test_a_value_the_node_does_offer_still_builds(server, registered): + """The control. Without it the test above passes on a check that refuses + everything.""" + from core import widgets + + _status, body = _request(server, "GET", "/funpack/api/pipeline") + slots = body["slots"] + described = widgets.describe("FunPackDiffusionModelLoader") + choices = next(w for w in described["widgets"] if w["name"] == "model_name")["choices"] + if not choices: + pytest.skip("no model files on this machine, so there is no valid value to use") + + for slot in slots: + if slot["id"] == "model": + slot["inputs"]["model_name"] = choices[0] + + _status, checked = _request(server, "POST", "/funpack/api/pipeline", + {"action": "check", "slots": slots}) + assert not any("not one of" in problem for problem in checked["incomplete"]), \ + checked["incomplete"]