rgthree / rgthree-comfy

Making ComfyUI more comfortable!
MIT License
1.03k stars 78 forks source link

Reroute nodes keep disconnecting #27

Closed alessandroperilli closed 1 year ago

alessandroperilli commented 1 year ago

Unfortunately, I have to report that the recent change did not fix the issue. After the Fix in Place of last week, I still have a few nodes that keep disconnecting randomly every time I reload the workflow or restart ComfyUI.

What's worse, these nodes would be very hard to impossible to troubleshoot for anyone not intimately familiar with the design decision I made for my workflow. And the new fix function doesn't even detect the issue.

Version 4.0 is ready to go, but I cannot release it until the reroute nodes issue is solved, and I only use your reroute nodes :)

Happy to share the new workflow, if you need it.

rgthree commented 1 year ago

Hmm.. What is most helpful is if we can recreate it (as in, load it and it's working, do something, then we find it's broken).

I haven't noticed any breakages locally since the last code changes, so your workflow is the best change to catch it. If you could paste your curent workflow, the nodes that are generally affected, and what you generally do between it loading correctly and breaking, so I can try to see if we can recreate it.

alessandroperilli commented 1 year ago

Thanks. The workflow is here.

Where to look

The affected route nodes seem to be the following:

  1. Red node carrying negative prompt here: Screenshot 2023-09-18 at 23 08 46

It disconnects 3 wires, 2 going up and 1 going down.

  1. Grey node carrying negative prompt here: Screenshot 2023-09-18 at 23 08 18

It disconnects 1 wire going to the right.

If I find others, I'll update this specific comment.

How to reproduce

I can replicate the issue, just not consistently:

A. The nodes that are causing the problem are always these two. Which is odd because, as you can see, this new v4 is an almost complete rewrite of the workflow you previously saw. Yet, even in previous 3.x versions, I always had 2 nodes carrying some negative prompt information keep disconnecting.

Why always 2 nodes? Coincidence? Why is has always something to do with the negative prompt? Coincidence, too?

Most certainly, but I wanted to point it out.

B. I discover these nodes disconnected after I have done one of the following actions + launched a run:

B1. Saved the workflow in PNG and reloaded the browser immediately after. B2. Interrupted ComfyUI via CTRL+C on the console and restarted it. B3. Fetched custom nodes updates, installed them, and then restarted ComfyUI.

However, I'm unable to systematically reproduce the problem. If I do B1 on repeat, the nodes don't disconnect. Same for the other actions. It's one of these 3 actions in combination with something else (a successful run? An interrupted run?).

Notice

  1. Out of desperation, I simply deleted the two problematic nodes and recreated them. It's not a sustainable strategy, and I'm almost certain that I tried this in the past, but I might have altered the conditions now. I'll update this comment if I see another disconnect.

  2. For reasons that I cannot understand, some of the most recent changes I have introduced in the workflow now cause a VERY LONG time to initialize the run. If you hit the Queue button and it seems like nothing is happening, it's just very, very slow. It will eventually start. I don't know what's happening but it's not related to the reroute disconnection problem. I had it before this new slow start issue.

I hope it helps. Happy to jump on a Discord chat to troubleshoot if our time zones make it viable.

rgthree commented 1 year ago

Cool. I'll see if I can take a look. The fact that they both come from "Negative Prompt Switch (Any)" makes me wonder if it's that node.. but their code doesn't look like it'd do something..

For the LONG time issue, there's some non-optimized recursion in ComfyUI itself. If you pull the latest from rgthree-node, it should fix it for you (I patch it until ComfyUI can get to it). Can see more here: https://github.com/comfyanonymous/ComfyUI/issues/1502

rgthree commented 1 year ago

Alright, I think I found the issue; interestingly enough, it did have to do with that Negative Switch node and was a bit of a race condition when loading the workflow.

The "Too Long" version: The issue was that Reroute nodes ensure that they're connected to valid inputs and outputs at either end of a chain. If something happens and they aren't, they'll disconnect from the invalid slots (like, with a reroute node that has a STRING type coming to its input and its output is connected to another nodes STRING input, if an IMAGE type is then connected to replace its input, the Reroute node will disconnect from its existing output connected node, since it's no longer a STRING type). It does this check when its connections change.

This works generally for most nodes. the bug here was your workflow's connected ImpactSwitch node initializes without input type information, so if the Reroute checks its connections before that ImpactSwitch decides it's a STRING type, it would disconnect the reroute chains outputs.

I've fixed it for reroute nodes which won't do this type enforcement while a graph is being configured. Also, debounced the check too, which will help naturally anyway.

alessandroperilli commented 1 year ago

I appreciate the long explanation. More of these are welcome.

I'm testing the new version of your suite to see if I have any more disconnect and if the recursion problem is gone, too.

Thank you!