KFP 1.4.0 Rebase #481

kfp-tekton-bot · 2021-02-25T19:11:51Z

Which issue is resolved by this Pull Request:
Resolves #469

Description of your changes:

Merging the changes from kubeflow/pipelines
1.3.0..1.4.0 into kfp-tekton:

TODO: resolve rejected merge conflicts.

Environment tested:

NOT tested

/cc @Tomcli
/cc @drewbutlerbb4

Rejected code changes that need to be resolved:

Please review/revise the following files which had/have merge conflicts. The *.rej files contain the chunks that Git (and I) could not resolve. Create a PR with the resolved changes on the kfp-tekton-bot fork and remove the *.rej files for each resolved file.

For reference, these are the changes in kubeflow/pipelines 1.3.0..1.4.0:

@Tomcli

@drewbutlerbb4

./frontend/src/lib/Utils.tsx.rej
./frontend/src/lib/StaticGraphParser.ts.rej
./frontend/src/pages/RunDetails.tsx.rej

@ckadner

./sdk/python/tests/compiler/testdata/withitem_nested.py.rej
./sdk/python/tests/compiler/testdata/withparam_global_dict.py.rej
./sdk/python/tests/compiler/testdata/parallelfor_item_argument_resolving.py.rej
./sdk/python/tests/compiler/testdata/loop_over_lightweight_output.py.rej
./sdk/python/tests/compiler/compiler_tests.py.rej
./CONTRIBUTING.md.rej

Resolves kubeflow#469

ckadner · 2021-02-25T19:44:14Z

@Tomcli -- I will still remove the samples coming from KFP so we only maintain the list of samples we created

Tomcli · 2021-02-26T00:27:29Z

Here is the PR for fixing api and sdk tests kfp-tekton-bot#4
@ckadner other than the UI conflicts, I see there's some rej files in the SDK and CONTRIBUTING.md. Are you going to handle those conflicts?

ckadner · 2021-02-26T01:18:59Z

@ckadner other than the UI conflicts, I see there's some rej files in the SDK and CONTRIBUTING.md. Are you going to handle those conflicts?

Yes. I was also working on the SDK testdata :-)

Tomcli · 2021-02-26T01:24:42Z

@ckadner other than the UI conflicts, I see there's some rej files in the SDK and CONTRIBUTING.md. Are you going to handle those conflicts?

Yes. I was also working on the SDK testdata :-)

I didn't check the compiler_tests.py so my changes on the testdata might be wrong. I updated the SDK testdata since i was testing the loop features and the generate yaml are getting inconsistent.

ckadner · 2021-02-26T01:33:27Z

I didn't check the compiler_tests.py so my changes on the testdata might be wrong. I updated the SDK testdata since i was testing the loop features and the generate yaml are getting inconsistent.

🤣 we might have to renegotiate the SDK testdata. I tried to reconcile the remaining changes from KFP 1.4.0 in commit 87658d5. Can you take a look?

$ git show --pretty="" --name-only 87658d5 | grep "\.py"
sdk/python/tests/compiler/testdata/loop_over_lightweight_output.py
sdk/python/tests/compiler/testdata/parallelfor_item_argument_resolving.py
sdk/python/tests/compiler/testdata/withitem_nested.py
sdk/python/tests/compiler/testdata/withparam_global_dict.py
sdk/python/tests/compiler/testdata/withparam_output.py
sdk/python/tests/compiler/testdata/withparam_output_dict.py

Tomcli · 2021-02-26T18:43:03Z

sdk/python/tests/compiler/testdata/parallelfor_item_argument_resolving.yaml

@@ -312,5 +312,13 @@ spec:
      taskRef:
        apiVersion: custom.tekton.dev/v1alpha1
        kind: PipelineLoop
-        name: for-loop-for-loop-00000003-3
+        name: for-loop-3


The generated pipeline-loop crd is still called for-loop-for-loop-00000003-3, so the name is not matching over here

I generated the YAML files using make unit_test GENERATE_GOLDEN_YAML=True

