Fix the issue where subgraph does not work under --cache-none mode (#…

[HelloClyde]

Summary

This PR fixes an issue where subgraph execution fails when running with the --cache-none mode.

Root Cause

When running with DependencyAwareCache, the ensure_subcache_for function in comfy_execution/caching.py tries to access self.ancestors[child_id] before it exists, leading to a KeyError.

Solution

Added a safe initialization for the subcache object under --cache-none mode to ensure ancestors and related mappings are properly created before use.

Related issue

Fixes #10329

[HelloClyde]

Hi @Kosinkadink 👋
This PR fixes issue #10329 (--cache-none mode subgraph execution).
Please take a look when you have time. Thanks!

[rattus128]

I have been working on this recently and was looking at a different approach. I use this test case:

This change still excepts on this flow:

!!! Exception during processing !!! '23.0.0.Recurse'
Traceback (most recent call last):
  File "/home/rattus/ComfyUI/execution.py", line 548, in execute
    subcache = await cache.ensure_subcache_for(unique_id, new_node_ids)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/rattus/ComfyUI/comfy_execution/caching.py", line 423, in ensure_subcache_for
    self.descendants[node_id].add(child_id)
    ~~~~~~~~~~~~~~~~^^^^^^^^^
KeyError: '23.0.0.Recurse'

Prompt executed in 0.01 seconds

In this case, the for loop close is adding new fresh renamed node for re-iteration that the DAC is unaware of.

Even if this was fixed, the DAC will still have the issue that it does not handle the dependencies between the for loop nodes, as that subcache logic adds the entire for loop graph as children of the for loop close. This means the DAC cannot free nodes as part of a loop iteration as it goes.

I am currently working on a different approach where these dependencies are tracked within the executor itself which has a full copy of the graph at all times are scans for the deps on the subgraph queue.

I'll PR it today.

Can I get your test cases you used? I could use more data on the problem.

[HelloClyde]

I have been working on this recently and was looking at a different approach. I use this test case:

This change still excepts on this flow:

!!! Exception during processing !!! '23.0.0.Recurse'
Traceback (most recent call last):
  File "/home/rattus/ComfyUI/execution.py", line 548, in execute
    subcache = await cache.ensure_subcache_for(unique_id, new_node_ids)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/rattus/ComfyUI/comfy_execution/caching.py", line 423, in ensure_subcache_for
    self.descendants[node_id].add(child_id)
    ~~~~~~~~~~~~~~~~^^^^^^^^^
KeyError: '23.0.0.Recurse'

Prompt executed in 0.01 seconds

In this case, the for loop close is adding new fresh renamed node for re-iteration that the DAC is unaware of.

Even if this was fixed, the DAC will still have the issue that it does not handle the dependencies between the for loop nodes, as that subcache logic adds the entire for loop graph as children of the for loop close. This means the DAC cannot free nodes as part of a loop iteration as it goes.

I am currently working on a different approach where these dependencies are tracked within the executor itself which has a full copy of the graph at all times are scans for the deps on the subgraph queue.

I'll PR it today.

Can I get your test cases you used? I could use more data on the problem.

Thanks for the detailed explanation — you’re right, this PR only provides a minimal fix to prevent the exception.

My test cases are as follows:

NodeExpansion:
https://github.com/BadCafeCode/execution-inversion-demo-comfyui/blob/main/examples/NodeExpansion.png

SimpleLoop:
https://github.com/BadCafeCode/execution-inversion-demo-comfyui/blob/main/examples/SimpleForLoop.png

[Kosinkadink]

Hey @HelloClyde, we just merged @rattus128's PR #10368 for the --cache-none issue that should resolve all previous issues + make more features possible! He and I actually started talking about adding a RAM-aware caching method a few days before you submitted this PR, so work was already underway to fix --cache-none as a part of that effort. Thank you for your contribution nonetheless!