feat(sdk): Add local runner which will run ops in docker or locally. Fixes #1138 #4983
Conversation
|
Hi @lynnmatrix. 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. |
|
Thank you for this tremendous work. |
|
@Ark-kun This feature requires docker in CI environment, need your help. |
|
/assign @chensun @numerology |
|
Should I remove the dependency to docker in unit test? |
I think the more sustainable way is actually declaring extra_requires = {
'test': ['docker']}and then Or you can simply add |
|
Do you remember what's the recommended way of using |
|
/retest |
|
It's very challenging to use docker within current presubmit process. Let me take another look at this PR soon. |
|
Yes, I tried to install docker in presubmit process which run in k8s container, but failed. I have no experience in dind.
|
|
/test kubeflow-pipelines-tfx-python36 |
Our sample tests use dind: pipelines/test/sample_test.yaml Line 113 in d14f64e It's probably fine to skip that in the initial PR. You can make another PR where you add the docker tests to sample tests. |
|
Thank you for you great contribution |
|
/hold |
|
I approve this PR, but I'd like let Ajay have a look. |
|
/retest |
There was a problem hiding this comment.
Many thanks for this @lynnmatrix! This looks great! Left some comments/discussion points in the PR.
|
Thank you for review, Ajay. Good points. |
|
New changes are detected. LGTM label has been removed. |
|
@neuromage Thanks for your review from which I have learned much. |
| return kfp.dsl.ContainerOp( | ||
| name="echo", | ||
| image=BASE_IMAGE, | ||
| command=["echo", "hello world", ">", "/tmp/outputs/output_file"], |
There was a problem hiding this comment.
Nice, thanks for adding this test!
|
/lgtm Many thanks @lynnmatrix! Really nice PR. This has been a much requested feature and will go a long way towards helping users develop/debug locally. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ark-kun, lynnmatrix, neuromage 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: Ark-kun, lynnmatrix, neuromage 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 |
Description of your changes:
Add local runner which will run ops in docker or locally.
kfp.run_pipeline_func_locally(my_pipeline, arguments={...})Fixes #1138