-
Notifications
You must be signed in to change notification settings - Fork 66
Fix duplicated inputs on loading nested subgraphs #1192
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
Fix duplicated inputs on loading nested subgraphs #1192
Conversation
Subgraphs are loaded in order of creation. Under most circumstances, this means newer subgraphs are loaded first. With nested subgraphs, this means a subgraph node has it's inputs connected before it's inside is loaded. When the inner subgraph is loaded, input-added events are triggered even though inputs already exist on the subgraph node. This is resolved by adding a check for if an input of the corresponding name already exists when adding an input.
Testing RecommendationThis bug highlights a gap in our test coverage for subgraph deserialization scenarios. We should add unit tests to prevent regression: Suggested Test Cases
These tests would help catch similar issues in the future and ensure the event system handles both runtime and loading scenarios correctly. |
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.
This PR fixes a loading order issue where nested subgraphs loaded in creation order can cause duplicate inputs.
The problem occurs when:
- SubgraphNode has inputs connected before its inner subgraph loads
- Inner subgraph loading triggers input-added events for already-existing inputs
- The event handler adds duplicate inputs
The SubgraphNode constructor listens for input-added events:
subgraphEvents.addEventListener("input-added", (e) => {
const { name, type } = e.detail.input
this.addInput(name, type) // Runs even if input exists
})
The fix adds a guard to check for existing inputs:
if (this.inputs.find((i) => i.name == name))
return
This minimal fix preserves runtime synchronization while preventing duplication during loading. The event system was designed for runtime changes, not deserialization scenarios where state already exists.
Approved - clean fix that addresses the root cause.
|
Can we change the |
|
Absolutely. Will look into fixing my setup so it happens automatically like it does for frontend. |
|
Thank you. Soon, we will merge litegraph and frontend into a single repo. Details here: Comfy-Org/ComfyUI_frontend#4667. We should make an ADR as well so all contributors are involved in the change. |
Subgraphs are loaded in order of creation. Under most circumstances, this means newer subgraphs are loaded first. With nested subgraphs, this means a subgraph node has it's inputs connected before it's inside is loaded. When the inner subgraph is loaded, input-added events are triggered even though inputs already exist on the subgraph node. This is resolved by adding a check for if an input of the corresponding name already exists when adding an input. Port of Comfy-Org/litegraph.js#1192
Subgraphs are loaded in order of creation. Under most circumstances, this means newer subgraphs are loaded first. With nested subgraphs, this means a subgraph node has it's inputs connected before it's inside is loaded. When the inner subgraph is loaded, input-added events are triggered even though inputs already exist on the subgraph node.
This is resolved by adding a check for if an input of the corresponding name already exists when adding an input.