Store the TrainingRuntime numNodes as runtime.Info.PodSet.Count #2539
Conversation
tenzen-y
force-pushed
the
use-count-as-podset
branch
from
322cb5b to
1556321
Compare
|
/assign @kubeflow/wg-training-leads @astefanutti |
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
tenzen-y
force-pushed
the
use-count-as-podset
branch
from
1556321 to
bcbe608
Compare
| if trainJob.Spec.Trainer != nil && trainJob.Spec.Trainer.NumNodes != nil { | ||
| numNodes = trainJob.Spec.Trainer.NumNodes | ||
| info.FindPodSetByName(constants.JobTrainerNode).Count = trainJob.Spec.Trainer.NumNodes |
There was a problem hiding this comment.
When we are going to switch to the model with trainjob-ancestor-step label, how are we going to find the appropriate PodSet ?
I guess, we still have to add ReplicatedJob name to the PodSet Pod name, but for MPI, we should assign count=NumNodes-1 for Node ReplicatedJob, right ?
There was a problem hiding this comment.
We can just add Ancestor field to PodSet.
There was a problem hiding this comment.
Additionally, PodSet.Name represnts rJob name.
| @@ -50,7 +49,7 @@ type Info struct { | |||
| } | |||
|
|
|||
| type RuntimePolicy struct { | |||
| MLPolicy *trainer.MLPolicy | |||
| MLPolicySource *trainer.MLPolicySource | |||
| PodGroupPolicy *trainer.PodGroupPolicy | |||
There was a problem hiding this comment.
Should we rename it for PodGroupPolicySource for consistency ?
There was a problem hiding this comment.
Wait, I think this PodGroupPolicy should be fine since this MLPolicySource introduction is the reason for dropping numNodes.
So, currently, I think this PodGroupPolicy should be fine, IMO
There was a problem hiding this comment.
Thank you @tenzen-y!
Feel free to unhold
/lgtm
/approve
/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich 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 |
Thank you. |
What this PR does / why we need it:
I replaced the
runtime.Info.RuntimePolicy.MLPolicywithruntime.Info.RuntimePolicy.MLPolicySourceso that we can store all rJob numNodes toruntime.Info.TemplateSpec.PodSet.Count.Previously, we stored the non-Trainer numNodes to
runtime.Info.TemplateSpec.PodSet.Countand stored the Trainer numNodes toruntime.Info.RuntimePolicy.MLPolicy.NumNodes, which introduce internal data structure fragmentation.Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Part-of: #2495
Checklist: