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(kubernetes_platform): Update kubernetes_platform go package to include pod labels and annotations #10357

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

Tomcli
Copy link
Member

@Tomcli Tomcli commented Jan 3, 2024

Description of your changes:
Part of #9714 and #9768 and #10363

Update kubernetes_platform go package to include pod labels and annotations. Once this is ready, I will also update the backend driver, compiler, and SDK with this new feature.

Checklist:

@Tomcli
Copy link
Member Author

Tomcli commented Jan 4, 2024

/retest

@Tomcli
Copy link
Member Author

Tomcli commented Jan 4, 2024

/test kfp-kubernetes-execution-tests

@Tomcli
Copy link
Member Author

Tomcli commented Jan 5, 2024

/test kfp-kubernetes-execution-tests

@Tomcli
Copy link
Member Author

Tomcli commented Jan 5, 2024

Hi @connor-mccarthy, I'm not sure what caused the execution-tests to fail. The PR is intended to add the syntax and proto definition for pod labels and annotations. It won't impact the existing tests.

@connor-mccarthy
Copy link
Member

connor-mccarthy commented Jan 5, 2024

@Tomcli, the kfp-kubernetes-execution-tests failures are independent of your code. Feel free to ignore. It is marked as optional and non-blocking.

@Tomcli
Copy link
Member Author

Tomcli commented Jan 5, 2024

Thanks @connor-mccarthy, feel free to let me know any syntax you want me to change in this PR. We want to start adding some common kubernetes pod configurations because we see a big demand from the community and from our users.

@connor-mccarthy
Copy link
Member

Thanks, @Tomcli.

If we are to proceed with these features, the backend changes should precede any SDK changes.

cc @chensun

@Tomcli
Copy link
Member Author

Tomcli commented Jan 8, 2024

Hi @connor-mccarthy, the backend changes actually depended on the go package changes in this PR. Because the go.mod is using an external version of the kubernetes_platform package, we will have to update the go.mod to use local module if we want to also include the backend changes in this PR.

github.com/kubeflow/pipelines/kubernetes_platform v0.0.0-20230404213301-bd9f74e34de6

@connor-mccarthy
Copy link
Member

@Tomcli, can we split out the kfp-kubernetes SDK changes from the go/proto/backend code? We should leave the kfp-kubernetes code in a releasable state whenever possible.

@Tomcli Tomcli changed the title feat(kfp-kubernetes SDK): Update kfp kubernetes sdk to include pod labels and annotations feat(kubernetes_platform): Update kubernetes_platform go package to include pod labels and annotations Jan 8, 2024
@Tomcli
Copy link
Member Author

Tomcli commented Jan 8, 2024

@connor-mccarthy I updated the PR to only include the proto file and go package changes. I will update the backend in my next PR and then update the SDK afterward.

Copy link

@Tomcli: 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 1b5b0af 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.

@Tomcli
Copy link
Member Author

Tomcli commented Jan 9, 2024

@connor-mccarthy Is there anything you want me to change in this PR? I also have the backend changes ready if you want to see why we need to update the kubernetes_platform package.
master...Tomcli:pipelines:add-podmetadata-v2

@droctothorpe
Copy link
Contributor

Happy to contribute to this initiative and the follow-up PRs since this feature and toleration support are a requirement for us to migrate to v2.

@Tomcli
Copy link
Member Author

Tomcli commented Jan 10, 2024

Thanks @droctothorpe, if you could help on implementing the toleration support that would be awesome. My plan is to add image pull secret and policy next.

It seems like this is the flow to add these features:

  1. Update the proto file and generate the new kubernetes_platform package (This PR)
  2. Update the driver to patch the new pod spec. For pod metadata my changes are not in pod spec so I need to update the backend compiler instead.
  3. Update the kfp-kubernetes Python SDK.

@droctothorpe
Copy link
Contributor

droctothorpe commented Jan 10, 2024

Sounds good. Down to take a stab at the tolerations implementation. Will follow the flow that you outlined. Makes a ton of sense to break it up into small, discrete PRs.

@connor-mccarthy
Copy link
Member

/lgtm
/approve

FYI, we're in the process of migrating all of the Pipelines SDKs (kfp, kfp-pipeline-spec, and kfp-kubernetes) to protobuf 4. To do this, we plan to cut a release of kfp-kubernetes within the next week. Let's release your SDK changes after that. The proto changes here are okay to be included in this upcoming release.

@google-oss-prow google-oss-prow bot added the lgtm label Jan 11, 2024
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 daa7299 into kubeflow:master Jan 11, 2024
8 of 9 checks passed
@Tomcli
Copy link
Member Author

Tomcli commented Jan 11, 2024

/lgtm /approve

FYI, we're in the process of migrating all of the Pipelines SDKs (kfp, kfp-pipeline-spec, and kfp-kubernetes) to protobuf 4. To do this, we plan to cut a release of kfp-kubernetes within the next week. Let's release your SDK changes after that. The proto changes here are okay to be included in this upcoming release.

That make sense to me. Thank you @connor-mccarthy for the context.

stijntratsaertit pushed a commit to stijntratsaertit/kfp that referenced this pull request Feb 16, 2024
petethegreat pushed a commit to petethegreat/pipelines that referenced this pull request Mar 27, 2024
petethegreat pushed a commit to petethegreat/pipelines that referenced this pull request Mar 29, 2024
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.

3 participants