Repository navigation
fix(graph): resolve If/Iterate/For branch deadlocks and gate branch For loops - #9789
Conversation
An If no longer waits for an unexecuted iterator whose frame a Collect closes before the If. Branch sources behind a Collect now resolve the If gate in its enclosing frame instead of silently dropping items. Iterators on a rejected branch no longer block downstream nodes.
Add regression cases for open iterator branches and root-frame If inputs.
JPPhoto
left a comment
There was a problem hiding this comment.
Merge blockers:
-
invokeai/app/services/shared/graph_materializer.py:1597-1611,1825-1830: with an openIteratefeeding anIfbranch, the materializer rejects theIfwhile activation gates the iterator on that unresolvedIf. The new direct-input test and both literal-branch cases stall. The PR description lists this shape as unsupported, but the new tests require completion. Effect: these validated graphs never finish. Likelihood: deterministic for the test graphs. Recovery: rewrite the graph and retry, or implement support. Test: the focused module reports765 passed, 6 failed; the direct-input case stalls withtrace=[]. -
invokeai/app/services/shared/graph_materializer.py:1550-1557: per-itemIfexecutions cannot attach root-frame branch outputs because attachment requires the source frame to equal the target frame. The new root-source test stalls afterconditionsanditeratenodes. Effect: the graph never finishes or collects its selected root values. Likelihood: deterministic for per-item conditions with root-frame branch sources. Recovery: rewrite the graph and retry, or support selecting root-frame values for per-itemIfs. Test: both scheduler-mode cases fail in the focused module.
Other findings/issues:
tests/app/services/shared/test_execution_engine_differential.py:3552-3563: this test passes when the scheduler stalls before preparing anyIf; the sibling case does so with an empty trace. Effect: it can pass without exercising the behavior named by the test. Likelihood: the current graph shape. Test: assert meaningful progress or the expected blocked state alongside the frame assertion.
Suggestions:
- Consider parameterizing the literal-branch cardinality test over both scheduler modes; it currently checks only the default mode.
…n can gate them Branch sources of an If whose frames depend on the selected branch, or that run shallower than every If frame, are admitted from the condition value; per-item Ifs broadcast parent-frame values. Branch frames are matched to If frames by structural depth, rejected iterators no longer add frames, and empty branches or conditions complete instead of stalling. An If whose condition and branch iterate unrelated collections raises UnsupportedIfFramesError instead of failing with a KeyError.
|
@JPPhoto thanks, all three findings were real. Fixed in e27f6fd; your new tests pass unchanged on both schedulers. Open Per-item
Probing beyond these turned up more shapes, all fixed with regression tests. The updated description lists them. Two are worth calling out:
Each of these shapes also failed on |
JPPhoto
left a comment
There was a problem hiding this comment.
Merge blockers:
invokeai/app/services/shared/graph_if_activation.py:147-151:For.output_collectionalready loses theForframe in_get_edge_depth(), thenCollect.itemsubtracts another frame. With a per-item condition[False, True]and a true branch containingFor -> Collect(For.output_collection), the branch runs for the false item too: I observed body executions at(0,0)and(0,1)while that item’s sink used the fallback. An error in that inactive body is recorded despite the false condition. Effect: inactive branch work can consume resources or fail a valid workflow. Likelihood: reproducible for mixed per-item conditions and this supported branch shape. Recovery: cancel and retry after correction; completed work may remain. Test: extend the For-output regression with[False, True]; assert no For/body execution for(0,), including an injected body failure.
Other findings/issues:
tests/app/services/shared/test_execution_engine_differential.py:3471-3487: thefor-outputandcollection-inputcases reportgeneric_allowed=False;force_compatibility_scheduler=Falsetherefore uses_ExecutionScheduler, same asTrue. Effect: these tests do not verify the claimed generic-scheduler coverage. Likelihood: confirmed for both cases. Test: assert the selected scheduler, or use a graph that actually qualifies for the generic scheduler.
## Summary The literal-If path fallback carries the supplied source graph through both If-to-If and non-If recursive paths. A focused regression test places an AddInvocation between nested Ifs and verifies constrained-graph fallback behavior; it fails if the non-If recursive call stops forwarding the supplied graph. ## Related Issues / Discussions Follow-up to #9786 review feedback. Issue #9484 and PR #9789 cover collected Iterate/If deadlock work outside this change. ## QA Instructions - `PYTHONOPTIMIZE=0 uv run --no-sync pytest -q tests/app/services/shared/test_graph_if_runtime.py tests/test_graph_execution_state.py::test_nested_if_literal_condition_activates_selected_branch -k 'not runtime_import_without_graph'` - 13 passed, 1 deselected. - The excluded import subprocess test fails with `free(): invalid pointer`; the same failure was reproduced on unchanged `main`. - Ruff check and format check passed. - `git diff --check` passed. ## Review The PR #9786 root-frame fix is already present in `main`. The remaining in-scope feedback is addressed, and no material review findings remain. ## Compatibility / Rollout <!-- When applicable: persisted-data migrations, API/generated-artifact compatibility, deployment ordering, and recovery considerations. Omit this section when unnecessary. --> ## Checklist - [x] _The PR has a short but descriptive title, suitable for a changelog_ - [x] _Meaningful regression coverage added / updated where needed; obsolete tests/code removed_ - [ ] _Persisted-state and API changes include required migrations / compatibility validation_ - [x] _Relevant performance/efficiency opportunities considered; material claims have evidence_ - [x] _Material review findings resolved and relevant checks rerun_ - [ ] _Documentation added / updated (if applicable)_ - [ ] _Updated `What's New` copy (if doing a release after this PR)_ <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/invoke-ai/codesmith/InvokeAI/pr/9815?autoLogin=true&ref=codesmith_pr_footer"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v3.svg"><img alt="View with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1794323543&installation_model_id=441795&pr_number=9815&ref=codesmith_pr_footer&repository=invoke-ai%2FInvokeAI&return_to=https%3A%2F%2Fgithub.com%2Finvoke-ai%2FInvokeAI%2Fpull%2F9815&signature=aef0ab086249e9e8d1beb1b800b0af56dc67ab89df9c37aa866d56210d957073"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>@codesmith-bot</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer -->
|
@JPPhoto thanks for the re-review. Both findings are real. I'm fixing the Merging #9843 into this branch exposed a semantic conflict I'd like you to settle before I go further, because both sides are your tests. A. If cardinality when only the unselected branch is per-item. Shape:
B. A branch that runs per inner item under a per-outer-item condition. Shape:
With a draft merge, every main test passes. All 24 failures are PR cases that follow the PR's model, where the If takes its frames from the branch it selects. Main's model takes them from every iterator above the If, so the result count doesn't depend on the condition. Which model do you want?
|
|
@lstein, I would preserve the current cardinality behavior for A in this PR. When the literal condition selects the root input, I expect the If to retain the surrounding per-item cardinality while leaving the unselected per-item computation inactive. Changing that to one root result is coherent, but it changes the output shape of a graph that already has defined behavior. I would decide that separately and specify how downstream grouping and empty branches behave. For B, I would keep the explicit error for this asymmetric case, where only the selected branch has deeper open axes. I would preserve the existing supported case where both branches share those deeper axes. Broadcasting the outer condition into a deeper branch while the fallback remains at the outer frame needs a clear output cardinality contract before we support it. I do not mean that If frames must always be static: completed Collect branches already preserve their selected frame groups. My recommendation is to keep the currently supported behavior by shape, make unsupported shapes fail clearly, and handle any broader selected-branch cardinality change as a separate semantics decision. |
Resolve against invoke-ai#9843, keeping its If frame cardinality and axis rejections. Gate branch For bodies with their If, and reject unrelated or uneven If axes up front. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@JPPhoto thanks. I merged Your findings:
New rejections: a literal One decision for you: a literal Known limitations, all shared with
|
JPPhoto
left a comment
There was a problem hiding this comment.
Some fixable items:
-
invokeai/app/services/shared/graph_if_runtime.py:37,100-115rejects a previously supported literal-Ifgraph. The unchangedtest_graph_static_nested_ifs_ignore_unselected_loop_branch_contextselectsa_for.output_collectionwithcondition=False, but validation also sees the unrelated axis from the unselectedb_forbranch and raisesUnsupportedIfIterationAxesError. The expected output has groups(0,) = ["a0", "a1"]and(1,) = ["a2"]. Linux and macOS CI fail on this test. Preserve this behavior and keep the regression test. -
The PR description still says an
Ifruns in the selected branch’s frames and describes tests for that cardinality model. The latest decision and current implementation keep cardinality static and reject some axis shapes. Update the description to match.
@lstein To answer your question, yes, I would reject unsafe reopened-axis shapes because they can silently drop items. For example, a workflow can collect processed images, append a reference through Collect.collection, then iterate the enlarged collection while an If pairs items against the earlier axis. The graph can complete with an empty group and a missing item. This is plausible batch composition, so I would not dismiss it as negligible. The proposed direct Collect-to-Iterate rule needs one more constraint: that Collect must not also append items through its collection input, or it does not guarantee matching cardinality.
…e reopens A literal If again runs in its selected branch's frames when an unrelated axis reaches it only through the unselected branch's For output. A literal If pairing an axis reopened after a Collect now requires a direct Collect-to-Iterate reopen over one nested chain of axes, and rejects reopens whose length can differ. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@JPPhoto thanks. Both items are addressed.
Reopened axes: done as you suggested, so this now closes #9917. A literal
Anything else raises Against |
Summary
A session could deadlock when an
Ifbranch, or the frames anIfruns in, depended on anIterateor aFor.next()returnedNone,is_complete()stayedFalse, and no error was raised, so the queue item hung. The simplest case is #9484'ssrc → iterate → body → collect → if.true_inputwith a literal condition. In a related case, a per-itemIfran aForon its rejected branch, and the error from thatForwas recorded.This PR builds on #9843. It keeps #9843's frame-cardinality and axis rules: an
Ifruns once per item of every iterator open at it, whichever branch it selects. Within those rules:Ifwaits only for the ancestors of its selected branch and for the iterators that give it its frames, so anIterateorForinside the rejected branch no longer blocks it.Foron a branch is gated together with its loop body. This includes aFornested in a gatedFor's body. A per-itemIftherefore runs none of that loop for items that reject the branch.Foroutputs. Iterators that reach anIfthrough aFor's final output count as frame sources for gating, empty-frame detection and validation.Ifwhose innermost frameIterateis empty produces no execution, and downstreamCollects get[].If's frame iterators, so theIfcan no longer close with zero executions before they run.UnsupportedIfIterationAxesError. These include unrelated open axes arriving through differentIfinputs, a root condition choosing between branches with different deeper axes, and a literalIfpairing an axis reopened after aCollectwhose length can differ from the closed axis. A reopen is accepted only when theIteratereads directly from the closingCollect(or a chain ofCollects), thatCollecttakes oneiteminput over one nested chain of axes, and nothing is appended through itscollectioninput. An unrelated axis that reaches a literalIfonly through the unselected branch'sForoutput adds no frames and is allowed.Related Issues / Discussions
Closes #9484
Closes #9917
Builds on #9843, whose description lists #9818–#9821 as addressed. Semantics follow the maintainer's decision in this PR's discussion: keep the existing If cardinality, and reject unsupported axis relationships explicitly.
QA Instructions
uv run --no-sync pytest -n 4 tests→ 14526 passed, 211 skipped, 7 xfailed. The graph tests also pass under severalPYTHONHASHSEEDvalues.tests/app/services/shared/test_execution_engine_differential.pyassert completion plus exact frames and values, or the rejection error before invocation. All of these shapes run on the compatibility scheduler, and each test asserts which scheduler ran. Every test fails onmainexcept two: the cross-product and the closed-then-reopened axis tests guard against over-rejecting shapesmainalready runs.If/Iterate/Collect/Forgraph shapes were compared againstmainon both schedulers:maincompletes produces a different value.maincompletes.Collectruns ahead of anIfthat never ran.mainstalls on them too (see Review, bug(graph): computed-condition If stalls when its branch reaches an empty Iterate through a For #9918).-m slow tests/app/services/shared/test_for_scheduler_performance.py, medians of 3 runs interleaved withmain: all within 1% (for example, 15.02s against 15.00s, and 5.26s against 5.24s).ruff checkandruff format --checkpass.Review
Foris nested in the body. Also fixed:Ifs insideForbodies and onIterate → For → Ifthat this branch had introduced.Ifthat closed empty before its inner iterators ran.main:Iffed directly byFor.output_collection, with noCollect, still runs theForfor items that reject that branch. TheIfcan only be prepared once thatForhas finished.Ifwhose branch reaches an emptyIterateonly through aFor's final output stalls (bug(graph): computed-condition If stalls when its branch reaches an empty Iterate through a For #9918).For's body still runs. This wastes work but changes no results.Collectof aCollectkeeping axes deeper than the condition is accepted and stalls (bug(graph): computed-condition If stalls on a Collect-of-Collect branch deeper than its condition #9922).maincompletes them correctly: reopens with a node between theCollectand theIterate(its length cannot be checked statically), reopens of aCollectwhose item crosses unrelated axes, and reopens through moreCollects than the item has axes. Each of these can drop or misgroup items in a closely related shape, so they fail explicitly.Compatibility / Rollout
No persisted-state or API changes. Graphs whose behaviour changes previously hung, ran work on rejected branches, or produced wrong values. Unsupported axis relationships now raise
UnsupportedIfIterationAxesErrorbefore any node runs.Checklist
What's Newcopy (if doing a release after this PR)🤖 Generated with Claude Code