(1/2) Implementing Compiler Pass for AutoTP - #8204
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3a2bb41dd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Thank you @therealnaveenkamal, this is amazing!
I don't see any critical issue in the code. The approach that reuses existing AutoTP's patterns is great. It gives consistent results with non-DeepCompile AutoTP.
One remaining work is validating the correctness in a more realistic setting. I think it would be good to compare loss values from existing AutoTP and this one. I did similar work for AutoEP. The harness for the verification might be useful if you don't have such a script. We should check different configs like DP1/TP4 and DP2/TP2.
I left a few comments about details. Please consider addressing them. Also, please fix the commit to pass DCO check.
|
Can you also share your plan for the the next step as this PR has 1/2 in the title. |
Signed-off-by: Naveenraj Kamalakannan <therealnaveenkamal@gmail.com>
Signed-off-by: Naveenraj Kamalakannan <therealnaveenkamal@gmail.com>
Signed-off-by: Naveenraj Kamalakannan <therealnaveenkamal@gmail.com>
a3a2bb4 to
478c1e9
Compare
Signed-off-by: Naveenraj Kamalakannan <therealnaveenkamal@gmail.com>
Signed-off-by: Naveenraj Kamalakannan <therealnaveenkamal@gmail.com>
Results of comparing AutoTP module injection against the DeepCompile autotp pass (deepspeedai/DeepSpeed#8204) on dense Qwen3.5 (618M, hybrid GatedDeltaNet/attention, 4xA100): 1.31x speedup at DP2/TP2 and 1.45x at DP1/TP4 with loss agreement at the bf16 noise floor. Includes the writeup, plots, per-run metrics, and pinned environment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Naveenraj Kamalakannan <therealnaveenkamal@gmail.com>
Results of comparing AutoTP module injection against the DeepCompile autotp pass (deepspeedai/DeepSpeed#8204) on dense Qwen3.5 (618M, hybrid GatedDeltaNet/attention, 4xA100): 1.31x speedup at DP2/TP2 and 1.45x at DP1/TP4 with loss agreement at the bf16 noise floor. Includes the writeup, plots, per-run metrics, and pinned environment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Naveenraj Kamalakannan <therealnaveenkamal@gmail.com>
autotp works now
tohtana
left a comment
There was a problem hiding this comment.
Hi @therealnaveenkamal,
Thank you for the update! I think the earlier issues have been addressed.
I also found some correctness issues in the current code. Can you check them?
| Any tensor-parallel layer the pass cannot rewrite is rejected rather than left on the | ||
| module-level path. | ||
| """ | ||
| for name, module in model.named_modules(): |
There was a problem hiding this comment.
Can we set the flag defer_collectives_to_compiler only all modules passed the check?
If this raises an error in the loop, only some modules have defer_collectives_to_compiler=True. But the outer code might catch the error and fallback to eager. In that case, some communication collectives will be skipped.
There was a problem hiding this comment.
got it. now we go through all the modules and if any module is incompatible, we raise an error
|
|
||
| assert specs is not None | ||
| by_type = {spec.partition_type for spec in specs} | ||
| assert PartitionType.ROW in by_type, "the supported entry should still be applied" |
There was a problem hiding this comment.
I think this expectation (and the code) is wrong. To make TP work, COLUMN and ROW should be paired. Partially skipping the conversion breaks it.
There was a problem hiding this comment.
The issue with Llama4 MoE router should be addressed in another PR. How about making it fail when any unsupported style is found.
There was a problem hiding this comment.
fixed it now.
| # already records the partitioning decision the pass needs. | ||
|
|
||
| COLUMN_PARALLEL_LAYERS = (LinearLayer, SubParamLinearLayer) | ||
| ROW_PARALLEL_LAYERS = (LinearAllreduce, SubParamLinearAllreduce) |
There was a problem hiding this comment.
SubParamLinearAllreduce.forward() always executes its module-level row all-reduce, while the compiler pass classifies that layer as row parallel and inserts another graph all-reduce.
There was a problem hiding this comment.
thanks for this, @tohtana. I've made a check for defer_collectives_to_compiler in layers.py
Signed-off-by: Naveenraj Kamalakannan <therealnaveenkamal@gmail.com>
Signed-off-by: Naveenraj Kamalakannan <therealnaveenkamal@gmail.com>
… transformers not 5.x fixed for partial compiler flags, now raises error
|
Hi @tohtana - thanks for pointing out the bugs. I've made the changes. Let me know if this is okay. Thanks. |
|
Hi @therealnaveenkamal, thank you for the update! |
Signed-off-by: Naveenraj Kamalakannan <therealnaveenkamal@gmail.com>
added guards for test_tp_compile
|
Hi @therealnaveenkamal, It started with huggingface/transformers#45621 and fixed by huggingface/transformers#45634. I found you already pushed the version guard, but can we be more specific about versions? |
Signed-off-by: Naveenraj Kamalakannan <therealnaveenkamal@gmail.com>
added warning for transformers bug
|
Thanks @tohtana, Guard is now exact: the range [5.8.0, 5.10.1) lives as a constant in init_tp.py, and the test skips it. |
Signed-off-by: Masahiro Tanaka <mtanaka@anyscale.com>
|
Hi @therealnaveenkamal, I just found the code should work even with the affected versions as long as we choose other than |
|
@tohtana sorry about that. I've merged the PR. |
tohtana
left a comment
There was a problem hiding this comment.
@therealnaveenkamal Thank you for your great work! This is definitely a significant step for DeepCompile.
I really appreciate your contribution to DeepSpeed.
7904603
…eepspeedai#8294) ## Problem huggingface/transformers#47579 (on `main` since `861f4c41`, 2026-08-21) makes `PretrainedConfig.__init__` inject `"embed_tokens": "embedding_rowwise"` into `base_model_tp_plan` whenever `tie_word_embeddings` is true. `SUPPORTED_STYLES` is a strict allowlist and `convert()` raises on any style outside it, rejecting the whole plan, so AutoTP plan conversion now fails for every tied-embedding model (Qwen2/Qwen3, Llama, Gemma) built against transformers `main`. ## Fix Recognize `embedding_rowwise` and convert it to a `SKIP` spec, which is option 2 in deepspeedai#8290: the entry is understood and the embedding is deliberately left replicated. What follows is the behaviour DeepSpeed already implements rather than a new policy. `lm_head` still converts to a gathered column spec, and `_configure_gathered_column_tie_fallbacks` then sees that `lm_head.weight is embed_tokens.weight` and leaves both modules replicated, logging that coupled vocabulary-parallel embedding is not supported yet. A tied model is therefore left in the shape it has on a transformers release without the injection, with both modules replicated and the tie intact. The entry maps to `SKIP` with `grad_allreduce` left false, unlike `replicated_with_grad_allreduce`. The parameter is never split, so `register_replicated_grad_hooks` must not register an all-reduce for it. Styles that are still unknown continue to reject the whole plan. `test_unsupported_style_rejects_whole_plan` is unchanged and still passes. ## Verification Run on CPU in a container at `edaa7221`, against transformers `main` (5.16.0.dev0) and torch 2.13.0+cpu. - The two added tests fail on master with the reported `ValueError` and pass with this change. - `tests/unit/module_inject/` and `tests/unit/runtime/test_tp_plan_extraction.py`: 47 passed, on Python 3.11 and on 3.12. - `pre-commit run --files` on the three changed files passes yapf, check-torchdist, check-license and codespell; flake8 5.0.4 exits 0 on them under Python 3.11. - Not verified here: `test_qwen2_tied_lm_head_falls_back_to_replicated`, which needs 2 GPUs. That is the test deepspeedai#8290 reports as failing and the one this change is meant to restore. ## Two things worth deciding separately Scoping this to `embedding_rowwise` leaves the next transformers-side style to fail the same way, since the injection is unconditional and the allowlist is deny-by-default against a vocabulary DeepSpeed does not own. A general rule for unknown styles looks like a maintainer call rather than something to settle here. Related to that, the `convert()` docstring says entries with an unsupported style become SKIP specs instead of invalidating the plan, but no code path does that, and none does after this change either: an unsupported style still raises before the loop is reached. The docstring and the raise arrived together in deepspeedai#8204, so I have left both alone. Happy to follow up once you have picked the policy. Refs deepspeedai#8290. This covers the conversion failure only, and does not implement vocabulary-parallel tied embeddings (option 1 or 3 in that issue), so I have not used a closing keyword. --------- Signed-off-by: Ehsan Barkhordar <realbarkhordar@gmail.com> Co-authored-by: Ma, Guokai <guokai.ma@gmail.com>
Working on #8104
cc @tohtana