ForNodes don't seem to cache output and have inverse iteration order

[pmrv]

I noticed that in the MWE below, the prints in the for node trigger on every pull, but in the range node only once (as intended). Also the for loop prints the list from the last item backwards. Afaict, this is not specified and by piping the input through the node output is not ambiguous, but it's a bit non-intuitive.

@Workflow.wrap.as_function_node('y')
def Double(x: int) -> int:
    print(x)
    return x*2

@Workflow.wrap.as_function_node('range')
def Range(max: int) -> list[int]:
    print('MAX', max)
    return list(range(max))

dl = Double.for_node(iter_on='x', output_as_dataframe=False)

r = Range(4)

dl.inputs.x = r.outputs.range

dl.pull()

dl.pull()

[liamhuber]

Also the for loop prints the list from the last item backwards.

This is an artifact of the sorting. I'm sure we could fix it so that in "normal" circumstances it prints them in the right order, but I don't think we need to bother -- each child in the for loop is formally independent of the other children so execution order is arbitrary. What matters is that the order is preserved in the output, and this is the case.

I noticed that in the MWE below, the prints in the for node trigger on every pull, but in the range node only once (as intended). ... Afaict, this is not specified and by piping the input through the node output is not ambiguous, but it's a bit non-intuitive.

Ayayay. This is actually a nasty side effect of the for loop not being lazy enough. Whenever you add, remove, or replace a child node in a Composite, it clears the cache. This is 100% right and good behaviour. At the same time, in order to accommodate variable length input, For macros rebuild their child graph at run-time. This is also correct -- But what we really need to do is first check if we need to rebuild the body. I.e. do we already have cached input, and does that cached input match our current input, in which case short-circuit. This short-circuiting happens in the return super()._on_run(), but self._build_body() necessarily comes before that.

I guess this would fix it:

def For._on_run(self):
    if not self.cache_hit:
        self._build_body()
    return super()._on_run()

(I also see that cache_hit excepts a raw Exception, ick and my bad...)

[pmrv]

Agree that the sorting is not critical, just found it curious.

For now I've worked around this by writing the for loops by hand, which I'm happy with for the time being, so we can leave the fix until times are a bit quieter.

[liamhuber]

Ah, I wrote a patch already anyhow 😂 just running tests locally