feat(api): Add PodTemplateOverrides API into TrainJob#2785
Conversation
|
@xigang: GitHub didn't allow me to request PR reviews from the following users: mimowo. Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this: 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. |
|
/ok-to-test |
Pull Request Test Coverage Report for Build 18348353117Details
💛 - Coveralls |
|
@andreyvelich @tenzen-y PTAL. thanks! |
andreyvelich
left a comment
There was a problem hiding this comment.
Thank you for this @xigang!
|
Alright, I’ll go ahead and update the code. |
e1f8c2e to
b5cec0f
Compare
|
/ok-to-test |
|
@andreyvelich @astefanutti The code has been updated. Please take another look, thanks. |
18651dd to
41371bb
Compare
e09ce3f to
bdb0e55
Compare
bdb0e55 to
1f50738
Compare
1f50738 to
aebc0d3
Compare
|
Awesome, appreciate your time @xigang! |
448de4f to
888dd64
Compare
|
@andreyvelich @tenzen-y @mimowo PTAL. |
astefanutti
left a comment
There was a problem hiding this comment.
Do we want to update the section about PodSpecOverrides in docs/proposals/2170-kubeflow-trainer-v2/README.md accordingly?
mimowo
left a comment
There was a problem hiding this comment.
LGTM, thank you so much @xigang and maintainers, happy to see it will address kubernetes-sigs/kueue#7156 in 2.1 for Kubeflow and 0.15 in Kueue 👍
@astefanutti Thanks for the reminder. I’ll submit a separate PR for the PodSpecOverrides section in the 2170 proposal. |
888dd64 to
546331a
Compare
|
/retest |
|
/lgtm Thanks @xigang! |
| if len(metadata) > 0 { | ||
| podTemplatePatch["metadata"] = metadata | ||
| } |
There was a problem hiding this comment.
The check should be kept as it prevents adding empty metadata objects to the strategic merge patch. It's a defensive programming approach that ensures we only modify the patch when there's actual metadata to apply.
| { | ||
| Name: "NEW_VALUE", | ||
| Value: "from_overrides", | ||
| TargetJobs: []trainer.PodTemplateOverrideTargetJob{{Name: constants.Node}}, |
There was a problem hiding this comment.
Please also add labels/annotations to the this test case, to verify it is working.
Signed-off-by: xigang <wangxigang2014@gmail.com>
546331a to
cbf2266
Compare
|
/lgtm |
tenzen-y
left a comment
There was a problem hiding this comment.
Thank you for this great effort!
/lgtm
/approve
/hold cancel
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich, tenzen-y The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: xigang <wangxigang2014@gmail.com>
Signed-off-by: xigang <wangxigang2014@gmail.com>
What this PR does / why we need it:
Follow-up: #2784