If I change the names in the YAML manually, the unittests fail.
`

I think the compiler test has to be updated, the loops crd get updated when i run TektonCompiler().compile(pipeline, __file__.replace('.py', '.yaml'))

Tomcli · 2021-02-26T18:59:35Z

sdk/python/tests/compiler/testdata/parallelfor_item_argument_resolving.py



 @func_to_container_op
 def produce_list_of_ints() -> list:
-    return (1234567890, 987654321)
+    return ([1234567890, 987654321],)


this new syntax with the [], is breaking our current loop feature. it's required for all loops now? maybe we need to get rid of it in our compiler to avoid this issue.

For now, let me revert these KFP 1.4.0 changes and create a separate issue. Though technically, if we claim KFP 1.4.0 alignment/support we may need those to work, no?

It seems like a limitation from the Tekton parameter API. Feng has to remove this [] in order to pass the list argument to Tekton. Maybe we should create an issue on this to post process in our compiler.

some of this work around is also apply to 1.3.0. We are still pending some Tekton api in order to run it like the Argo way as Tekton custom task has its own limitation. I can add some documents after this pr on what are the different between Tekton and Argo when running dynamic loops. Our previous loop implementation doesn't support dynamic loop so it's still an improvement.

Tomcli · 2021-02-26T19:05:04Z

sdk/python/tests/compiler/testdata/withitem_nested.py

@@ -30,28 +30,28 @@ def get_code(self, ):

 @dsl.pipeline(name='my-pipeline')
 def pipeline(my_pipe_param: int = 10):
-    loop_args = [1, 2]
+    loop_args = [{'a': 1, 'b': 2}, {'a': 10, 'b': 20}]


right now the loops tekton has only can take string or string Array argument. We cannot take dictionary array as it required us to change the tekton parameter api.

Okay, let me revert the KFP 1.4.0 changes in the affected test cases for now and create a separate issue to address those

Tomcli · 2021-02-26T19:13:52Z

@ckadner for the 1.4.0 sdk, the changes from echo to printf works fine. However, the new 1.4.0 test cases where they are wrapping all the loop arguments with [], is breaking our current implementation. If this is a new syntax requirement for Argo, then we need to support it in our pipeline loop controller.

ckadner · 2021-02-26T20:17:55Z

@ckadner for the 1.4.0 sdk, the changes from echo to printf works fine. However, the new 1.4.0 test cases where they are wrapping all the loop arguments with [], is breaking our current implementation. If this is a new syntax requirement for Argo, then we need to support it in our pipeline loop controller.

I think then we should keep the changed syntax in the testdata/*.py files (as they currently are in this PR) to reflect what is supported/mandated in KFP and quickly follow up with the changes required to make those work in a subsequent PR. Unless changing the pipeline loop controller can be done quickly and included in this PR #481

Tomcli · 2021-02-26T20:26:16Z

@ckadner for the 1.4.0 sdk, the changes from echo to printf works fine. However, the new 1.4.0 test cases where they are wrapping all the loop arguments with [], is breaking our current implementation. If this is a new syntax requirement for Argo, then we need to support it in our pipeline loop controller.

I think then we should keep the changed syntax in the testdata/*.py files (as they currently are in this PR) to reflect what is supported/mandated in KFP and quickly follow up with the changes required to make those work in a subsequent PR. Unless changing the pipeline loop controller can be done quickly and included in this PR #481

The testdata/*.py should be the working examples for how to use some of these features on kfp-tekton. Many of these limitations require a TED process in Tekton which it's taking us a while (from few weeks to months, as of now the Tekton team is still preparing for a TED). As the Argo loop syntax requires significant changes to the Tekton API, we have to make some workaround compromise in order to make it work in Tekton.

Tomcli · 2021-02-26T20:27:39Z

The report is where we can use the original kfp tests to verify what features we are supporting natively.

ckadner · 2021-02-26T21:04:18Z

The report is where we can use the original kfp tests to verify what features we are supporting natively.

Sure, but the sdk/python/tests/test_kfp_samples.sh will merely report whether or not the dsl-compile-tekton CLI succeeded to compile the KFP pipeline sample. That report does not verify anything in the generated Tekton YAML. Only our unittests verify the Tekton YAML

ckadner · 2021-02-26T21:06:55Z

@ckadner for the 1.4.0 sdk, the changes from echo to printf works fine. However, the new 1.4.0 test cases where they are wrapping all the loop arguments with [], is breaking our current implementation. If this is a new syntax requirement for Argo, then we need to support it in our pipeline loop controller.

I think then we should keep the changed syntax in the testdata/*.py files (as they currently are in this PR) to reflect what is supported/mandated in KFP and quickly follow up with the changes required to make those work in a subsequent PR. Unless changing the pipeline loop controller can be done quickly and included in this PR #481

The testdata/*.py should be the working examples for how to use some of these features on kfp-tekton. Many of these limitations require a TED process in Tekton which it's taking us a while (from few weeks to months, as of now the Tekton team is still preparing for a TED). As the Argo loop syntax requires significant changes to the Tekton API, we have to make some workaround compromise in order to make it work in Tekton.

Okay. That is a valid point. I will revert the two pipelines that use lists to wrap the loop arguments.

Tomcli · 2021-02-26T21:12:55Z

@ckadner for the 1.4.0 sdk, the changes from echo to printf works fine. However, the new 1.4.0 test cases where they are wrapping all the loop arguments with [], is breaking our current implementation. If this is a new syntax requirement for Argo, then we need to support it in our pipeline loop controller.

I think then we should keep the changed syntax in the testdata/*.py files (as they currently are in this PR) to reflect what is supported/mandated in KFP and quickly follow up with the changes required to make those work in a subsequent PR. Unless changing the pipeline loop controller can be done quickly and included in this PR #481

The testdata/*.py should be the working examples for how to use some of these features on kfp-tekton. Many of these limitations require a TED process in Tekton which it's taking us a while (from few weeks to months, as of now the Tekton team is still preparing for a TED). As the Argo loop syntax requires significant changes to the Tekton API, we have to make some workaround compromise in order to make it work in Tekton.

Okay. That is a valid point. I will revert the two pipelines that use lists to wrap the loop arguments.

I created an issue and hopefully some of these can address in the compiler. #482

ckadner · 2021-02-26T23:06:00Z

@Tomcli -- a7e5378 has reverted the list-wrapping of the pipeline-loop arguments and regenerated pipelineloop CRDs along with some other minor fixes.

We need to create another issue to make sure the pipeline-loop CRD YAML files get regenerated along with the "Golden" YAML files for the pipelines. For now I did it this way:

for f in sdk/python/tests/compiler/testdata/*_pipelineloop_cr*.yaml; do
    echo ${f/_pipelineloop_cr*.yaml/.py}
done | sort -u | while read f; do
    echo $f; dsl-compile-tekton --py $f --output ${f/.py/.yaml}
done

make unit_test GENERATE_GOLDEN_YAML=True

Tomcli · 2021-02-27T01:42:51Z

thanks @ckadner overall it looks good to me. I will run the e2e test to test the 1.4.0 sdk and will merge after that.

Tomcli · 2021-03-01T17:59:27Z

/lgtm

Tomcli · 2021-03-01T18:19:27Z

/approve

k8s-ci-robot · 2021-03-01T18:19:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kfp-tekton-bot, Tomcli

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:

~~OWNERS~~ [Tomcli]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

KFP 1.4.0 Rebase

9788897

Resolves kubeflow#469

k8s-ci-robot requested review from drewbutlerbb4 and Tomcli February 25, 2021 19:11

k8s-ci-robot added do-not-merge/work-in-progress size/XXL labels Feb 25, 2021

Remove duplicated KFP samples

1336dd5

Resolve api and backend and SDK test conflicts

ea88472

kfp-tekton-bot force-pushed the kfp_1.4.0_rebase branch from ce8dad5 to ea88472 Compare February 26, 2021 01:07

kfp-tekton-bot added 2 commits February 25, 2021 17:25

Upgrade kfp requirements to 1.4.0

c9309fa

Update testdata regenerate YAML files

87658d5

kfp-tekton-bot added 3 commits February 25, 2021 18:17

Resolve CONTRIBUTING.md

4edfbe6

Regenerate KFP samples compile report

e75e11e

Update README.md files for KFP version 1.4.0

0cb62dd

ckadner mentioned this pull request Feb 26, 2021

Add 0.7.0 release template #479

Merged

2 tasks

Tomcli reviewed Feb 26, 2021

View reviewed changes

Resolve frontend conflicts

d7515a8

kfp-tekton-bot force-pushed the kfp_1.4.0_rebase branch from 9a08552 to d7515a8 Compare February 26, 2021 20:24

Revert list-wrapped loop arguments

a7e5378

Remove compiler_tests.py.rej

fbc8387

kubeflow deleted a comment from k8s-ci-robot Feb 26, 2021

kubeflow deleted a comment from review-notebook-app bot Feb 26, 2021

ckadner changed the title ~~[WIP] KFP 1.4.0 Rebase~~ KFP 1.4.0 Rebase Feb 27, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress label Feb 27, 2021

k8s-ci-robot assigned Tomcli Mar 1, 2021

k8s-ci-robot added the lgtm label Mar 1, 2021

k8s-ci-robot added the approved label Mar 1, 2021

k8s-ci-robot merged commit ede6750 into kubeflow:master Mar 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KFP 1.4.0 Rebase #481

KFP 1.4.0 Rebase #481

kfp-tekton-bot commented Feb 25, 2021 •

edited by ckadner

Loading

ckadner commented Feb 25, 2021

Tomcli commented Feb 26, 2021

ckadner commented Feb 26, 2021

Tomcli commented Feb 26, 2021

ckadner commented Feb 26, 2021 •

edited

Loading

Tomcli Feb 26, 2021

ckadner Feb 26, 2021

Tomcli Feb 26, 2021

Tomcli Feb 26, 2021

ckadner Feb 26, 2021

Tomcli Feb 26, 2021

Tomcli Feb 26, 2021

Tomcli Feb 26, 2021

ckadner Feb 26, 2021

Tomcli commented Feb 26, 2021

ckadner commented Feb 26, 2021 •

edited

Loading

Tomcli commented Feb 26, 2021

Tomcli commented Feb 26, 2021 •

edited

Loading

ckadner commented Feb 26, 2021

ckadner commented Feb 26, 2021

Tomcli commented Feb 26, 2021

ckadner commented Feb 26, 2021 •

edited

Loading

Tomcli commented Feb 27, 2021

Tomcli commented Mar 1, 2021

Tomcli commented Mar 1, 2021

k8s-ci-robot commented Mar 1, 2021

KFP 1.4.0 Rebase #481

KFP 1.4.0 Rebase #481

Conversation

kfp-tekton-bot commented Feb 25, 2021 • edited by ckadner Loading

ckadner commented Feb 25, 2021

Tomcli commented Feb 26, 2021

ckadner commented Feb 26, 2021

Tomcli commented Feb 26, 2021

ckadner commented Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tomcli commented Feb 26, 2021

ckadner commented Feb 26, 2021 • edited Loading

Tomcli commented Feb 26, 2021

Tomcli commented Feb 26, 2021 • edited Loading

ckadner commented Feb 26, 2021

ckadner commented Feb 26, 2021

Tomcli commented Feb 26, 2021

ckadner commented Feb 26, 2021 • edited Loading

Tomcli commented Feb 27, 2021

Tomcli commented Mar 1, 2021

Tomcli commented Mar 1, 2021

k8s-ci-robot commented Mar 1, 2021

kfp-tekton-bot commented Feb 25, 2021 •

edited by ckadner

Loading

ckadner commented Feb 26, 2021 •

edited

Loading

ckadner commented Feb 26, 2021 •

edited

Loading

Tomcli commented Feb 26, 2021 •

edited

Loading

ckadner commented Feb 26, 2021 •

edited

Loading