feat(pytorch): Add elastic proposal #522
Conversation
Signed-off-by: cegao <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gaocegege The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: cegao <[email protected]>
Signed-off-by: cegao <[email protected]>
gaocegege
changed the title
|
/cc @kubeflow/wg-training-leads @alculquicondor @zw0610 |
| } | ||
| ``` | ||
|
|
||
| Two fields are added in `common.ReplicaSpec`: `minReplicas` and `maxReplicas`. They acts as MIN_SIZE and MAX_SIZE in the elastic example above. |
There was a problem hiding this comment.
Do these fields make sense for every other operator?
There was a problem hiding this comment.
Personally, I think so. But we should discuss it further /cc @kubeflow/wg-training-leads
|
|
||
| ### Environment Variables | ||
|
|
||
| `SetPodEnv` in `pkg/controller.v1/pytorch/pytorch.go` should be changed. There is no need to set `RANK`, `WORLD_SIZE`, `MASTER_ADDR`, `MASTER_PORT` if TorchElastic is used. `KUBEFLOW_RDZV_HOST`, `KUBEFLOW_RDZV_PORT`, `KUBEFLOW_MIN_SIZE` and `KUBEFLOW_MAX_SIZE` Should be set instead. |
There was a problem hiding this comment.
what determines if a PyTorchJob is elastic or not? whether minReplicas and maxReplicas are different than nil?
There was a problem hiding this comment.
how is selection of elastic vs non elastic execution for the operator?
There was a problem hiding this comment.
Option 1. introduce new field to indicate if it's elastic job.
Option 2. Compare min & max to tell controller implicitly.
I think operator side, it will just reconcile deltas, either add or remove which is already part of the logic. But we do need some changes to honor new fields
There was a problem hiding this comment.
Yes, I think so. Prefer the latter.
|
|
||
| ### Reconciliation | ||
|
|
||
| `JobController.ReconcilePods` should be refactored. Now the pods are returned by `GetPodSlices`. For example, if `spec.Replicas` is 3, the PodSlices may look like: `[[0],[1],[2]]`. It is not expected when elastic training is enabled. |
There was a problem hiding this comment.
What should GetPodSlices return for elastic jobs instead?
How does the controller decide how many pods to create?
There was a problem hiding this comment.
@alculquicondor I assume controller just provides the elastic ability. A different control loop should make the decision like an autoscaler?\
There was a problem hiding this comment.
I will illustrate more about it in the proposal.
There was a problem hiding this comment.
If it happens in a different control loop, then the pytorch controller will create the number of pods equal to replicas?
There was a problem hiding this comment.
Also, if it happens in a separate control loop, why do we have to make minReplicas and maxReplicas part of the ReplicaSpec?
They could just be part of the HPA object.
| value: "${pytorchjob.spec.replicas[worker].minReplicas}" | ||
| - name: KUBEFLOW_MAX_SIZE | ||
| value: "${pytorchjob.spec.replicas[worker].macReplicas}" | ||
| command: "python -m torch.distributed.run --rdzv_backend=c10d --rdzv_endpoint=$KUBEFLOW_RDZV_HOST:$KUBEFLOW_RDZV_PORT --nnodes=$KUBEFLOW_MIN_SIZE:$KUBEFLOW_MAX_SIZE --nproc_per_node=1 xxx.py" |
There was a problem hiding this comment.
Could the operator set a default command and then users can use the args to append more arguments and the python file?
There was a problem hiding this comment.
Maybe users want to set their own entrypoint in the command, I think. Thus it may be better to keep command here, WDYT
There was a problem hiding this comment.
PyTorch 1.10 introduced torchrun, so you may need to be flexible to accomidate <1.10 versions that use python -m torch.distributed.run and >=1.10 with torchrun.
There was a problem hiding this comment.
Yeah, now we use the built-in environment variables PET_* to do it. Then I think we do not have the problem.
| } | ||
| ``` | ||
|
|
||
| ### Autoscaler Integration |
There was a problem hiding this comment.
Is this part of the "alternatives considered"?
There was a problem hiding this comment.
I think we need to support it, but I am not sure how it affects API design. As you know, there is no built-in resource in Kubernetes which has minReplicas and maxReplicas except Autoscaler.
I was thinking if we should put them in the PyTorchJob CRD. Thus make it in the alternatives considered
Signed-off-by: cegao <[email protected]>
|
Nice proposal. I will leave some comments tomorrow. |
|
|
||
| ### Environment Variables | ||
|
|
||
| `SetPodEnv` in `pkg/controller.v1/pytorch/pytorch.go` should be changed. There is no need to set `RANK`, `WORLD_SIZE`, `MASTER_ADDR`, `MASTER_PORT` if TorchElastic is used. `KUBEFLOW_RDZV_HOST`, `KUBEFLOW_RDZV_PORT`, `KUBEFLOW_MIN_SIZE` and `KUBEFLOW_MAX_SIZE` Should be set instead. |
There was a problem hiding this comment.
how is selection of elastic vs non elastic execution for the operator?
|
|
||
| ## Limatations | ||
|
|
||
| - KUBEFLOW_RDZV_PORT will be open for every pod even though workers except worker-0 do not use it. |
There was a problem hiding this comment.
Can we add and expose this port only for Worker-0 pod ?
There was a problem hiding this comment.
Maybe we can, do you mean we deal with it with a custom condition loop?
There was a problem hiding this comment.
Yes, for example. If we don't want to have additional ports.
| containers: | ||
| - name: pytorch | ||
| image: <image> | ||
| command: "python -m torch.distributed.run --rdzv_backend=c10d --rdzv_endpoint=$KUBEFLOW_RDZV_HOST:$KUBEFLOW_RDZV_PORT --nnodes=$KUBEFLOW_MIN_SIZE:$KUBEFLOW_MAX_SIZE --nproc_per_node=1 xxx.py" |
There was a problem hiding this comment.
Let's make sure it's user's choice on rendezvous backends? We don't want to manage this part, right?
There was a problem hiding this comment.
I think so. There are three backends now: static, c10d, etcd. And users can also implement their own backend like redis and so on. Users can specify it manually. If they use c10d, we can set rdzv endpoint for them. If they use etcd, they can set the endpoint by themselves.
There was a problem hiding this comment.
This part may need correction since pytorch-elastic shall be able to read these environment variables directly without being specified as launch arguments.
Co-authored-by: Andrey Velichkevich <[email protected]>
Co-authored-by: Andrey Velichkevich <[email protected]>
Co-authored-by: Andrey Velichkevich <[email protected]>
Signed-off-by: cegao <[email protected]>
|
|
||
| [TorchElastic operator](https://github.com/pytorch/elastic/blob/master/kubernetes/api/v1alpha1/elasticjob_types.go) implemented by @jeffwan puts the new fields under `PyTorchJobSpec`. | ||
|
|
||
| Personally, prefer keeping it in `common.ReplicaSpec` since other Jobs may also need it. |
| Personally, prefer keeping it in `common.ReplicaSpec` since other Jobs may also need it. | ||
|
|
||
| ```diff | ||
| // PyTorchJobSpec is a desired state description of the PyTorchJob. |
There was a problem hiding this comment.
API side. Let's take this into consideration. We want to support user specified pods to scale in as an optional field to give more granular control
kubeflow/mpi-operator#410
|
|
||
| ## Abstract | ||
|
|
||
| [TorchElastic](https://pytorch.org/docs/1.9.0/distributed.elastic.html), which was open sourced over a year ago in the pytorch/elastic github repository, is a runner and coordinator for PyTorch worker processes. it has been part of PyTorch core since 1.9.0. This proposal is to support such feature with the help of PyTorchJob. |
There was a problem hiding this comment.
What's involved to support additional frameworks with elastic capabilities?
There was a problem hiding this comment.
Now we do not have a unified CRD to support all frameworks. Thus I think we can support different frameworks in different CRDs. This PR is for PyTorchJob.
WDYT
There was a problem hiding this comment.
Sounds good. We just need to highlight what features are available for which job kinds in the docs then.
Signed-off-by: cegao <[email protected]>
|
�We may also have this problem: pytorch/pytorch#65992 We should consider it in this proposal. |
|
/cc @qiankunli |
|
@gaocegege: GitHub didn't allow me to request PR reviews from the following users: qiankunli. Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs. In 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. |
Signed-off-by: cegao <[email protected]>
|
I updated the details about HPA. BTW, I think we should upgrade the PyTorchJob APIVersion to v2beta1 since it is a breaking change. |
|
What is breaking about it? If users don't set elastic configurations, wouldn't it work as before? |
|
I double-checked the CRD definition. We can keep compatibility with v1. common.JobStatus is changed but it does not block v1 if we upgrade the kubeflow/common version. Thus we can do it in v1. type ReplicaStatus struct {
+ // LabelSelector is the selector for the replica.
+ LabelSelector *metav1.LabelSelector `json:"labelSelector,omitempty"`
// The number of actively running pods.
Active int32 `json:"active,omitempty"`
// The number of pods which reached phase Succeeded.
Succeeded int32 `json:"succeeded,omitempty"`
// The number of pods which reached phase Failed.
Failed int32 `json:"failed,omitempty"`
} |
|
Right, API changes are breaking only if you remove or rename a field. I think the proposed changes look backwards compatible. |
|
/cc @kubeflow/wg-training-leads Could you please have another look? kubeflow/training-operator#1453 The PR is ready, and the coverall coverage increased (+7.1%) to 15.252%, PyTorch related test coverage is increased from 0% to 80% |
|
The proposal overall looks good to me. A thing I am not clear is the metrics HPA part. I feel it's better to decouple with the job controller but I understand user may need some simple solution for easy onboarding. A global optimizer is something I am looking for. We can discuss it later. Looks like the PR is merged, let's merge this one as well. /lgtm |
|
@kubeflow/wg-training-leads could we get an "/approved" for this? |
|
/approve |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gaocegege, terrytangyuan The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/assign @james-jwu |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
/assign @theadactyl |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Signed-off-by: cegao [email protected]