KEP-2170: Implement MPI Plugin for Kubeflow Trainer #2394
Conversation
google-oss-prow
bot
requested review from
a team,
deepanker13,
kuizhiqing,
helenxie-bit,
franciscojavierarceo,
alculquicondor,
Electronic-Waste and
tenzen-y
|
@andreyvelich: GitHub didn't allow me to request PR reviews from the following users: seanlaii, roteme-runai, astefanutti, shravan-achar, kannon92, mlsorensen, saileshd1402, akshaychitneni, mchmarny. 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. |
Yes, we are going to use the base images from mpi-operator to start OpenSSH server on the Trainer Nodes: https://github.com/kubeflow/mpi-operator/blob/master/build/base/Dockerfile. I don't want to re-use the Go assets from the mpi-operator, since our Go module will get additional dependencies in that case. |
andreyvelich
force-pushed
the
issue-2217-mpi-plugin
branch
from
ef67ca8 to
58beb0c
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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: Andrey Velichkevich <[email protected]>
Signed-off-by: Andrey Velichkevich <[email protected]>
andreyvelich
force-pushed
the
issue-2217-mpi-plugin
branch
from
58beb0c to
97ea1c0
Compare
Signed-off-by: Andrey Velichkevich <[email protected]>
andreyvelich
changed the title
|
We discussed with @tenzen-y that we implement remaining items for MPI plugin in the followup PRs. |
Signed-off-by: Andrey Velichkevich <[email protected]>
There was a problem hiding this comment.
Basically LGTM. Just some small questions for you @andreyvelich
| @@ -528,11 +530,12 @@ func TestWatchExtensionPlugins(t *testing.T) { | |||
| registry fwkplugins.Registry | |||
| wantPlugins []framework.WatchExtensionPlugin | |||
| }{ | |||
| "coscheding and jobset are performed": { | |||
| "coscheding, jobset, and mpi are performed": { | |||
There was a problem hiding this comment.
| "coscheding, jobset, and mpi are performed": { | |
| "coscheduling, jobset, and mpi are performed": { |
A small typo error.
| type ComponentBuilderPlugin interface { | ||
| Plugin | ||
| Build(ctx context.Context, runtimeJobTemplate client.Object, info *runtime.Info, trainJob *trainer.TrainJob) (client.Object, error) | ||
| Build(ctx context.Context, runtimeJobTemplate client.Object, info *runtime.Info, trainJob *trainer.TrainJob) ([]client.Object, error) |
There was a problem hiding this comment.
Why do we need to change it to slice type?
| // Update envs for Info object. | ||
| // TODO (andreyvelich): Add validation to check that TrainJob doesn't have MPI envs. | ||
| // TODO (andreyvelich): We should validate that envs from different plugins don't conflict with each other. | ||
| // Ref: https://github.com/kubeflow/training-operator/pull/2308#discussion_r1823229940 |
There was a problem hiding this comment.
| // Ref: https://github.com/kubeflow/training-operator/pull/2308#discussion_r1823229940 | |
| // Ref: https://github.com/kubeflow/trainer/pull/2308#discussion_r1823229940 |
| // Update values from the Info object. | ||
| // Update the env variables. | ||
| if info.Trainer.Env != nil { |
There was a problem hiding this comment.
Should we combine these two comments into one?
Fixes: #2290, #2217
This is initial implementation of MPI Plugin for Kubeflow Trainer.
I am using the similar approach to generate SSH keys and ConfigMap as in MPI-Operator.
This PR will only add support for OpenMPI.
List of items for the followup PRs:
sshd./cc @kubeflow/wg-training-leads @Electronic-Waste @astefanutti @tenzen-y @deepanker13 @seanlaii @saileshd1402 @kannon92 @kuizhiqing @shravan-achar @akshaychitneni @helenxie-bit @franciscojavierarceo @alculquicondor @roteme-runai @mchmarny @mlsorensen