Skip to content

Conversation

RunDevelopment
Copy link
Member

Fixes #2807.

image

This PR adds lazy inputs and uses them to add a "Skip existing files" to Save Image. Lazy inputs work with the Lazy type which represent a lazy infallible computation. (While Lazy of course handles errors correctly, its users are supposed to assume errors don't happen/pass them up the stack.) This abstraction makes everything pretty easy to implement.

One bug that I haven't addressed yet is that lazy inputs mess with node timing. Since the time it takes to evaluate a lazy input currently counts towards both the node being lazily executed and the node evaluating the lazy execution, the time durations of Save Image nodes are wrong.
Not sure if this is a blocker for this PR or whether this can be fixed later.

@joeyballentine
Copy link
Member

So if I understand correctly, this works by not evaluating the path for each input until it's used, rather than before the node runs, so that if we don't need the input we don't evaluate it, but we evaluate everything else?

If that's the case, would there be any downside to doing this all the time for every input? (Besides the major refactoring we'd have to do) Feels like we could get some free optimizations out of this, and even accomplish the switch node optimization through this

@RunDevelopment
Copy link
Member Author

RunDevelopment commented Apr 19, 2024

That's exactly how it works.

would there be any downside to doing this all the time for every input? Feels like we could get some free optimizations out of this, [...]

Aside from the obvious fact that we'd have to change every single node, this would also make simple nodes a lot more complex. Every node would have to deal with Lazy, which would make chainner nodes more difficult to implement for new contributors and plugin authors. Also, most node use all of their inputs, so there's no point in delaying the evaluation of their arguments. This would just make things slower overall, because lazy evaluation also has an overhead.

So not only would this just make all nodes more complex, it would even make most chains slower.

even accomplish the switch node optimization through this

Yes, and no. Yes, Switch could be implemented like this in its current form, but this would make #2762 impossible, so no.

It's also plain more efficient to have Switch be a static optimization. Not only because the executor needs to plan for fewer nodes, but also because removing a Switch node may enable further chain optimizations. In general, any optimization that removes nodes/edges also improves other optimizations. Of course, we currently don't implement many optimizations, so I'm just speaking in the abstract right now.

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.

Skip existing images for Save Image
2 participants