Fix MLU_Accelerator conformance to the DeepSpeedAccelerator ABC - #8208
Merged
delock merged 2 commits intoAug 4, 2026
Merged
Conversation
MLU_Accelerator diverges from the abstract accelerator contract in three places, and it is the only backend of the nine that does: - pin_memory(self, tensor) omits the ABC's align_bytes parameter. The ZeRO-Infinity / NVMe offload swap path passes it by keyword (runtime/swap_tensor/utils.py:188, partitioned_param_swapper.py:131 and :398, all align_bytes=0), so the call raises TypeError. - initial_seed(self, seed) requires an argument the ABC does not declare. deepspeedai#5569 removed that argument from the ABC and every backend because torch.<device>.initial_seed() takes none; MLU landed later as a copy of the pre-deepspeedai#5569 code and reintroduced it. - create_graph() builds torch.mlu.MLUGraph() but drops the return value, so graph_process() (runtime/utils.py) passes None to capture_to_graph() and replay_graph(). Signatures now match the ABC and the other backends; create_graph returns its graph. Adds two CPU-only regression tests to the existing accelerator suite: a Liskov check that an override may widen the abstract signature but never narrow it, and a check that create_graph returns. Signed-off-by: Ehsan Barkhordar <realbarkhordar@gmail.com>
delock
approved these changes
Aug 4, 2026
delock
enabled auto-merge
August 4, 2026 06:04
Collaborator
|
@ebarkhordar I think the test you added served as good hardening of DeepSpeedAccelerator ABC. Thanks for your fix! |
Contributor
Author
|
Thanks for the review and the merge. The signature check was the part I was least sure would land, so it is good to hear it reads as hardening: it catches any backend that narrows the ABC, not just MLU. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MLU_Acceleratordiverges from theDeepSpeedAcceleratorcontract in three places. I compared all nine backends against the ABC and it is the only one that does.1.
pin_memorydropsalign_bytes(accelerator/mlu_accelerator.py:226)The ABC declares
pin_memory(self, tensor, align_bytes=1)(abstract_accelerator.py:264); MLU declarespin_memory(self, tensor). Three in-tree call sites pass it by keyword, all on the ZeRO-Infinity / NVMe offload swap path:deepspeed/runtime/swap_tensor/utils.py:188deepspeed/runtime/swap_tensor/partitioned_param_swapper.py:131deepspeed/runtime/swap_tensor/partitioned_param_swapper.py:398all
align_bytes=0, so on MLU these raiseTypeErrorrather than differing quietly.2.
initial_seedrequires an argument the ABC does not declare (:81)#5569 changed
initial_seed(self, seed)toinitial_seed(self)in the ABC and in cpu/cuda/hpu/mps/npu/xpu, on the grounds thattorch.<device>.initial_seed()takes no argument. MLU landed 3 months later in #6472 as a copy of the pre-#5569 NPU code, which brought the argument back, soget_accelerator().initial_seed()raisesTypeErroron MLU alone.To be accurate about severity: the only in-tree reference is commented out (
deepspeed/runtime/pipe/module.py:200), so this is a break in the public accelerator API and a regression of the #5569 decision, not a live in-tree crash.3.
create_graphbuilds a graph and drops it (:186)The in-tree consumers use the returned handle:
graph_process()(deepspeed/runtime/utils.py:101) passes it straight intocapture_to_graph()and thenreplay_graph(), so both getNone.Worth separating from a deliberate opt-out. cpu, mps, npu, sdaa and xpu return
Noneand pair it with anoop_context()capture_to_graph. MLU pairs the missing return with a real capture (torch.mlu.graph(...),:189) and a realgraph.replay()(:192), which is the cuda/hpu/supa shape. That reads as an oversight rather than an abstention, but you would know better than I would.The fix
Three one-line changes, matching what the other backends already do: add
align_bytes=1topin_memory(the cuda/npu/sdaa/supa body is byte-identical to MLU's, so nothing else changes), drop theseedargument frominitial_seed, and return fromcreate_graph.How I verified
tests/unit/accelerator/test_accelerator.py: a Liskov check that an override may widen the abstract signature but never narrow it, and a check thatcreate_graphreturns. Ondf84f6d8the file goes from 2 failed / 25 passed to 27 passed; both failures are MLU and the other eight backends pass untouched.device/device_namea default the ABC declares required, and that widening is fine. Only requiring more than the ABC, or dropping one of its defaulted parameters, fails. That is what keeps the cpu/mpscreate_op_buildernaming difference from tripping it.cpu-torch-latestjob. yapf 0.40.0 reports both files clean.graph_process) are read from the call sites rather than observed on silicon.Happy to split this into three commits, or to drop the tests if you would rather keep the suite as it is.