Python: Workflow Edge Groups#393
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces the concept of "Edge Groups" to the workflow framework, providing a more organized and flexible way to handle connections between workflow executors. The PR implements several types of edge groups to support different connection patterns, moving away from individual edge management to group-based edge management.
- Introduces the abstract
EdgeGroupbase class and five concrete implementations:SingleEdgeGroup,SourceEdgeGroup,TargetEdgeGroup,ConditionalEdgeGroup, andPartitioningEdgeGroup - Updates the workflow builder to use edge groups instead of individual edges, providing new methods for conditional and partitioning fan-out patterns
- Refactors the validation system and runner to work with edge groups while maintaining backward compatibility
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
step_06_map_reduce.py |
Cleanup of unused imports and code formatting improvements |
step_00_foundation_patterns.py |
New comprehensive sample demonstrating all edge group patterns with simple arithmetic operations |
test_workflow_builder.py |
Updated test assertion to check edge groups count instead of individual edges |
test_validation.py |
Updated validation tests to use SingleEdgeGroup instead of raw Edge objects |
test_runner.py |
Updated runner tests to use SingleEdgeGroup for consistency |
test_edge.py |
Added extensive test coverage for all new edge group implementations |
_workflow.py |
Major refactor to use edge groups, added new builder methods for conditional and partitioning patterns |
_validation.py |
Updated validation logic to work with edge groups while maintaining type compatibility checks |
_typing_utils.py |
Added handling for Any type in type checking utility |
_runner.py |
Refactored to work with edge groups instead of individual edges |
_executor.py |
Enhanced executor ID generation to include class name for better debugging |
_edge.py |
Major expansion adding the EdgeGroup hierarchy and all concrete implementations |
|
Something to consider here is that keeping the low-level set of edge types the minimal needed to implement all of the conceptual edges we want to support makes the toolkit more portable acros platforms. To that end, At the same time, having a richer set of types could make tooling easier, as it would allow tools to better understand the intent of an edge-group (conditional vs. partitioner). (Maybe we should support some kind of metadata through which higher-level toolkits could include information for tooling/visualization?) |
Good point!
@property
def type(self) -> str:
return self.__class__.__name__ |
|
The reason I think
No, I do not really have a concrete proposal. Just if we do what I suggest above, we want some way for tooling to be able to go back to the higher-level concept. Like the |
eavanvalkenburg
left a comment
There was a problem hiding this comment.
I have some big disagreements here, mostly with the terminology, and I think the code can be much simpler!
Python Test Coverage Report •
Python Unit Test Overview
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
eavanvalkenburg
left a comment
There was a problem hiding this comment.
small comment on the naming, but looking much better!
* Introducing edge groups * Add conditional and partitioning edge groups; next add samples and tests * Add unit tests * Add samples * Address comments 1 * Address comments 2 * Update conditional edge group to take in cases and default * Minor updates to sample * Collapsing Paritioning Edge group and Conditional Edge group to source edge group * Improve sample clarity * Name consolidation --------- Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com>
* Introducing edge groups * Add conditional and partitioning edge groups; next add samples and tests * Add unit tests * Add samples * Address comments 1 * Address comments 2 * Update conditional edge group to take in cases and default * Minor updates to sample * Collapsing Paritioning Edge group and Conditional Edge group to source edge group * Improve sample clarity * Name consolidation --------- Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com>
Motivation and Context
Addresses: #357
Related comment: #271 (comment)
Description
Contribution Checklist