-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(backend): parallelFor resolve upstream inputs. Fixes #11520 #11627
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
Conversation
Hi @zazulam. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
/retest |
Adding some details here related to The example used was from the comment and also added as the test case in the from kfp import dsl
@dsl.component
def print_op(message: str) -> str:
print(message)
return message
@dsl.component
def reduce_op(message: str) -> str:
print(message)
return message[0]
@dsl.pipeline()
def my_pipeline():
with dsl.ParallelFor([1, 2, 3]):
one = print_op(message='foo')
two = print_op(message='bar').after(one) |
3adfc00
to
bb48a9a
Compare
/lgtm Thanks for the quick turn around on this folks! |
@zazulam can you rebase? there are conflicts |
Signed-off-by: zazulam <[email protected]>
bb48a9a
to
c8a49fc
Compare
I'm going to save the |
@zazulam I think separate pr makes sense, if we can keep this one light weight I might be able to cherry pick this for the 2.4.1 patch release I'll make next week so we can address the regression for kubeflow 1.10. Feel free to hit me up on slack once the pr is ready, or if you get hit with flaky tests. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: droctothorpe, HumairAK 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 |
/lgtm |
… (kubeflow#11627) Signed-off-by: zazulam <[email protected]>
… (kubeflow#11627) (cherry picked from commit f7c0616) Signed-off-by: zazulam <[email protected]>
… (kubeflow#11627) (cherry picked from commit f7c0616) Signed-off-by: zazulam <[email protected]> Signed-off-by: Humair Khan <[email protected]>
* chore: Remove License checker (#11609) * update dependencies Signed-off-by: Humair Khan <[email protected]> * remove license checking Signed-off-by: Humair Khan <[email protected]> --------- (cherry picked from commit c100648) Signed-off-by: Humair Khan <[email protected]> * fix(backend): Replaced hardcoded ServiceAccount with default config (#11578) (cherry picked from commit 18641e1) Signed-off-by: Helber Belmiro <[email protected]> Signed-off-by: Humair Khan <[email protected]> * fix(backend) fix run retry for argo (#11585) (cherry picked from commit b131566) Signed-off-by: arpechenin <[email protected]> Signed-off-by: Humair Khan <[email protected]> * fix(backend): parallelFor resolve upstream inputs. Fixes #11520 (#11627) (cherry picked from commit f7c0616) Signed-off-by: zazulam <[email protected]> Signed-off-by: Humair Khan <[email protected]> * fix(backend): the metacontroller is broken since #11474 (#11608) * Update cluster-role-binding.yaml Signed-off-by: Julius von Kohout <[email protected]> * Create cluster-role.yaml Signed-off-by: juliusvonkohout <[email protected]> * Update kustomization.yaml Signed-off-by: juliusvonkohout <[email protected]> * Update stateful-set.yaml Signed-off-by: juliusvonkohout <[email protected]> --------- (cherry picked from commit a40163f) Signed-off-by: Julius von Kohout <[email protected]> Signed-off-by: juliusvonkohout <[email protected]> Signed-off-by: Humair Khan <[email protected]> * fix(manifests): Upgrading metacontroller to v4.11.22 (#11656) (cherry picked from commit ebaaf75) Signed-off-by: Tarek Abouzeid <[email protected]> Signed-off-by: Humair Khan <[email protected]> * fix(backend): ignore unknown fields for pb json unmarshaling (#11662) (cherry picked from commit 9afe23e) Signed-off-by: Humair Khan <[email protected]> * chore: improved securitycontext for mysql (#11678) * Update mysql-deployment.yaml Signed-off-by: Julius von Kohout <[email protected]> * Update mysql-deployment.yaml Signed-off-by: Julius von Kohout <[email protected]> * Update mysql-deployment.yaml Signed-off-by: Julius von Kohout <[email protected]> --------- (cherry picked from commit 78675b0) Signed-off-by: Julius von Kohout <[email protected]> Signed-off-by: Humair Khan <[email protected]> * chore: partially revert 9afe23e (#11713) In 9afe23e we introduced blackend dropping of unknown fields for unmarshalling, but going forward we want to handle this more on a case by case basis. In the case for driver we should drop them because by this point the api server has declare the pipeline spec is acceptable, so the driver should not fail here. As such we keep driver changes, but revert those utilized by the api server. (cherry picked from commit 13b8194) Signed-off-by: Humair Khan <[email protected]> * fix(backend) fix execution-level retry on the Argo Workflows backend (#11673) (cherry picked from commit 30210e3) Signed-off-by: ntny <[email protected]> Signed-off-by: arpechenin <[email protected]> Co-authored-by: arpechenin <[email protected]> Signed-off-by: Humair Khan <[email protected]> * Limit the number of parallel tests in SDK execution tests (#11680) Often times, pods could not be scheduled because of insufficient CPU and the worker would run out of disk space. Signed-off-by: mprahl <[email protected]> Signed-off-by: Humair Khan <[email protected]> * correct kfp deploy images Signed-off-by: Humair Khan <[email protected]> --------- Signed-off-by: Humair Khan <[email protected]> Signed-off-by: Helber Belmiro <[email protected]> Signed-off-by: arpechenin <[email protected]> Signed-off-by: zazulam <[email protected]> Signed-off-by: Julius von Kohout <[email protected]> Signed-off-by: juliusvonkohout <[email protected]> Signed-off-by: Tarek Abouzeid <[email protected]> Signed-off-by: ntny <[email protected]> Signed-off-by: mprahl <[email protected]> Co-authored-by: Helber Belmiro <[email protected]> Co-authored-by: Anton Pechenin <[email protected]> Co-authored-by: Michael <[email protected]> Co-authored-by: Julius von Kohout <[email protected]> Co-authored-by: Tarek Abouzeid <[email protected]> Co-authored-by: arpechenin <[email protected]> Co-authored-by: Matt Prahl <[email protected]>
Description of your changes:
Updated
resolveUpstreamArtifacts
to usegetDAGTasks
to allow for it parse through parallelFor DAG contexts to retrieve the appropriate producerTask. This fixes [backend] dsl.ParallelFor loop: cannot resolve the upstream artifact output of a previous pod #11520, it seems that the call with the filter toGetExecutionsInDAG
was not reverted back to the same as it is inresolveUpstreamParameters
in feat(backend): implement subdag output resolution #11196.Updated the argocompiler for the
iteratorTask
section. The additional "-loop" added to the task associated with the parallelFor DAG breaks the dependency validation that the argoworkflows API calls in the submission of the pipeline. This partially resolves [backend/sdk] Support dsl.collected() in KFP #10050 for the.after()
usage of a parallelTask. The implementation fordsl.Collected
in the backend will be coming in a follow-up PR.cc: @droctothorpe
Checklist: