-
Notifications
You must be signed in to change notification settings - Fork 11.5k
Node Replacement API #12014
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Node Replacement API #12014
Conversation
christian-byrne
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the draft! Left some inline comments. Overall the old_widget_ids approach is the right solution for mapping positional widget values to names.
Summary of suggestions:
- Rename
old_widget_ids→old_widget_names(they're names, not IDs) - Clarify
InputMap.OldId— is it for input slot names or widget names? Consider splitting - Add docstrings explaining that
old_widget_namesorder must match serialization order - Consider
Cache-Controlheader for the GET endpoint (static per session)
| new_node_id: str, | ||
| old_node_id: str, | ||
| old_widget_ids: list[str] | None=None, | ||
| input_mapping: list[InputMap] | None=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming to old_widget_names — these are ordered widget names, not IDs.
|
|
||
| class OldId(_Assign): | ||
| """ | ||
| Connect the input of the old node with given id to new node when replacing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring says "input of the old node with given id" but old_id could refer to either an input slot name or a widget name (from old_widget_ids). Consider splitting into OldInputName and OldWidgetName subclasses, or clarify in the docstring which it references.
| Defines a possible node replacement, mapping inputs and outputs of the old node to the new node. | ||
|
|
||
| Also supports assigning specific values to the input widgets of the new node. | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a docstring to NodeReplace explaining that old_widget_ids must be in the same order as the old node's widgets_values serialization order. This is crucial for custom node devs to understand — the positional mapping is the key insight.
|
One clarification needed in the docs: is Also worth clarifying that Example scenario that could confuse devs: if a node has 3 linkable inputs and 4 widgets, |
|
The purpose of the old_widgets_ids field is to pseudo-assign what the expected input id of the widget field is, such that all widgets can still be referred to by an input id like input links. It's why imo we should call them 'ids' instead of names, even if in the json the inputs have the 'name' property - I don't think there is a pragmatic use case for separating widget value transferal with input link transferal - if a link connected to a widget input slot is transferred, it should naturally follow that the value of the widget should also be transferable in that case. The name (id) of the input stored in the json is the same as what the id of the widget would be. All input transferal should be explicit - if a widget/input on old node is not assigned to something on the new node, then it should not be carried over. So in the case that there are more widgets in the old node than new, the ones that aren't referenced directly should be ignored. Thus if the list of old_widget_ids is shorter than the amount of widgets in the old node, it can be assumed that only those first N widgets need to be enumerated with the id alias, others can be ignored. We def need to make it clear in the docstring that the old_widget_ids are for the labeling of widgets_values in the json. |
|
I have changed UseValue (and use_value string) to SetValue (set_value string now), it feels better. |
This draft PR can now be used to work on the frontend.
Everything here is subject to change if we find anything to add before merging.
Unrelated to frontend, I will have claude code refactor the classes in _node_replace.py to use pydantic that should be able to cover our use case perfectly. Not a blocker.
Can also add some validation potentially, but also not a blocker for initial work.
Note: 99% of the PR text below was generated by Claude Code and I have reviewed it to be correct. Here is some of my personal notes:
use_valueassign_type is made for this caseCode snippet:
GET /api/node_replacements
Returns all registered node replacements. Node replacements define how to migrate from deprecated/old nodes to their newer equivalents, including how to map inputs and outputs between them.
Request
No parameters required.
Response
Returns a JSON object where keys are old node IDs and values are arrays of possible replacements for that node.
Response Schema
{ "<old_node_id>": [ { "new_node_id": "string", "old_node_id": "string", "old_widget_ids": ["string", ...] | null, "input_mapping": [...] | null, "output_mapping": [...] | null } ] }Fields
new_node_idold_node_idold_widget_idsinput_mappingoutput_mappingWidget ID Binding
The
old_widget_idsfield is used to bind input IDs to their relative widget indexes. This is necessary because the graph JSON file stores widget values by their relative position index, not by their ID. By providing this ordered list, the system can resolve which widget value corresponds to which input ID when performing the replacement.For example, if
old_widget_idsis["steps", "cfg", "sampler"], then:"steps""cfg""sampler"Input Mapping
Each input mapping entry has the following structure:
{ "new_id": "string", "assign": { "assign_type": "old_id" | "set_value", "old_id": "string", // only if assign_type is "old_id" "value": <any> // only if assign_type is "set_value" } }new_idassign.assign_type"old_id"(map from old input) or"set_value"(use a fixed value)assign.old_idassign_typeis"old_id")assign.valueassign_typeis"set_value")Output Mapping
Each output mapping entry has the following structure:
{ "new_idx": 0, "old_idx": 0 }new_idxold_idxExample Response
{ "OldSamplerNode": [ { "new_node_id": "NewSamplerNode", "old_node_id": "OldSamplerNode", "old_widget_ids": ["num_steps", "cfg_scale", "sampler_name"], "input_mapping": [ { "new_id": "model", "assign": { "assign_type": "old_id", "old_id": "model" } }, { "new_id": "steps", "assign": { "assign_type": "old_id", "old_id": "num_steps" } }, { "new_id": "scheduler", "assign": { "assign_type": "set_value", "value": "normal" } } ], "output_mapping": [ { "new_idx": 0, "old_idx": 0 } ] } ] }Registering Node Replacements
Custom node developers can register replacements using the
comfy_api.latestmodule:Classes
NodeReplaceDefines a node replacement mapping.
new_node_idold_node_idold_widget_idsinput_mappingoutput_mappingInputMapMaps an input from the old node to the new node.
new_idassignInputMap.OldIdConnect an input from the old node to the new node.
old_idInputMap.SetValueUse a fixed value for the new node's input widget.
valueOutputMapMaps an output from the old node to the new node by index.
new_idxold_idxUse Cases