-
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 local pipeline execution #localexecution #10423
feat(sdk): support local pipeline execution #localexecution #10423
Conversation
Skipping CI for Draft Pull Request. |
64d7384
to
18dca03
Compare
/assign @chensun |
1214612
to
cbedc32
Compare
/retest |
c925921
to
9810542
Compare
9810542
to
a20da3f
Compare
/retest |
1 similar comment
/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.
/lgtm
pipeline_spec=self.pipeline_spec, | ||
arguments=args, | ||
) | ||
elif self.component_spec is not 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.
The two branches possibly need to be merged, otherwise I suspect we may run into bugs for components with DagSpec.
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.
I think this LOC should not be hit for the case of a pipeline in a pipeline, since execute_locally
would be False
(passed in here). For condition and iteration DagSpec
s, there is no associated PipelineTask
so this LOC would also not be hit.
Is there another case you had in mind?
In my WIP implementation of pipeline-in-pipeline local execution, I've also not hit an issue in this location.
|
||
Returns: | ||
If DAG succeeds, a two-tuple of: (Status.SUCCESS, None). | ||
If DAG fails, a two-tuple of: (Status.FAILURE, '<task-that-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.
This interface implies the failed task must be from the the top level? A better option would be using a chain of names in case the task failed is nested in layers of dags.
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.
I think that makes sense. Since in the current PR this would only amount to wrapping this single element in a list, can we wait to implement this until the implementation of local pipelines-in-pipelines? I think that specific feature will make the motivation for such a data structure clear.
485e91b
to
1a8f2e3
Compare
1a8f2e3
to
c0be34c
Compare
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.
/lgtm
/approve
Thanks!
[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 |
@connor-mccarthy: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
* local pipeline implementation * address review feedback Signed-off-by: Alexey Roytman <roytman@il.ibm.com>
* local pipeline implementation * address review feedback
* local pipeline implementation * address review feedback
* local pipeline implementation * address review feedback
Description of your changes:
Enables running a pipeline locally:
Currently:
dsl.Condition
,dsl.ParallelFor
, anddsl.ExitHandler
) are not supported yet.dsl.importer
is not supported yet.Checklist: