Skip to content

Conversation

droctothorpe
Copy link
Collaborator

@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
Collaborator 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!

@droctothorpe
Copy link
Collaborator 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
Collaborator 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
Collaborator 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 <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>
Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>
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
Collaborator 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
Collaborator Author

/retest-required

@droctothorpe
Copy link
Collaborator 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 <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>
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
@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 <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>

* Address PR review 1

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>

* Refactor add_toleration to use Python primitives

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>

* Update go.mod to pull in latest kubernetes_platform package

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>

---------

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>
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 <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>

* Address PR review 1

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>

* Refactor add_toleration to use Python primitives

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>

* Update go.mod to pull in latest kubernetes_platform package

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>

---------

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>
VaniHaripriya pushed a commit to VaniHaripriya/data-science-pipelines that referenced this pull request Sep 23, 2024
… tolerations (kubeflow#10471)

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

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>

* Address PR review 1

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>

* Refactor add_toleration to use Python primitives

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>

* Update go.mod to pull in latest kubernetes_platform package

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>

---------

Signed-off-by: droctothorpe <[email protected]>
Co-authored-by: edmondop <[email protected]>
Co-authored-by: tarat44 <[email protected]>
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.

5 participants