-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(sdk): Support using pipeline in exit handlers #8220
feat(sdk): Support using pipeline in exit handlers #8220
Conversation
/retest |
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 Chen, the test looks great to me!
) | ||
exit_task_component_spec = exit_task_pipeline_spec.root | ||
else: | ||
raise RuntimeError |
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.
nit: Should we add an error message? (e.g. Exit task is missing both container spec and pipeline spec)
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.
Updated.
|
||
with dsl.ExitHandler(exit_task, name='my-pipeline'): | ||
print_op(message=message) | ||
fail_op(message='Task failed.') |
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 wondering, do we support nested exit handling? For example, there may be ways to handle a failed inner task but let the outer task run as usual.
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.
Good question. While I believe IR can possibly support nested exit handler, I'm not sure if Argo can support it.
So unless we have concrete use case/strong feature demand, I think we'll hold off adding such a feature that could potentially break symmetry between KFP and Vertex (again, this needs investigation on whether Argo can support it).
/lgtm |
/approve |
[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 |
/test kubeflow-pipelines-samples-v2 |
/lgtm Thank you for this, @chensun! And thanks for reviewing @zichuan-scott-xu. |
* Support using pipeline in exit handlers * release note * remove dead code * fix component spec merging flakiness * address comments in PR#8209 * address review comments * fix bad merge in release note
Description of your changes:
Checklist: