SDK/Compiler: Add add_pvolumes() method to ContainerOp

[elikatsis]

Following this discussion.

/cc @jinchihe

---

This change is 

[elikatsis]

/ok-to-test

[jinchihe]

Tested on-prem cluster with the add_pvolumes() method, that works fine for on-prem cases:

[root@jche1 ~]# argo list tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp
NAME                                                              STATUS      AGE   DURATION   PRIORITY
tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp   Succeeded   14m   7m         0
[root@jche1 ~]# argo get tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp

.....

STEP                                                                PODNAME                                                                     DURATION  MESSAGE
 ✔ tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp                                                                                        
 ├-✔ create-pvc                                                     tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp-4218758567  3s        
 ├-✔ checkout                                                       tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp-1053616485  2m        
 ├-✔ tfx-data-validation                                            tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp-3015059516  39s       
 ├-✔ transform-using-tf-on-dataflow                                 tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp-3048657474  51s       
 ├-✔ train-fc-dnn-using-tf                                          tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp-2639066820  2m        
 ├-✔ kubeflow-serve-tf-model                                        tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp-1317934331  40s       
 ├-✔ predict-using-tf-on-dataflow                                   tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp-1844614545  14s       
 ├-✔ tfx-analyze-model                                              tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp-133088148   27s       
 ├-✔ confusion-matrix                                               tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp-1162815283  5s        
 └-✔ roc-curve                                                      tfx-taxi-cab-classification-pipeline-example-for-on-prem-cdrccp-2939236461  5s        

[jinchihe]

/lgtm

[jinchihe]

Seems the method add_pvolumes has similar function with onprem.mount_pvc?

[elikatsis]

Seems the method add_pvolumes has similar function with onprem.mount_pvc?

In the end yes, they both run add_volume() and add_volume mount().
But add_pvolumes() also updates the pvolumes dict accordingly, making available the functionality of referencing a volume as step.pvolumes["/common"].

[elikatsis]

Oh! It also handles the dependencies carried by PipelineVolumes, of course.

[jinchihe]

Oh! It also handles the dependencies carried by PipelineVolumes, of course.

Oh, you are right. By the way, seems each following steps depend on the create_pvc component, that means there is a line to connnect from create_pvc to each following step, It looks a little messy, Is that possiable to cut down to just connect first related step? Just my personal thinking, not problems.

[elikatsis]

It does look weird, yes.
However, these dependencies exist because all of the steps use the create_pvc's output parameter, and are automatically derived by the compiler's analysis.

I think it would be nice to not show redundant dependencies, but that would be misleading in a way. In such occasions the compiler finds out dependencies some of which are not very important to be shown, while others are. How would such distinction be made?

[elikatsis]

/assign @Ark-kun

[elikatsis]

@Ark-kun friendly ping.

As you mention in the first part of your comment here, it seems reasonable to expose such functionality.

Could you take a look in this PR?
Thanks!

[Ark-kun]

As you mention in the first part of your comment here, it seems reasonable to expose such functionality.

The functionality I've mentioned (op.add_volume_mount) already exists...

BTW, with this PR in place, do you still need the pvolumes parameter in the ContainerOp constructor?

/lgtm

@gaoning777 @hongye-sun WDYT?

[elikatsis]

The functionality I've mentioned (op.add_volume_mount) already exists...

I meant, according to the logic behind your comment, more of that functionalities should be available, and that is one of them.

[...] do you still need the pvolumes parameter in the ContainerOp constructor?

Yes, I think both are good to have.

[Ark-kun]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

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:

• sdk/OWNERS [Ark-kun]

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

[Ark-kun]

/test kubeflow-pipeline-e2e-test

[elikatsis]

/test kubeflow-pipeline-e2e-test

[elikatsis]

There seem to be some problems with the integration tests.
E.g., now the build-frontend-integration-tests-image test failed, but I think some other time it was the run-frontend-integration-tests that had failed.

Is it an infra issue? I guess it couldn't be broken by some PR (since that would have failed as well)

[Ark-kun]

/test kubeflow-pipeline-e2e-test