Skip to content

fix: guarantee proper ordering of decompose subtask dependencies#407

Open
AngeloDanducci wants to merge 3 commits intogenerative-computing:mainfrom
AngeloDanducci:ad-decompose-order
Open

fix: guarantee proper ordering of decompose subtask dependencies#407
AngeloDanducci wants to merge 3 commits intogenerative-computing:mainfrom
AngeloDanducci:ad-decompose-order

Conversation

@AngeloDanducci
Copy link
Contributor

@AngeloDanducci AngeloDanducci commented Feb 3, 2026

Misc PR

Type of PR

  • Bug Fix
  • New Feature
  • Documentation
  • Other

Description

Guarantee proper ordering of decompose subtask dependencies and associated tests.

Testing

  • Tests added to the respective file if code was changed
  • New code has 100% coverage if code as added
  • Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2026

The PR description has been updated. Please fill out the template for your PR to be reviewed.

@mergify
Copy link

mergify bot commented Feb 3, 2026

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 Enforce conventional commit

Wonderful, this rule succeeded.

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert|release)(?:\(.+\))?:

@AngeloDanducci AngeloDanducci changed the title ensure proper ordering of decompose vars Guarantee proper ordering of decompose subtask dependencie Feb 3, 2026
@AngeloDanducci AngeloDanducci changed the title Guarantee proper ordering of decompose subtask dependencie fix: guarantee proper ordering of decompose subtask dependencie Feb 3, 2026
@AngeloDanducci AngeloDanducci changed the title fix: guarantee proper ordering of decompose subtask dependencie fix: guarantee proper ordering of decompose subtask dependencies Feb 3, 2026
Raises:
ValueError: If a circular dependency is detected
"""
# Build dependency graph
Copy link
Member

@psschwei psschwei Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be any benefit to changing the internal representation of tasks to a graph rather than a list (within all of decompose, not just here)? I'm pretty sure the answer is no, but this is probably the time to ask 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I was looking at it a bit further down the chain and the only benefit would be that you wouldn't need this intermediary bit to verify ordering.

Thinking about that though, while the subtasks are ordered correctly I think I need to double check that the numbering matches as well post re-ordering.

Copy link
Member

@psschwei psschwei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I'll leave final approval for someone who knows this space a little better

defined_subtask_tags.add(subtask_tag)

# Reorder if needed
if needs_reordering:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we log that this is happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to the idea.

From a user perspective nothing related to the creation of the output is currently available until the final output (though you will see some progress markers via cli) - this behavior is unchanged.

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.

bug: m decompose intermediary script is not always propery ordered correctly for user variables

2 participants