Conversation
christian-byrne
left a comment
There was a problem hiding this comment.
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)
|
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. |
## Summary Add infrastructure for automatic node replacement feature that allows missing/deprecated nodes to be replaced with their newer equivalents. ## Changes - **Types**: `NodeReplacement`, `NodeReplacementResponse` types matching backend API spec (PR #12014) - **Service**: `fetchNodeReplacements()` API wrapper - **Store**: `useNodeReplacementStore` with `getReplacementFor()`, `hasReplacement()`, `isEnabled()` - **Setting**: `Comfy.NodeReplacement.Enabled` toggle (experimental) - **Tests**: 11 unit tests covering store functionality ## Related - Backend PR: Comfy-Org/ComfyUI#12014 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8364-feat-Add-node-replacement-store-and-types-2f66d73d3650816bb771c9cc6a8e1774) by [Unito](https://www.unito.io) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Automatic node replacement is now available, allowing missing nodes to be replaced with their newer equivalents when replacement mappings exist. * Added "Enable automatic node replacement" experimental setting in Workflow preferences (enabled by default). * Replacement data is loaded during app initialization. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…deReplaceManager + GET /api/node_replacements
5a629c6 to
d5b3da8
Compare
* refactor: process isolation support for node replacement API - Move REGISTERED_NODE_REPLACEMENTS global to NodeReplaceManager instance state - Add NodeReplacement class to ComfyAPI_latest with async register() method - Deprecate module-level register_node_replacement() function - Call register_replacements() from comfy_entrypoint() This enables pyisolate compatibility where extensions run in separate processes and communicate via RPC. The async API allows registration calls to cross process boundaries. Refs: TDD-002 Amp-Thread-ID: https://ampcode.com/threads/T-019c2b33-ac55-76a9-9c6b-0246a8625f21 * fix: remove whitespace and deprecation cruft Amp-Thread-ID: https://ampcode.com/threads/T-019c2be8-0b34-747e-b1f7-20a1a1e6c9df
…tead of objects, simplified the schema sent to the frontend, updated nodes_post_processing.py replacements to use new schema
… replacements when executing /prompt endpoint
guill
left a comment
There was a problem hiding this comment.
Using is_link(input_value) instead of is_instance(input_value, list) is the only change I feel really need to be made. Added a couple other nits, but feel free to ignore them.
Approving as these changes are all trivial and I don't think there's any value in me re-reviewing if those are the only changes made.
… code accordingly
…ing the public interface's node_replacement (not working currently, pushing to share)
Code 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 is a dictionary. The type is inferred by the keys present:
Map from old input (
old_id):{ "new_id": "string", "old_id": "string" }Set a fixed value (
set_value):{ "new_id": "string", "set_value": <any> }new_idold_idset_value)set_valueold_id)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", "old_id": "model"}, {"new_id": "steps", "old_id": "num_steps"}, {"new_id": "scheduler", "set_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_mappingInputMapInput mappings are TypedDicts. The mapping type is inferred by the dictionary keys:
InputMapOldId- Map from old input:new_idold_idInputMapSetValue- Set a fixed value:new_idset_valueOutputMapMaps an output from the old node to the new node by index.
new_idxold_idxUse Cases