Allow any valid node with no incoming connections and with side effects to run automatically #2944
Conversation
|
This needs to be an opt-in system, I think. We have nodes like Create Noise that can potentially take multiple seconds to run. I think the a1111 web UI Text to Image node is also valid without incoming connections. Having nodes like this run every time you change an input isn't a good idea. |
|
And about correctness: |
|
That's a valid concern. But also, I think you misread this PR. I didn't change the actual isStartingNode function. In fact, I don't even use it for this calculation anymore. |
I thought about this for a bit, and I'm wondering if this is actually true. We're basically just running any node we can run individually ahead of time, which will end up saving a bit of time when the entire chain is run. I don't really see a downside. In fact, I almost think these nodes (create noise, create gradient, etc) should have a large image output like load image, so you can create the image and visualize it without having to re-run the chain constantly to see it. |
|
One thing to note is that since this doesnt actually change the isStartingNode function, the hook for doing those fast type tags won't run for these anymore, so I'm thinking I probably should change it to be done there instead. As for implementation, I'm thinking of making "starting nodes" have side effects (technically all of them do anyway since they display some kind of information after being run. The idea is that since these nodes need to run, and we run them automatically, they have side effects. Limiting this to just nodes with side effects would mean we can opt-in to this feature in an easier way. Thoughts? |
|
I got rid of the isStartingNode function and replaced it with a new hook that determines whether "automatic" features should be used. Then, i made it so that the load nodes have side effects, and use side effects to help determine whether the node should run. Technically, these nodes do have side effects, so it was kinda incorrect before anyway. I still would kinda like to have more nodes run automatically, but i think i'll just give those nodes large image outputs in a separate PR and give them side effects. |
joeyballentine
changed the title
joeyballentine
changed the title
|
Going to merge this as I need it for what i'm going to work on next. Feel free to make PRs for any further suggestions |
| @@ -166,6 +166,7 @@ def _for_ext(ext: str | Iterable[str], decoder: _Decoder) -> _Decoder: | |||
| DirectoryOutput("Directory", of_input=0), | |||
| FileNameOutput("Name", of_input=0), | |||
| ], | |||
| side_effects=True, | |||
There was a problem hiding this comment.
@joeyballentine Making these nodes side-effect nodes is major change. It completely messes up our current system of "only nodes with side effects run" and is going to cause these nodes to be executed unnecessarily.
Why did you make this change in the first place?
There was a problem hiding this comment.
It actually doesn't mess that up in practice, at least not that I found in testing. I think it's because they already run.
Anyway, I did that because they technically do have side effects. I explained this. We've even talked about similar things before: when I gave every image output a preview, you said they all had side effects. When we talked about giving every node the ability to toggle an image preview inline in the output, you said we'd need a way to toggle side effects on those nodes.
Why is that suddenly not the case now?
This is necessary to be able to allow connections to Load Image. (which I will do in a separate future PR)