Fix Watson pipeline example #1246
Conversation
|
Hi @Tomcli. 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. |
1 similar comment
|
Hi @Tomcli. 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. |
|
/assign @animeshsingh |
| runtime=runtime, | ||
| runtime_version=runtime_version, | ||
| run_definition=run_definition, | ||
| run_name=run_name |
There was a problem hiding this comment.
since we can just go with the variable values, maybe we can remove duplication here
There was a problem hiding this comment.
I removed duplications on the last 2 components since they have few arguments. For the train component, I decided to keep using keyword arguments since it has a long list of args and users might mistakenly put them in the wrong order.
There was a problem hiding this comment.
My experience is that this makes it less error-prone and also makes it easier for users.
Without explicit names, if you add a new parameter, the calls may become invalid since the wrong arguments will be passed.
Also this makes it easier since the users see the parameter names which can be different from argument names, especially in cases where arguments are constant.
There was a problem hiding this comment.
Yeah we discussed internally extensively, specially if there are large number of params. Maybe it would behoove to make it a standard, specially on our pipelines. i am not a fan of mix and match in general
|
/lgtm |
|
/retest |
|
/retest |
|
@Tomcli: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
1 similar comment
|
@Tomcli: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
|
/lgtm |
|
/ok-to-test |
|
@Ark-kun any reason the test is failing? Don`t think it has anything to do with us? |
|
/test kubeflow-pipeline-e2e-test |
|
Don`t think its us - so this can be merged I believe @Ark-kun I see the following error message
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: animeshsingh 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: animeshsingh 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-pipeline-e2e-test |
3 similar comments
|
/test kubeflow-pipeline-e2e-test |
|
/test kubeflow-pipeline-e2e-test |
|
/test kubeflow-pipeline-e2e-test |
|
@Ark-kun is it peculiar to this PR? |
|
Saw this error now ++ curl -LO https://github.com/ksonnet/ksonnet/releases/download/v0.13.0/ks_0.13.0_linux_amd64.tar.gz 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 0 0 0 0 0 0 0 0 --:--:-- 0:00:01 --:--:-- 0 gzip: stdin: not in gzip format |
|
/test kubeflow-pipeline-e2e-test |
|
I had the same problem this morning local with the 0.13.0 version. The download link for version 0.13.1 is working. If you change the version here it should work. |
No |
|
/test kubeflow-pipeline-e2e-test |
|
/test kubeflow-pipeline-sample-test |
This PR contains the following fixes:
kubectl deletewhen old secret is not present.metadata.labelssince Argo can't take labels with white space.createdtext. Also parameterize author email field.This change is