Skip to content
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 non-serialized widget values included in graph state check #3323

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

christian-byrne
Copy link
Contributor

@christian-byrne christian-byrne commented Apr 5, 2025

Changes graphEqual comparison to ignore non-serialized widget values. These values are not part of the restorable state of the graph, so it does not make sense for them to influence the state check comparison.

Previously, this issue did not surface because either (a) the widget was not properly set to be non-serialized or (b) the non-serialized widget was added prior to the graph being configured. In #2689, one instance of (a) was fixed with the LoadImage node, which caused this bug to surface. This issue is not actually caused by serialize widget option. See update in comments.

Afterwards, any graph with LoadImage nodes would be marked dirty upon first opening, which casued countless issues, including interfering with tab restore (as unsaved tabs are not restored) and causing manual confirmation when closing otherwise unmodified tabs.

Resolves #3313, #3254, comfyanonymous/ComfyUI#7475

┆Issue is synchronized with this Notion page by Unito

@christian-byrne christian-byrne requested review from a team as code owners April 5, 2025 17:49
Copy link
Member

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think whether to serialize widget value should be a litegraph side API instead.

@christian-byrne
Copy link
Contributor Author

Can you elaborate a bit with specifics? The issue here is just that we are using the widget concept for the file upload button and image preview, but since they are not really widgets with values, we don't serialize them. However, it then causes the loaded workflow to be different from the runtime workflow on the very first graphEquals check.

@huchenlei
Copy link
Member

I think the root cause is that when loading the graph is complete before the images are loaded, the image preview widget is not added to the graph yet, so we are getting wrong initial state.

@christian-byrne
Copy link
Contributor Author

christian-byrne commented Apr 8, 2025

Update: The issue is not actually caused by #2689 (options.serialize property of the widget).

options.serialize is used in graphToPrompt and should not affect the saved state of non-API workflows. I.e., non-serialized widgets are still present in the saved non-API workflow. For example, LoadImage nodes still have three items in their widgets_values in non-API workflows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: activeState of LoadImage node contains extra empty string in widgets_values
2 participants