KEP-2170: Add validation to Torch numProcPerNode field
#2409
Conversation
astefanutti
force-pushed
the
pr-num-proc
branch
2 times, most recently
from
70577cc to
a74355d
Compare
astefanutti
force-pushed
the
pr-num-proc
branch
from
a74355d to
4b3c70d
Compare
astefanutti
force-pushed
the
pr-num-proc
branch
from
4b3c70d to
412f1f8
Compare
| @@ -194,7 +195,7 @@ type Trainer struct { | |||
| // Number of processes/workers/slots on every training node. | |||
| // For the Torch runtime: `auto`, `cpu`, `gpu`, or int value can be set. | |||
| // For the MPI runtime only int value can be set. | |||
| NumProcPerNode *string `json:"numProcPerNode,omitempty"` | |||
| NumProcPerNode *intstr.IntOrString `json:"numProcPerNode,omitempty"` | |||
There was a problem hiding this comment.
I am also thinking about changing numProcPerNode to IntOrString. However, it is not a standard type in Python.
Thus, users should know what does this type mean: K8sIoApimachineryPkgUtilIntstrIntOrString.
Or we need to make numProcPerNode: Union[int,str] type.
Do we have any plans to expose numProcPerNode parameters to the Trainer object in SDK: https://github.com/kubeflow/trainer/blob/master/sdk/kubeflow/trainer/types/types.py#L73?
Or it will be always auto-populated based on num_nodes and resources_per_node ?
There was a problem hiding this comment.
cc @kubeflow/wg-training-leads @Electronic-Waste
There was a problem hiding this comment.
Yes, I also think there should be a parameter exposed in the SDK. It is only loosely coupled with resources_per_node.
The Kubernetes Python client declares IntOrString fields as object, but Union[int,str] would be better.
There was a problem hiding this comment.
For me, I prefer Union[int, str]. And exposing num_proc_per_node is better.
There was a problem hiding this comment.
Let's continue discussion here: #2313 (comment)
There was a problem hiding this comment.
As we discussed above PR, I fully agree with introducing this IntOrString typed.
astefanutti
force-pushed
the
pr-num-proc
branch
2 times, most recently
from
905fbe7 to
34b00e8
Compare
There was a problem hiding this comment.
Mostly lgtm, thank you!
After the @akshaychitneni PR is merged, I think we can merge this.
@andreyvelich Any thoughts?
astefanutti
force-pushed
the
pr-num-proc
branch
2 times, most recently
from
08f63b7 to
492a90e
Compare
|
I think we can merge this once the conflicts are resolved |
astefanutti
force-pushed
the
pr-num-proc
branch
from
492a90e to
a3b3e8c
Compare
Signed-off-by: Antonin Stefanutti <[email protected]>
astefanutti
force-pushed
the
pr-num-proc
branch
from
a3b3e8c to
9b352e0
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tenzen-y 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 |
What this PR does / why we need it:
Change the TorchMLPolicy
numProcPerNodetype tointstr.IntOrStringand add CRD validation.Checklist: