support tolerations for ContainerOps #1269

hamedhsn · 2019-05-01T14:59:02Z

This change is

k8s-ci-robot · 2019-05-01T14:59:19Z

Hi @hamedhsn. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

k8s-ci-robot · 2019-05-01T14:59:19Z

Hi @hamedhsn. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

hongye-sun · 2019-05-01T16:24:30Z

sdk/python/kfp/dsl/_container_op.py

+
+        Args:
+          volume: Kubernetes toleration
+          For detailed spec, check volume definition


You probably need to rename volume to tolerations

hongye-sun · 2019-05-01T16:26:47Z

sdk/python/kfp/dsl/_container_op.py

@@ -745,6 +746,17 @@ def add_volume(self, volume):
        self.volumes.append(volume)
        return self

+    def add_toleration(self, tolerations):


Could you add a basic unit test to cover the method: calling the method and checking the output yaml contains the correct tolerations config? This will help us to make sure future changes won't break existing feature.

sure will do.

You'll probably need to test the op_to_template method for this.

hongye-sun · 2019-05-01T16:39:54Z

/ok-to-test

sdk/python/kfp/dsl/_container_op.py

Ark-kun · 2019-05-01T22:13:38Z

sdk/python/kfp/dsl/_container_op.py

@@ -745,6 +746,17 @@ def add_volume(self, volume):
        self.volumes.append(volume)
        return self

+    def add_toleration(self, tolerations):


You should probably specify the type of tolerations

Right now it's not apparent to the user what kind of argument they need to pass here.

gaoning777 · 2019-05-03T19:38:33Z

Thanks for your contributions.
Please see the comments above and address them. I'll make sure the PR is merged once they are addressed.

googlebot · 2019-05-05T23:16:49Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-05T23:18:45Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Ark-kun · 2019-05-06T19:50:49Z

As you probably see from the test failures, the golden compiled Argo YAML is wrong. At the very least it lacks the root DAG.

hamedhsn · 2019-05-06T20:51:17Z

@Ark-kun hmm yeah saw that, is that the right way to test the function?

Ark-kun · 2019-05-06T21:37:21Z

@Ark-kun hmm yeah saw that, is that the right way to test the function?

Maybe just call kfp.compiler.Compiler()._op_to_template(ContainerOp(...).add_toleration(bla)) and check the resulting dictionary for correct keys and values.

hamedhsn · 2019-05-09T15:21:18Z

@Ark-kun sounds like tests are passes, can you review pls

sdk/python/tests/compiler/compiler_tests.py

gaoning777 · 2019-05-09T20:54:00Z

It looks good to me.
\lgtm
@Ark-kun I will let Alexey to send the approve once he thinks his concerns are addressed.

@hamedhsn thanks for quick turnaround.

gaoning777 · 2019-05-09T20:54:22Z

/lgtm

gaoning777 · 2019-05-09T20:57:20Z

/lgtm

Ark-kun · 2019-05-09T22:08:59Z

/approve

k8s-ci-robot · 2019-05-09T22:09:10Z

[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

hamedhsn · 2019-05-10T09:22:42Z

@Ark-kun @gaoning777 thanks guys :)

…kubeflow#1269)

k8s-ci-robot requested review from gaoning777, hongye-sun and Ark-kun May 1, 2019 14:59

k8s-ci-robot added size/S needs-ok-to-test labels May 1, 2019

hongye-sun reviewed May 1, 2019

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels May 1, 2019

Ark-kun reviewed May 1, 2019

View reviewed changes

sdk/python/kfp/dsl/_container_op.py Show resolved Hide resolved

Ark-kun reviewed May 1, 2019

View reviewed changes

gaoning777 self-assigned this May 3, 2019

Ark-kun self-assigned this May 3, 2019

k8s-ci-robot added size/XXL and removed size/S labels May 5, 2019

hamedhsn force-pushed the add_tolerations branch from 8a2632f to ad36487 Compare May 5, 2019 23:18

k8s-ci-robot added size/L and removed size/XXL labels May 5, 2019

hamedhsn force-pushed the add_tolerations branch from 5211cba to a90d5a5 Compare May 7, 2019 16:42

gaoning777 reviewed May 9, 2019

View reviewed changes

sdk/python/tests/compiler/compiler_tests.py Outdated Show resolved Hide resolved

hamedhsn added 5 commits May 9, 2019 18:59

add tolerations to ContainerOps

7e502a1

add test

176f484

add type for tolerations

78c8ce3

remove fix

c7a5a96

remove print

a0b04b9

hamedhsn force-pushed the add_tolerations branch from a90d5a5 to a0b04b9 Compare May 9, 2019 18:07

k8s-ci-robot added lgtm labels May 9, 2019

k8s-ci-robot added the approved label May 9, 2019

k8s-ci-robot merged commit ce60661 into kubeflow:master May 9, 2019

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023

Fix: Use istio's grafana and prometheus addons for torchserve metrics (…

fe53e68

…kubeflow#1269)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support tolerations for ContainerOps #1269

support tolerations for ContainerOps #1269

hamedhsn commented May 1, 2019 •

edited by jlewi

Loading

k8s-ci-robot commented May 1, 2019

k8s-ci-robot commented May 1, 2019

hongye-sun May 1, 2019

hamedhsn May 5, 2019

hongye-sun May 1, 2019

hamedhsn May 1, 2019

Ark-kun May 1, 2019

hongye-sun commented May 1, 2019

Ark-kun May 1, 2019

Ark-kun May 3, 2019

hamedhsn May 5, 2019

gaoning777 commented May 3, 2019

googlebot commented May 5, 2019

googlebot commented May 5, 2019

Ark-kun commented May 6, 2019

hamedhsn commented May 6, 2019

Ark-kun commented May 6, 2019

hamedhsn commented May 9, 2019 •

edited

Loading

gaoning777 commented May 9, 2019

gaoning777 commented May 9, 2019 •

edited

Loading

gaoning777 commented May 9, 2019

Ark-kun commented May 9, 2019

k8s-ci-robot commented May 9, 2019

hamedhsn commented May 10, 2019

support tolerations for ContainerOps #1269

support tolerations for ContainerOps #1269

Conversation

hamedhsn commented May 1, 2019 • edited by jlewi Loading

k8s-ci-robot commented May 1, 2019

k8s-ci-robot commented May 1, 2019

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

hongye-sun commented May 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaoning777 commented May 3, 2019

googlebot commented May 5, 2019

googlebot commented May 5, 2019

Ark-kun commented May 6, 2019

hamedhsn commented May 6, 2019

Ark-kun commented May 6, 2019

hamedhsn commented May 9, 2019 • edited Loading

gaoning777 commented May 9, 2019

gaoning777 commented May 9, 2019 • edited Loading

gaoning777 commented May 9, 2019

Ark-kun commented May 9, 2019

k8s-ci-robot commented May 9, 2019

hamedhsn commented May 10, 2019

hamedhsn commented May 1, 2019 •

edited by jlewi

Loading

hamedhsn commented May 9, 2019 •

edited

Loading

gaoning777 commented May 9, 2019 •

edited

Loading