Conversation
Sorry, something went wrong.
…erator Nothing upstream exercised the MPS paths, so regressions could only be caught on a developer's Mac. The new workflow runs the MPS-green unit test subset (fused/CPU Adam, comm staging, config-driven training) on GitHub's arm64 macOS runners, which expose a working MPS device. It cannot interfere with the existing CI: PR triggers are scoped to MPS-relevant paths, the job runs in its own workflow with a hard timeout and concurrency cancellation, and it is not a required check. MPS_Accelerator now fails at construction with a clear message on torch older than 2.3, where the torch.mps memory queries ZeRO depends on do not exist, instead of an AttributeError deep inside the runtime. Signed-off-by: PKUWZP <zhipeng.rainbowserie@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e794b17f2
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Sorry, something went wrong.
| - name: Install PyTorch | ||
| run: | | ||
| pip install torch torchvision | ||
| python -c "import torch; assert torch.backends.mps.is_available(), 'no MPS device on this runner'; print(torch.__version__)" |
There was a problem hiding this comment.
Run the MPS job on a GPU-capable runner
On GitHub's hosted macos-15 image, the arm64 CPU does not imply that the VM exposes GPU hardware acceleration, so torch.backends.mps.is_available() remains false and this assertion terminates every PR, merge-queue, scheduled, and manual run before the tests execute. Use an MPS-capable self-hosted runner or another runner provider that exposes Metal; see GitHub's macOS runner limitations.
Useful? React with 👍 / 👎.
Sorry, something went wrong.
| @@ -0,0 +1,55 @@ | |||
| name: mps-torch-latest | |||
There was a problem hiding this comment.
Add the mandated license header
This newly added workflow begins directly with its name and omits the required SPDX and DeepSpeed Team license header. Add the mandated header before merging.
AGENTS.md reference: AGENTS.md:L12-L16
Useful? React with 👍 / 👎.
Sorry, something went wrong.
| @@ -0,0 +1,55 @@ | |||
| name: mps-torch-latest | |||
There was a problem hiding this comment.
Sign off this non-merge commit
The reviewed commit message has no Signed-off-by trailer, although it is a non-merge commit; the repository's required DCO workflow validates commit signoffs and will reject it. Recreate the commit with --signoff.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
Sorry, something went wrong.
| if not hasattr(torch.mps, "recommended_max_memory"): | ||
| raise ValueError("MPS_Accelerator requires torch>=2.3 " | ||
| "(this torch build has no torch.mps.recommended_max_memory)") |
There was a problem hiding this comment.
Add regression coverage for the MPS floor check
The new constructor failure is only described as manually validated in the commit message; none of the added tests exercises either the missing-attribute error or successful construction. Add an automated unit test that controls recommended_max_memory, so this user-visible compatibility check cannot silently regress.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
Sorry, something went wrong.
| raise ValueError("MPS_Accelerator requires torch>=2.3 " | ||
| "(this torch build has no torch.mps.recommended_max_memory)") |
There was a problem hiding this comment.
Align the documented MPS PyTorch minimum
This error now advertises PyTorch 2.3 as the MPS minimum, while docs/_tutorials/accelerator-setup-guide.md still instructs Apple Silicon users to install PyTorch 2.4 or newer. Align the guide and runtime floor so users on 2.3 receive an unambiguous support statement.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
Sorry, something went wrong.
Spawn-based DistributedTest cases cost 60-75s each on the shared arm64 runners, which pushed the sequential suite past the job budget before pytest could even print its summary. Three workers bring it inside the limit, and a 300s per-launch timeout fails hung tests fast. Signed-off-by: PKUWZP <zhipeng.rainbowserie@gmail.com>
DistributedFixture registers itself through pytest internals (_pytestfixturefunction) that stopped working in newer pytest, which is why requirements-dev.txt pins pytest<8.4; the unpinned install on the macOS runner picked a newer version and the fixture-based tests errored with 'fixture not found' instead of skipping. Signed-off-by: PKUWZP <zhipeng.rainbowserie@gmail.com>
Reviewers
At least 1 approving review is required to merge this pull request.
Summary
Closes the remaining gap in the Apple Silicon support series (#8293, #8300, #8303, #8307): none of the MPS paths were exercised by CI — every MPS-gated test skips on Linux runners, so regressions could only be caught on a developer's Mac.
macOS CI workflow (mps-torch-latest.yml)
Runs the MPS-green unit test subset on GitHub's arm64 macOS runners (macos-15), which expose a working MPS device:
Designed not to interfere with existing CI:
torch floor check
MPS_Accelerator.__init__ now fails with a clear message on torch older than 2.3, where the torch.mps memory queries ZeRO depends on (recommended_max_memory) do not exist — previously this surfaced as a bare AttributeError deep inside ZeRO's flatten logic. Feature-detected rather than version-parsed. (The Metal FusedAdam kernel already degrades gracefully on torch without compile_shader.)
Validation