Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change GraphExecutionState's _get_next_node method to prioritize Iterate Nodes #6032

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

cgi-joe
Copy link

@cgi-joe cgi-joe commented Mar 22, 2024

Summary

This PR optimizes the graph execution order by prioritizing Iterate nodes and their children, ensuring the correct execution sequence. The changes made to the _get_next_node method within the GraphExecutionState class address the issue of random output order and execution of Iterate node children.

Key changes include:

  • Extracting Iterate nodes from the topological order and sorting them based on their index attribute.
  • Prioritizing the execution of Iterate nodes and their children, ensuring the correct order.
  • Falling back to the topological order if no Iterate node or its children are ready for execution.

These improvements provide a more predictable and intuitive behavior for Iterate nodes.

Related Issues / Discussions

QA Instructions

To test the changes in this PR:

  1. Set up a workflow with one or more Iterate nodes.
  2. Execute the workflow and inspect the output of the Iterate nodes (select the node in the workflow editor and inspecting the "Outputs" tab in the lower side panel)
  3. Verify that the output order starts correctly with index 0 and that the item at index 0 is executed first.

Before this change, the index 0 item would be executed in a random order. With these changes, the execution order should be consistent and aligned with the index attribute of the Iterate nodes.

Merge Plan

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • Documentation added / updated (if applicable)

@github-actions github-actions bot added python PRs that change python files services PRs that change app services python-tests PRs that change python tests labels Mar 22, 2024
@psychedelicious
Copy link
Collaborator

Thanks for fixing this very long-standing and impactful issue!

Here's my test setup:
image

I also added a test to the PR for execution order.

@psychedelicious psychedelicious enabled auto-merge (rebase) March 25, 2024 22:07
@psychedelicious psychedelicious merged commit b4b1dbd into invoke-ai:main Mar 25, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python PRs that change python files python-tests PRs that change python tests services PRs that change app services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants