-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(sdk): adding support for dsl.condition and dsl.parallelFor to local runner #12511
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
base: master
Are you sure you want to change the base?
Conversation
e49ddf0 to
61b068a
Compare
| for task_spec in dag_spec.tasks.values()) | ||
|
|
||
| # Route to enhanced orchestrator if control flow is detected | ||
| if has_control_flow: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the routing to the two different kinds of orchestrators be more explicit in the function and file structure? e.g. a top level router function (and maybe module as well) that routes to either dag_orchestrator or enhanced_dag_orchestrator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I think the dag_orchestrator should be decomposed and we should try to reuse any IO handling for both paths of dag executions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@droctothorpe @zazulam Just pushed a refactor of the code, and in fact added more test coverage and enhanced the enhanced_dag_orchectrator to support more dsl.conditions and parallelfor scenarios.
Can you take a relook at the code and let me know if this looks good to you now
| for task_spec in dag_spec.tasks.values()) | ||
|
|
||
| # Route to enhanced orchestrator if control flow is detected | ||
| if has_control_flow: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I think the dag_orchestrator should be decomposed and we should try to reuse any IO handling for both paths of dag executions.
| def _execute_task( | ||
| task_name: str, | ||
| task_spec: pipeline_spec_pb2.PipelineTaskSpec, | ||
| pipeline_resource_name: str, | ||
| components: Dict[str, pipeline_spec_pb2.ComponentSpec], | ||
| executors: Dict[str, | ||
| pipeline_spec_pb2.PipelineDeploymentConfig.ExecutorSpec], | ||
| io_store: io.IOStore, | ||
| pipeline_root: str, | ||
| runner: config.LocalRunnerType, | ||
| unique_pipeline_id: str, | ||
| fail_stack: List[str], | ||
| ) -> Tuple[Outputs, status.Status]: | ||
| """Execute a single task.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this function be extracted and just import it in both the base orchestration and the enhanced?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just refactored this code, and extracted common code to a utils file
| TestData( | ||
| name='Pipeline with Loops', | ||
| pipeline_func=pipeline_with_loops, | ||
| pipeline_func_args={'loop_parameter': ['item1', 'item2', 'item3']}, | ||
| expected_output=None, | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for nested loops and a loop with a subdag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added 2 more scenarios of which 1 has nested loops and a subdag
61b068a to
2b0ed94
Compare
8a2acf7 to
0f116e0
Compare
deec44a to
e50f65f
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Nelesh Singla <[email protected]>
e50f65f to
00baab0
Compare
zazulam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this @nsingla! Just had a few more thoughts from my end
| strategy: | ||
| matrix: | ||
| python-version: ['3.9', '3.13'] | ||
| python-version: ['3.11', '3.13'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for bumping this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we update our minimum supported python version to 3.11 recently, I just did that based on that, but if that's not true, I will revert it back to 3.9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We updated our images, but we were still testing the sdk against 3.9-3.13. We wanted to make sure that users who were slow to upgrade their python environments wouldn't be cut off from newer sdk releases.
| strategy: | ||
| matrix: | ||
| python-version: ['3.9', '3.13'] | ||
| python-version: ['3.11', '3.13'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same reason
| def test_parallel_for_supported(self): | ||
| # Use use_venv=False to avoid pip race conditions when installing | ||
| # packages in parallel virtual environments | ||
| local.init( | ||
| local.SubprocessRunner(use_venv=False), | ||
| pipeline_root=ROOT_FOR_TESTING) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to make sure that use_venv=True can work for small test cases of parallelFor, maybe using the parallelism value can help avoid issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm.... let me add that test case as well
Summary
Adding support for dsl.Condition and dsl.ParallelFor control flow features in the Kubeflow Pipelines Local Runner.
Key Implementation Details:
- ConditionEvaluator class for evaluating conditional expressions
- ParallelExecutor class for handling parallel task execution
- run_enhanced_dag() function that detects and routes control flow features
- Detects trigger_policy.condition for conditional tasks
- Detects WhichOneof('iterator') for parallel loop tasks
- Routes to enhanced orchestrator when control flow is detected
- Flip Coin (Conditional) - tests dsl.Condition support
- Pipeline with Loops (ParallelFor) - tests dsl.ParallelFor support
- Integrated with existing docker_specific_pipeline_funcs as requested
- Parent input parameters (from pipeline inputs)
- Task output parameters (from upstream task outputs)
- Raw JSON values in loop specifications
Technical Features
Test Results
The implementation successfully adds control flow support to Local Runner while maintaining full backward compatibility and integrating seamlessly with the existing test infrastructure.
Checklist: