Repository navigation
Conversation
The DeepSpeed and MLX images ship Open MPI 5, so no e2e covered the OMPI_MCA_orte_* env vars anymore. Run the deepspeed-distributed layout with the Open MPI 4 mpi-pi image from kubeflow/mpi-operator and check that mpirun reaches both hosts. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Matteo Bunino <48362942+matbun@users.noreply.github.com>
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Share one test body between the DeepSpeed image and the Open MPI 4 image instead of duplicating it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Matteo Bunino <48362942+matbun@users.noreply.github.com>
Signed-off-by: Matteo Bunino <48362942+matbun@users.noreply.github.com>
andreyvelich
left a comment
andreyvelich
left a comment
There was a problem hiding this comment.
Thanks for this @matbun!
/ok-to-test
| ginkgo.By("Create a copy of the deepspeed-distributed runtime with the given image", func() { | ||
| gomega.Expect(k8sClient.Create(ctx, mpiRuntime)).Should(gomega.Succeed()) | ||
| }) |
There was a problem hiding this comment.
After this PR is merged: #4050 (comment), we can directly set the image via TrainJob spec rather than updating the runtime.
There was a problem hiding this comment.
True. Let's wait for that and I'll rebase
What this PR does / why we need it:
Since #4029 the DeepSpeed and MLX images ship Open MPI 5, so the OpenMPI e2e only exercises the
PRTE_MCA_*env vars. This turns that test into aDescribeTablewith two entries running the samempirun hostnamecheck from #4166: the DeepSpeed image as before, andmpioperator/mpi-pi:v0.6.0-openmpi(Open MPI 4.1.4, pinned by digest), so theOMPI_MCA_orte_*path stays tested.For the Open MPI 4 entry the test copies the deployed
deepspeed-distributedruntime into a namespaced TrainingRuntime and sets the image on both replicatedJobs, sincespec.trainer.imageonly reaches the launcher (#4030). If the launcher hits the DNS race described in #4241, its container restarts in place, because JobSet defaultsrestartPolicytoOnFailure, and the Job still succeeds. Most of the diff is re-indentation of the existing test, with whitespace hidden it is 33 lines added and 4 removed.go vetpasses and a Ginkgo dry run lists both entries. The same runtime and check completed on a cluster with Trainer v2.1.0, as reported in #4241.Which issue(s) this PR fixes:
Fixes #4241
Checklist:
Cc @andreyvelich as discussed in the last meeting