diff --git a/comfy_cli/workflow_to_api.py b/comfy_cli/workflow_to_api.py index 91adab2b9..c7044fd40 100644 --- a/comfy_cli/workflow_to_api.py +++ b/comfy_cli/workflow_to_api.py @@ -397,9 +397,131 @@ def _expand_one_subgraph( ] ) + next_id = _inject_promoted_widget_values(outer_node, sg_def, input_targets, expanded_nodes, expanded_links, next_id) + return expanded_nodes, expanded_links, input_targets, output_sources +def _promoted_widget_def_indices( + sg_def: dict, + input_targets: dict[int, list[tuple[Any, int]]], + node_by_id: dict[Any, dict], + outer_id: Any, +) -> list[int]: + """Def-input indices that materialize as widgets on a subgraph instance. + + The instance's ``widgets_values`` array holds one entry per promoted + widget, in def-input order. A def input is widget-backed exactly when the + interior input it feeds is itself widget-backed — which the serialized + graph marks with a ``widget`` key on that interior input slot. Deriving it + from the interior (rather than guessing from the def input's declared + type) is what keeps the indices aligned: a promoted widget can carry any + type at all, including node-pack types like ``COMFY_DYNAMICCOMBO_V3``, and + an allowlist that misses one shifts every later value into the wrong slot. + """ + indices: list[int] = [] + for def_idx, in_def in enumerate(sg_def.get("inputs") or []): + if not isinstance(in_def, dict): + continue + for target_id, target_slot in input_targets.get(def_idx) or []: + target = node_by_id.get(f"{outer_id}:{target_id}") + if not isinstance(target, dict): + continue + inputs = target.get("inputs") or [] + if not (isinstance(target_slot, int) and 0 <= target_slot < len(inputs)): + continue + slot = inputs[target_slot] + if isinstance(slot, dict) and slot.get("widget"): + indices.append(def_idx) + break + return indices + + +def _inject_promoted_widget_values( + outer_node: dict, + sg_def: dict, + input_targets: dict[int, list[tuple[Any, int]]], + expanded_nodes: list[dict], + expanded_links: list, + next_id: int, +) -> int: + """Apply the instance's promoted-widget values to the expanded interior. + + A subgraph instance exposes its interior nodes' unlinked widgets as its + own; the frontend's graphToPrompt substitutes those instance values into + the interior nodes at queue time. Without this, conversion silently falls + back to the interior nodes' saved defaults — an instance-edited prompt or + seed never reaches the API graph. + + Mechanism: for each promoted value, synthesize a virtual ``PrimitiveNode`` + carrying it and link it to every interior target of that def input. The + existing primitive-value machinery then injects the value (and skips the + virtual node in the output), exactly as it does for user-placed primitives. + + Values whose def input is externally linked on the instance are stale + residue — the link wins, same as any widget-behind-a-link on a regular + node — but they still occupy their slot in ``widgets_values``. + """ + inst_widgets = outer_node.get("widgets_values") + if not isinstance(inst_widgets, list) or not inst_widgets: + return next_id + outer_id = outer_node.get("id") + node_by_id = {n.get("id"): n for n in expanded_nodes} + promoted = _promoted_widget_def_indices(sg_def, input_targets, node_by_id, outer_id) + + # Fail closed. The value<->slot correspondence is purely positional, so a + # length disagreement means we cannot say which value belongs to which + # input. Falling back to the interior defaults is wrong but self-consistent + # and executable; a misaligned guess submits a prompt string into an INT + # slot. Prefer the recoverable failure — but warn, because every value the + # instance carries is being dropped and the run would otherwise look fine. + if len(promoted) != len(inst_widgets): + logger.warning( + "Subgraph instance %s exposes %d promoted widget(s) but carries %d " + "widgets_values; cannot align them, so the instance's values are " + "ignored and the subgraph's interior defaults run instead", + outer_id, + len(promoted), + len(inst_widgets), + ) + return next_id + + externally_linked = { + inp.get("name") + for inp in outer_node.get("inputs") or [] + if isinstance(inp, dict) and inp.get("link") is not None + } + + for widget_idx, def_idx in enumerate(promoted): + in_def = sg_def["inputs"][def_idx] + value = inst_widgets[widget_idx] + if value is None or in_def.get("name") in externally_linked: + continue + targets = input_targets.get(def_idx) or [] + if not targets: + continue + virtual_id = f"{outer_id}:promoted{def_idx}" + expanded_nodes.append( + { + "id": virtual_id, + "type": "PrimitiveNode", + "inputs": [], + "outputs": [], + "mode": 0, + "widgets_values": [value], + } + ) + for target_id, target_slot in targets: + expanded_links.append([next_id, virtual_id, 0, f"{outer_id}:{target_id}", target_slot, in_def.get("type")]) + target = node_by_id.get(f"{outer_id}:{target_id}") + if target is not None: + inputs = target.get("inputs") or [] + if isinstance(target_slot, int) and 0 <= target_slot < len(inputs): + inputs[target_slot]["link"] = next_id + next_id += 1 + return next_id + + def _rewrite_internal_input( input_info: dict, internal_link_map: dict[int, dict], link_id_remap: dict[int, int] ) -> dict: @@ -941,12 +1063,15 @@ def _build_api_node( ordered = _get_ordered_input_names(node_type, node, object_info) if ordered: # First widget-like values in the declared order, then link inputs. - # This matches what ComfyUI's "Save (API)" produces. + # This matches what ComfyUI's "Save (API)" produces. A primitive-fed + # input beats the node's own widget entry: when a widget has been + # converted to an input and wired, the frontend uses the incoming + # value and the widgets_values slot is stale residue. for name in ordered: - if name in widget_inputs: - api_node["inputs"][name] = widget_inputs[name] - elif name in primitive_inputs: + if name in primitive_inputs: api_node["inputs"][name] = primitive_inputs[name] + elif name in widget_inputs: + api_node["inputs"][name] = widget_inputs[name] elif name in default_inputs: api_node["inputs"][name] = default_inputs[name] for name in ordered: @@ -954,7 +1079,7 @@ def _build_api_node( api_node["inputs"][name] = link_inputs[name] # Anything we didn't know an order for is still emitted (preserves data). - for source in (widget_inputs, primitive_inputs, default_inputs, link_inputs): + for source in (primitive_inputs, widget_inputs, default_inputs, link_inputs): for key, value in source.items(): if key not in api_node["inputs"]: api_node["inputs"][key] = value diff --git a/tests/comfy_cli/test_workflow_to_api.py b/tests/comfy_cli/test_workflow_to_api.py index cfffe891a..311d24df0 100644 --- a/tests/comfy_cli/test_workflow_to_api.py +++ b/tests/comfy_cli/test_workflow_to_api.py @@ -2419,3 +2419,367 @@ def test_batch_option_maps_max_images_and_fail_on_partial(self, seedream_object_ assert "model.images" not in inputs assert "fixed" not in inputs.values() + + +class TestSubgraphPromotedWidgets: + """A subgraph instance's promoted-widget values must reach the interior + nodes. Regression: conversion used the interior nodes' saved defaults, so + an instance-edited prompt or seed silently never made it into the API + graph (bit every official MiniMax H3 template, whose prompt is a promoted + widget on the pipeline subgraph).""" + + SG = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + + def _workflow(self, instance_widgets, instance_inputs=None, external_links=None): + return { + "nodes": [ + { + "id": 100, + "type": self.SG, + "inputs": instance_inputs or [], + "outputs": [], + "mode": 0, + "widgets_values": instance_widgets, + }, + ], + "links": external_links or [], + "definitions": { + "subgraphs": [ + { + "id": self.SG, + "name": "Promoted", + # IMAGE first: connection-only def inputs consume no + # widgets_values entry. + "inputs": [ + {"name": "pixels", "type": "IMAGE", "linkIds": [300]}, + {"name": "text", "type": "STRING", "linkIds": [301]}, + ], + "outputs": [], + "nodes": [ + { + "id": 50, + "type": "CLIPTextEncode", + "inputs": [ + {"name": "clip", "link": None}, + {"name": "text", "link": 301, "widget": {"name": "text"}}, + ], + "outputs": [], + "mode": 0, + "widgets_values": ["interior default text"], + }, + ], + "links": [ + { + "id": 301, + "origin_id": -10, + "origin_slot": 1, + "target_id": 50, + "target_slot": 1, + "type": "STRING", + }, + ], + } + ] + }, + } + + def test_instance_widget_overrides_interior_default(self, object_info): + result = convert_ui_to_api(self._workflow(["instance text wins"]), object_info) + assert result["100:50"]["inputs"]["text"] == "instance text wins" + + def test_virtual_primitive_not_emitted(self, object_info): + result = convert_ui_to_api(self._workflow(["instance text wins"]), object_info) + assert not any("promoted" in key for key in result) + + def test_external_link_beats_stale_instance_widget(self, object_info): + # The def input is wired externally on the instance; its stale + # widgets_values entry must be ignored (link wins). + wf = self._workflow( + ["stale residue"], + instance_inputs=[{"name": "text", "link": 400}], + external_links=[[400, 7, 0, 100, 0, "STRING"]], + ) + wf["nodes"].insert( + 0, + { + "id": 7, + "type": "PrimitiveNode", + "inputs": [], + "outputs": [{"links": [400]}], + "mode": 0, + "widgets_values": ["linked value wins"], + }, + ) + # External link enters the subgraph at def-input index 1 ("text"). + wf["definitions"]["subgraphs"][0]["inputs"][1]["linkIds"] = [301] + result = convert_ui_to_api(wf, object_info) + assert result["100:50"]["inputs"]["text"] == "linked value wins" + + def test_missing_instance_widgets_keeps_interior_default(self, object_info): + result = convert_ui_to_api(self._workflow([]), object_info) + assert result["100:50"]["inputs"]["text"] == "interior default text" + + +class TestSubgraphPromotedWidgetOrdering: + """``widgets_values`` maps to promoted def inputs positionally, so the set + of def inputs that *count* as promoted has to be exactly right — one + missed entry shifts every later value into the wrong input. + + Regression: promotion was decided from an allowlist of def-input types + (STRING/INT/FLOAT/BOOLEAN/COMBO). A promoted widget can carry any type at + all, so a node-pack type like ``COMFY_DYNAMICCOMBO_V3`` fell through the + allowlist and shifted the rest — on the real corpus that put + ``"two_speakers"`` into a FLOAT ``audio_scale`` and ``1`` into + ``CLIPTextEncode.text``. Promotion is now read off the interior input's + ``widget`` marker instead. + + The fixture deliberately has five promoted inputs of five distinct types, + interleaved with connection-only inputs, so any off-by-N or reordering + surfaces as a type error rather than a silently plausible value. + """ + + SG = "cccccccc-dddd-eeee-ffff-000000000000" + + # Declared type per promoted def input, in def order, with the instance + # value each must receive. Values are mutually type-incompatible so a + # shift cannot masquerade as a correct assignment. + PROMOTED = [ + ("mode", "COMFY_DYNAMICCOMBO_V3", "two_speakers", str), + ("text", "STRING", "instance prompt", str), + ("steps", "INT", 24, int), + ("cfg", "FLOAT", 3.5, float), + ("sampler_name", "COMBO", "ddim", str), + ] + + @pytest.fixture + def object_info(self, object_info): + object_info["DynamicModeNode"] = { + "input": {"required": {"mode": ["COMFY_DYNAMICCOMBO_V3", {"default": "one_speaker"}]}}, + "input_order": {"required": ["mode"]}, + "output_node": False, + "output": ["MODE"], + "display_name": "Dynamic Mode", + } + return object_info + + def _workflow(self, instance_widgets, *, instance_inputs=None): + return { + "nodes": [ + { + "id": 100, + "type": self.SG, + "inputs": instance_inputs or [], + "outputs": [], + "mode": 0, + "widgets_values": instance_widgets, + }, + ], + "links": [], + "definitions": { + "subgraphs": [ + { + "id": self.SG, + "name": "Ordering", + # Connection-only inputs are interleaved with promoted + # ones: they consume no widgets_values slot, so a rule + # that miscounts them shifts the tail. + "inputs": [ + {"name": "pixels", "type": "IMAGE", "linkIds": [310]}, + {"name": "mode", "type": "COMFY_DYNAMICCOMBO_V3", "linkIds": [311]}, + {"name": "text", "type": "STRING", "linkIds": [312]}, + {"name": "clip", "type": "CLIP", "linkIds": [313]}, + {"name": "steps", "type": "INT", "linkIds": [314]}, + {"name": "cfg", "type": "FLOAT", "linkIds": [315]}, + {"name": "sampler_name", "type": "COMBO", "linkIds": [316]}, + ], + "outputs": [], + "nodes": [ + { + "id": 50, + "type": "DynamicModeNode", + "inputs": [{"name": "mode", "link": 311, "widget": {"name": "mode"}}], + "outputs": [], + "mode": 0, + "widgets_values": ["interior mode"], + }, + { + "id": 51, + "type": "CLIPTextEncode", + "inputs": [ + {"name": "text", "link": 312, "widget": {"name": "text"}}, + {"name": "clip", "link": 313}, + ], + "outputs": [], + "mode": 0, + "widgets_values": ["interior text"], + }, + { + "id": 52, + "type": "KSampler", + "inputs": [ + {"name": "model", "link": None}, + {"name": "seed", "link": None, "widget": {"name": "seed"}}, + {"name": "steps", "link": 314, "widget": {"name": "steps"}}, + {"name": "cfg", "link": 315, "widget": {"name": "cfg"}}, + { + "name": "sampler_name", + "link": 316, + "widget": {"name": "sampler_name"}, + }, + {"name": "positive", "link": None}, + {"name": "negative", "link": None}, + {"name": "latent_image", "link": None}, + ], + "outputs": [], + "mode": 0, + "widgets_values": [7, 99, 1.0, "euler", "normal", 1.0], + }, + { + "id": 53, + "type": "PreviewImage", + "inputs": [{"name": "images", "link": 310}], + "outputs": [], + "mode": 0, + }, + ], + "links": [ + { + "id": 310, + "origin_id": -10, + "origin_slot": 0, + "target_id": 53, + "target_slot": 0, + "type": "IMAGE", + }, + { + "id": 311, + "origin_id": -10, + "origin_slot": 1, + "target_id": 50, + "target_slot": 0, + "type": "COMFY_DYNAMICCOMBO_V3", + }, + { + "id": 312, + "origin_id": -10, + "origin_slot": 2, + "target_id": 51, + "target_slot": 0, + "type": "STRING", + }, + { + "id": 313, + "origin_id": -10, + "origin_slot": 3, + "target_id": 51, + "target_slot": 1, + "type": "CLIP", + }, + { + "id": 314, + "origin_id": -10, + "origin_slot": 4, + "target_id": 52, + "target_slot": 2, + "type": "INT", + }, + { + "id": 315, + "origin_id": -10, + "origin_slot": 5, + "target_id": 52, + "target_slot": 3, + "type": "FLOAT", + }, + { + "id": 316, + "origin_id": -10, + "origin_slot": 6, + "target_id": 52, + "target_slot": 4, + "type": "COMBO", + }, + ], + } + ] + }, + } + + def _values(self, result): + """Flatten every promoted input's resulting value, by def-input name.""" + by_name = {} + for node in result.values(): + for key, value in (node.get("inputs") or {}).items(): + by_name.setdefault(key, value) + return by_name + + def test_every_promoted_value_lands_in_its_own_input(self, object_info): + values = self._values(convert_ui_to_api(self._workflow([v for _, _, v, _ in self.PROMOTED]), object_info)) + assert {name: values.get(name) for name, _, _, _ in self.PROMOTED} == { + name: value for name, _, value, _ in self.PROMOTED + } + + def test_promoted_value_types_match_declared_input_types(self, object_info): + """The whole class of ordering bug: a shifted value is usually the + wrong Python type for the slot it lands in.""" + values = self._values(convert_ui_to_api(self._workflow([v for _, _, v, _ in self.PROMOTED]), object_info)) + mistyped = { + name: values.get(name) for name, _, _, py_type in self.PROMOTED if not isinstance(values.get(name), py_type) + } + assert not mistyped + + def test_connection_only_input_consumes_no_slot(self, object_info): + """``pixels``/``clip`` are link inputs with no interior widget: if + either were counted, the tail of widgets_values would shift.""" + result = convert_ui_to_api(self._workflow([v for _, _, v, _ in self.PROMOTED]), object_info) + assert self._values(result)["sampler_name"] == "ddim" + + def test_external_link_still_consumes_its_slot(self, object_info): + """A promoted input wired externally loses to the link, but its stale + entry keeps its place — dropping the slot would shift everything after + it.""" + wf = self._workflow( + [v for _, _, v, _ in self.PROMOTED], + instance_inputs=[{"name": "text", "type": "STRING", "widget": {"name": "text"}, "link": 400}], + ) + wf["nodes"].insert( + 0, + { + "id": 7, + "type": "PrimitiveNode", + "inputs": [], + "outputs": [{"links": [400]}], + "mode": 0, + "widgets_values": ["linked value wins"], + }, + ) + wf["links"] = [[400, 7, 0, 100, 0, "STRING"]] + values = self._values(convert_ui_to_api(wf, object_info)) + assert values["text"] == "linked value wins" + # Everything after the linked input keeps its own value. + assert (values["steps"], values["cfg"], values["sampler_name"]) == (24, 3.5, "ddim") + + def test_count_disagreement_falls_back_to_interior_defaults(self, object_info): + """Fail closed. If we cannot say which value belongs to which input, + the interior defaults are wrong but self-consistent and executable; a + misaligned guess submits a string into an INT slot.""" + values = self._values(convert_ui_to_api(self._workflow(["two_speakers", "instance prompt"]), object_info)) + assert values["text"] == "interior text" + assert values["mode"] == "interior mode" + assert values["steps"] == 99 + + def test_count_disagreement_warns(self, object_info, caplog): + """Dropping every instance value is not something to do quietly: the + prompt still converts and still runs, just with the wrong values.""" + import logging + + with caplog.at_level(logging.WARNING, logger="comfy_cli.workflow_to_api"): + convert_ui_to_api(self._workflow(["two_speakers", "instance prompt"]), object_info) + # Assert the level explicitly rather than leaning on at_level's + # threshold to filter a debug-level regression for us. + assert any( + rec.name == "comfy_cli.workflow_to_api" + and rec.levelno == logging.WARNING + and "interior defaults run instead" in rec.getMessage() + for rec in caplog.records + )