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.

Sign up for GitHub

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

Jump to bottom

feat(kubernetes_platform): Update kubernetes_platform go package to i… #10442

Merged
merged 1 commit into from
Feb 16, 2024
Merged

feat(kubernetes_platform): Update kubernetes_platform go package to i… #10442

merged 1 commit into from
Feb 16, 2024

Conversation

droctothorpe
Copy link
Contributor

…nclude tolerations

Description of your changes:
This PR adds tolerations to the kubernetes platform proto file. It also regenerates the Golang stub.

It follows the piece-meal approach that @Tomcli led by example with in this PR and clarified in this comment.

We will submit a follow up PR that updates driver.go and another PR that updates the SDK.

The toleration message mimics the message in the core K8s tolerations proto.

This will let us use V2 with GPUs.

Relevant issues: #9682, #9768, and #10252.

Checklist:

@google-cla Google CLA
Copy link

google-cla bot commented Jan 31, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@google-oss-prow Google OSS Prow
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.

@chensun
Copy link
Member

chensun commented Feb 1, 2024

/ok-to-test

Tomcli
Tomcli suggested changes Feb 7, 2024
View reviewed changes
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, overall the new definition looks good to me other than a minor format fix. Can you rebase this PR to the master branch and regenerate the go package? Thank you.

kubernetes_platform/proto/kubernetes_executor_config.proto Outdated Show resolved Hide resolved
@droctothorpe droctothorpe force-pushed the tolerations branch 2 times, most recently from 126d4f7 to addf158 Compare February 8, 2024 16:15
Tomcli
Tomcli approved these changes Feb 8, 2024
View reviewed changes
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

@google-oss-prow google-oss-prow bot added the lgtm label Feb 8, 2024
@Tomcli
Copy link
Member

Tomcli commented Feb 8, 2024

/assign @connor-mccarthy

@google-oss-prow google-oss-prow bot removed the lgtm label Feb 11, 2024
@droctothorpe
Copy link
Contributor Author

Rebased and resolved conflicts.

Tomcli
Tomcli reviewed Feb 12, 2024
View reviewed changes
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.

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Feb 12, 2024
@droctothorpe droctothorpe mentioned this pull request Feb 12, 2024
Merged
1 task
@Tomcli
Copy link
Member

Tomcli commented Feb 14, 2024

@droctothorpe can you rebase this PR? I think you need this to merge first because the backend is pulling the package from github upstream instead of local packages.

/cc @connor-mccarthy

@droctothorpe
Copy link
Contributor Author

I'm going to close this PR in favor of #10471, which is a superset of this one (it includes the driver and SDK changes as well).

@droctothorpe droctothorpe reopened this Feb 14, 2024
@google-oss-prow google-oss-prow bot removed the lgtm label Feb 14, 2024
Tomcli
Tomcli approved these changes Feb 15, 2024
View reviewed changes
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.

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Feb 15, 2024
@droctothorpe
Copy link
Contributor Author

Need to rebase again after the last merge to master but won't have a chance to until this evening (when I have access to my personal laptop). Apologies for the delay.

@droctothorpe @edmondop @tarat44
feat(kubernetes_platform): Update kubernetes_platform go package to i…
4b39e61 
…nclude 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>
@google-oss-prow google-oss-prow bot removed the lgtm label Feb 16, 2024
@droctothorpe
Copy link
Contributor Author

Rebase complete. Thanks for your patience! @Tomcli.

@Tomcli
Copy link
Member

Tomcli commented Feb 16, 2024

thanks @droctothorpe,
/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Feb 16, 2024
@Tomcli
Copy link
Member

Tomcli commented Feb 16, 2024

/cc @connor-mccarthy

@google-oss-prow Google OSS Prow
Copy link

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

Test name Commit Details Required Rerun command
kfp-kubernetes-execution-tests 4b39e61 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.

connor-mccarthy
connor-mccarthy approved these changes Feb 16, 2024
View reviewed changes
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

@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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 6fb997a into kubeflow:master Feb 16, 2024
9 of 10 checks passed
@droctothorpe
Copy link
Contributor Author

Woohoo ty friends!

@droctothorpe droctothorpe deleted the tolerations branch February 16, 2024 20:39
@joeyism
Copy link

joeyism commented Feb 19, 2024 •
edited
Loading

Do you guys know when you guys are going to release this feature? I see the latest is 2.0.5 from Dec 2023

petethegreat pushed a commit to petethegreat/pipelines that referenced this pull request Mar 27, 2024
@droctothorpe @edmondop
@tarat44 @petethegreat
feat(kubernetes_platform): Update kubernetes_platform go package to i… (
595d09c 
kubeflow#10442)

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
@droctothorpe @edmondop
@tarat44 @rimolive
feat(kubernetes_platform): Update kubernetes_platform go package to i… (
a49c696 
kubeflow#10442)

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
@droctothorpe @edmondop
@tarat44 @petethegreat
feat(kubernetes_platform): Update kubernetes_platform go package to i… (
dbb2dde 
kubeflow#10442)

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
Reviewers

@Tomcli Tomcli Tomcli approved these changes

@connor-mccarthy connor-mccarthy connor-mccarthy approved these changes

@gkcalat gkcalat Awaiting requested review from gkcalat

Assignees

@Tomcli Tomcli

@connor-mccarthy connor-mccarthy

Labels
approved lgtm ok-to-test size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@droctothorpe @chensun @Tomcli @joeyism @connor-mccarthy