feat(sdk): support more than one exit handler per pipeline #8088
Conversation
|
Skipping CI for Draft Pull Request. |
|
/retest |
| second_exit_task = print_op(message='Second exit handler has worked!') | ||
|
|
||
| with dsl.ExitHandler(second_exit_task): | ||
| print_op(message=message).after(first_exit_print_task) |
There was a problem hiding this comment.
The after usage is an interesting one. Normally we don't allow dependency reference across DAGs. I wonder if we should disallow this?
In any case, the run result you showed me doesn't reflect the same dependency.
There was a problem hiding this comment.
Discussed offline and decided to disallow this to be consistent with dsl condition and looping
connor-mccarthy
force-pushed
the
enable-multiple-exit-handlers-per-pipeline
branch
from
99f335d to
16a415c
Compare
|
/test all |
connor-mccarthy
force-pushed
the
enable-multiple-exit-handlers-per-pipeline
branch
from
455bb90 to
6151802
Compare
connor-mccarthy
force-pushed
the
enable-multiple-exit-handlers-per-pipeline
branch
from
6151802 to
be7d29f
Compare
|
/test all |
connor-mccarthy
force-pushed
the
enable-multiple-exit-handlers-per-pipeline
branch
from
be7d29f to
d1e4aa8
Compare
|
/retest |
sdk/python/kfp/compiler/compiler.py
Outdated
| dependent_group = group_name_to_group.get( | ||
| upstream_groups[0], None) | ||
| if isinstance(dependent_group, | ||
| (tasks_group.Condition, tasks_group.ParallelFor)): | ||
| if isinstance(dependent_group, tasks_group.ParallelFor): |
There was a problem hiding this comment.
Shouldn't we add ExitHandler to the check instead of removing Condition from it?
There was a problem hiding this comment.
Yes, I was thinking about this the wrong way
connor-mccarthy
force-pushed
the
enable-multiple-exit-handlers-per-pipeline
branch
from
d1e4aa8 to
ba5340c
Compare
connor-mccarthy
force-pushed
the
enable-multiple-exit-handlers-per-pipeline
branch
from
24dc715 to
debfe4b
Compare
|
/lgtm Thanks @connor-mccarthy ! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chensun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…8088) * add compiler test pipeline with multiple exit handlers * remove blocker of multiple exit handlers * move exit handler builder logic to pipeline_spec_builder * build all exit handlers per pipeline * add compiler test with IR inspection * prevent usage of cross-pipeline after * test cross-pipeline after is prevented * update existing task dependency logic and tests * add v2 sample test * remove cross-pipeline .after * prevent cross-dag data dependency for dsl features * add compiler test pipeline with nested exit handlers * add support for nested exit handlers * clean up pipeline with nested exit handlers * remove sample with multiple exit handlers * remove compiler test with nested exit handlers * add compilation guard against nested exit handlers in subdag * update release notes
…8088) * add compiler test pipeline with multiple exit handlers * remove blocker of multiple exit handlers * move exit handler builder logic to pipeline_spec_builder * build all exit handlers per pipeline * add compiler test with IR inspection * prevent usage of cross-pipeline after * test cross-pipeline after is prevented * update existing task dependency logic and tests * add v2 sample test * remove cross-pipeline .after * prevent cross-dag data dependency for dsl features * add compiler test pipeline with nested exit handlers * add support for nested exit handlers * clean up pipeline with nested exit handlers * remove sample with multiple exit handlers * remove compiler test with nested exit handlers * add compilation guard against nested exit handlers in subdag * update release notes
Description of your changes:
Enables use of more than one exit handler per pipeline.
Checklist:
title convention. Learn more about the pull request title convention used in this repository.