Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Backend + SDK): Update kfp backend and kubernetes sdk to support tolerations #10471

Merged
merged 4 commits into from
Feb 22, 2024

Conversation

droctothorpe
Copy link
Contributor

@droctothorpe droctothorpe commented Feb 12, 2024

Description of your changes:
This PR adds support for tolerations to the driver and kubernetes_platform SDK.

It is a follow up to #10442, which updated the protobuf and Golang stub.

It follows the excellent example set by #10427.

Huge thanks to @Tomcli for paving the way and sharing how to validate things end to end.

Checklist:

Copy link

Hi @droctothorpe. 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.

@droctothorpe
Copy link
Contributor Author

Rebased and resolved merge conflicts.

Copy link
Member

@connor-mccarthy connor-mccarthy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @droctothorpe!

kubernetes_platform/python/kfp/kubernetes/__init__.py Outdated Show resolved Hide resolved
kubernetes_platform/python/kfp/kubernetes/toleration.py Outdated Show resolved Hide resolved
kubernetes_platform/python/kfp/kubernetes/toleration.py Outdated Show resolved Hide resolved
kubernetes_platform/proto/kubernetes_executor_config.proto Outdated Show resolved Hide resolved
kubernetes_platform/python/kfp/kubernetes/toleration.py Outdated Show resolved Hide resolved
@droctothorpe
Copy link
Contributor Author

@connor-mccarthy We addressed all feedback. We put it in a discrete commit to make review easier, but happy to squash if that's helpful. Thank you! 🙏

@Tomcli
Copy link
Member

Tomcli commented Feb 14, 2024

/ok-to-test

@Tomcli
Copy link
Member

Tomcli commented Feb 14, 2024

fyi @droctothorpe your tests will be failing because the backend is pulling the kubernetes_platform package from github upstream instead of local packages. This is why the PRs our team created have to at least separate into two PRs.

@droctothorpe
Copy link
Contributor Author

fyi @droctothorpe your tests will be failing because the backend is pulling the kubernetes_platform package from github upstream instead of local packages. This is why the PRs our team created have to at least separate into two PRs.

Doh. That makes sense. I'll rebase and resolve merge conflicts in the other PR this evening. Thanks, @Tomcli!

@droctothorpe
Copy link
Contributor Author

@Tomcli I rebased and re-opened #10442. It should be good to go. Does it make sense to add the kubernetes_platform package to the replace directive either in general or for CI specifically? That would negate the need to submit these kinds of changes in discrete PRs. Just a thought.

droctothorpe and others added 2 commits February 20, 2024 17:42
Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>
Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>
Copy link
Member

@connor-mccarthy connor-mccarthy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

Thank you, @droctothorpe!

@droctothorpe
Copy link
Contributor Author

droctothorpe commented Feb 20, 2024

/lgtm /approve

Thank you, @droctothorpe!

Still trying to debug why the license checks are failing in CI.

@droctothorpe droctothorpe force-pushed the tolerations2 branch 2 times, most recently from a6ba900 to 6819b40 Compare February 21, 2024 00:40
@droctothorpe
Copy link
Contributor Author

/retest-required

@droctothorpe
Copy link
Contributor Author

droctothorpe commented Feb 21, 2024

All required tests are finally passing. Thanks for your patience! CC @connor-mccarthy. Just gonna squash the last few commits to get past the DCO gate.

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>
Copy link

@droctothorpe: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
kubeflow-pipelines-samples-v2 dda1ec3 link false /test kubeflow-pipelines-samples-v2
kfp-kubernetes-execution-tests dda1ec3 link false /test kfp-kubernetes-execution-tests

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. I understand the commands that are listed here.

Copy link
Member

@Tomcli Tomcli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @droctothorpe
/lgtm

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun, connor-mccarthy

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 2983a7d into kubeflow:master Feb 22, 2024
12 of 14 checks passed
@droctothorpe droctothorpe deleted the tolerations2 branch February 22, 2024 22:04
petethegreat pushed a commit to petethegreat/pipelines that referenced this pull request Mar 27, 2024
… tolerations (kubeflow#10471)

* feat(Backend + SDK): Update kfp backend and kubernetes sdk to support tolerations

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>

* Address PR review 1

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>

* Refactor add_toleration to use Python primitives

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>

* Update go.mod to pull in latest kubernetes_platform package

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>

---------

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>
rimolive pushed a commit to rimolive/data-science-pipelines that referenced this pull request Mar 28, 2024
… tolerations (kubeflow#10471)

* feat(Backend + SDK): Update kfp backend and kubernetes sdk to support tolerations

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>

* Address PR review 1

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>

* Refactor add_toleration to use Python primitives

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>

* Update go.mod to pull in latest kubernetes_platform package

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>

---------

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>
petethegreat pushed a commit to petethegreat/pipelines that referenced this pull request Mar 29, 2024
… tolerations (kubeflow#10471)

* feat(Backend + SDK): Update kfp backend and kubernetes sdk to support tolerations

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>

* Address PR review 1

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>

* Refactor add_toleration to use Python primitives

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>

* Update go.mod to pull in latest kubernetes_platform package

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>

---------

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: edmondop <edmondo.porcu@gmail.com>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants