Repository navigation
[BC][MLA][CP] Optimize MLA all-gather and Ulysses communication for Kimi K3 and dpskV3 - #4760
Conversation
83be25f to
2a83004
Compare
7141d4b to
69f2ea2
Compare
a7497ea to
b2d187c
Compare
b2d187c to
6e40f0e
Compare
fegin
left a comment
There was a problem hiding this comment.
Put MLAAttention in common if you want to preserve _materialize_kv approach.
| # P = packed per-head KV channels (N + V) | ||
|
|
||
|
|
||
| class MLAInnerAttention(InnerAttention, ABC): |
There was a problem hiding this comment.
Why putting this for Kimi_k3 only? DSv3 can actually share this. I don't see a difference.
6e40f0e to
e96aa78
Compare
452a5d9 to
0b1b18f
Compare
| **kwargs, | ||
| ) -> torch.Tensor: | ||
| k_THK, v_THV = materialize_mla_kv(q_THK, kv_THP, k_shared_TR) | ||
| return VLLMInnerAttention.forward( |
There was a problem hiding this comment.
this defeats the purpose of mla as it still caches full kv. Left a todo for now.
| KVAllGatherCPSlidingWindowFlexInnerAttention, | ||
| ) | ||
| from torchtitan.models.common.attention.cp_mla import ( | ||
| KVAllGatherCPMLAFlexInnerAttention, |
There was a problem hiding this comment.
We should remove "CP" prefix:
- The naming is already too long.
- KVAllGather and Ulysses already show CP meaning.
- CP as upper cases make the reading much harder.
We don't need to do that in this PR, but I feel this is needed. My PR has KVAllGatherCPVarlenInnerAttention and KVAllGatherCPSlidingWindowVarlenInnerAttention. It is hard to read.
| from torchtitan.models.common.attention import ( | ||
| materialize_mla_kv, | ||
| MLAInnerAttention, | ||
| ScaledDotProductInnerAttention, |
There was a problem hiding this comment.
Any reason why graph trainer needs ScaledDotProductInnerAttention? cc., @SherlockNoMad
| k_shared_TR = spmd.redistribute( | ||
| k_shared_TR, | ||
| cp_group, | ||
| src=spmd.S(_TOKEN_DIM), | ||
| dst=spmd.R, | ||
| backward_options={"op_dtype": k_shared_TR.dtype}, | ||
| ) |
There was a problem hiding this comment.
One minor improvement:
Move this redistribute before packed_q_kv_THF cat. This way we may be able to gain some overlapping, you probably see no improvement at all since cat should be fast, but this change cost nothing.
| class Config(Module.Config): | ||
| n_heads: int | ||
| inner_attention: InnerAttention.Config | ||
| inner_attention: "InnerAttention.Config | MLAAttention.Config" |
There was a problem hiding this comment.
this may look weird, codex proposes an alternative way to use
class Attention(Module):
@dataclass(kw_only=True, slots=True)
class Config(Module.Config):
n_heads: int
backend: "InnerAttention.Config | MLAAttention.Config"
class MLAAttention(Module):
@dataclass(kw_only=True, slots=True)
class Config(Module.Config):
wkv_b: Linear.Config
packed_kv_head_dim: int
inner_attention: InnerAttention.Config
There was a problem hiding this comment.
I suggest not to mess up this class with MLA
| inner.kernel_options["BLOCK_M"] = self._BLOCK_M | ||
| inner.kernel_options["BLOCK_N"] = self._BLOCK_N | ||
| inner_attention_config = ( | ||
| layer_cfg.attention.inner_attention.inner_attention_config |
| inner_attention_map: Mapping[ | ||
| type[InnerAttention | MLAAttention], type[InnerAttention | MLAAttention] | ||
| ] |
There was a problem hiding this comment.
wait, it'd be Mapping[type Inner, type Inner] | Mapping[type MLA, type MLA]?
| FlexAttentionMetadata | VarlenAttentionMetadata | LinearAttentionMetadata | ||
| ) | ||
| AttentionMetadataMap = Mapping[type[InnerAttention], AttentionMetadata] | ||
| AttentionMetadataMap = Mapping[type[InnerAttention | MLAAttention], AttentionMetadata] |
There was a problem hiding this comment.
- is MLAAttention outer or inner or somewhere in the middle? hard to tell from naming
- did you end up choosing the 4 layer structure?
- this PR is mixing two optimizations?
maybe let's chat in person.
| @dataclass(kw_only=True, slots=True) | ||
| class Config(Module.Config): | ||
| @property | ||
| def inner_attention_config(self) -> "InnerAttention.Config": |
There was a problem hiding this comment.
removed this field and switched to get_attention_kernel_config. This is used to extract the inner inner attention for MLA.
| class Config(Module.Config): | ||
| n_heads: int | ||
| inner_attention: InnerAttention.Config | ||
| inner_attention: "InnerAttention.Config | MLAAttention.Config" |
There was a problem hiding this comment.
I suggest not to mess up this class with MLA
|
|
||
| class ContextParallelInnerKDA( | ||
| CPInnerAttention[ | ||
| CPAttention[ |
There was a problem hiding this comment.
i think we need a chart for MLA <> CP <> inference
There was a problem hiding this comment.
we could add once after inference story is completed.
| return out_TD.view(num_tokens, -1, self.value_head_dim) | ||
|
|
||
|
|
||
| class VLLMMLAAttention(MLAAttention): |
There was a problem hiding this comment.
maybe put in vllm/mla.py as it's going to have its own complexity
There was a problem hiding this comment.
vllm mla is banned for now, so this class is deleted.
|
|
||
| class CPInnerAttention( | ||
| InnerAttention, | ||
| class CPAttention( |
There was a problem hiding this comment.
I didn't get this change.
If every module that inherits CPAttention is also inheriting InnerAttention, should we make CPAttention inheriting InnerAttention?
| return self.inner_attention(q_THK, k_THK, v_THV, **kwargs) | ||
|
|
||
|
|
||
| def get_attention_kernel_config( |
There was a problem hiding this comment.
not clear to me why we need this yet
| ), | ||
| "sharding_config": inner_attention_config.sharding_config, | ||
| } | ||
| if isinstance(attention_backend_config, MLAAttention.Config): |
There was a problem hiding this comment.
let's ban this for now, so that you don't need get_attention_kernel_config
|
Claude finished @acisseJZhong's task in 2m 14s —— View job Review of @tianyu-l's latest comments (2026-10-16)
Summary: I agree with 3 of the 4 comments as written. I'd push back on the 4th, banning MLA in vLLM, because Kimi K3 RL recipes already exist on 1.
|
| CP class | non-CP base | Is it an InnerAttention? |
|---|---|---|
KVAllGatherFlexInnerAttention, Ulysses*InnerAttention |
FlexInnerAttention / VarlenInnerAttention |
yes |
KVAllGatherMLAFlexAttention, UlyssesMLAAttention |
MLAAttention |
yes (MLAAttention(InnerAttention), mla.py:52) |
ContextParallelInnerKDA |
InnerKDA |
yes (kda.py:141) |
ContextParallelInnerGDN |
InnerGDN |
yes (gdn.py:268) |
So the switch from CPInnerAttention(InnerAttention, ...) to CPAttention(Module, ...) gains nothing. It also causes the unrelated churn in cp_kda.py / cp_gdn.py / validation.py / decoder.py that the earlier "hmm what happened" comment asked about. My suggestion is to restore main's shape and name:
class CPInnerAttention(
InnerAttention, ABC, Generic[_GlobalAttentionMetadataT, _LocalAttentionMetadataT]
):
@dataclass(kw_only=True, slots=True)
class Config(InnerAttention.Config):
passThe MRO should still resolve, e.g. UlyssesMLAAttention -> Ulysses -> CPInnerAttention -> MLAAttention -> InnerAttention -> Module. That's the same diamond main already has for KVAllGatherFlexInnerAttention(KVAllGatherFlex, FlexInnerAttention). Both intermediate Configs add no slots, so slots=True won't hit a layout conflict. (I couldn't run Python in this job, so please confirm locally.) This also settles the earlier Mapping[type Inner, type Inner] | Mapping[type MLA, type MLA] question: ContextParallelTransform.inner_attention_map can stay Mapping[type[InnerAttention], type[InnerAttention]], as it already is at context_parallel.py:33. fegin's "drop the CP prefix" rename can come in a follow-up.
2. vllm/attention.py:453: "maybe put in vllm/mla.py"
VLLMMLAAttention is currently an empty subclass (Config(MLAAttention.Config): pass, no behavior). I'd delete it rather than move it. _replace_vllm_layer_configs only needs to swap the kernel inside the existing MLA config:
if isinstance(backend_cfg, MLAAttention.Config):
vllm_attention_cfg = dataclasses.replace(
backend_cfg, inner_attention=VLLMInnerAttention.Config(**config_kwargs)
)Once the native vLLM MLA backend arrives (the TODO about caching [kv_c_normed, k_pe]), it will have real logic, and that's when vllm/mla.py makes sense. Creating the file now would only hold an empty class. You'd also need to update tests/rl/unit_tests/cpu/test_vllm_wrapper.py (lines 24, 210, 219, 240, 257).
3 & 4. mla.py:113: "not clear why we need this" and model_wrapper.py:102: "let's ban this for now, so that you don't need get_attention_kernel_config"
I'd push back on banning MLA in vLLM.
- These Kimi K3 RL recipes already exist on
mainand run MLA through vLLM:rl_grpo_kimi_k3_debug_varlen,rl_grpo_kimi_k3_debug_varlen_batch_invariantandrl_grpo_kimi_k3_debug_batch_invariantintorchtitan_recipes/tests/rl/alphabet_sort.py. tests/rl/unit_tests/gpu/test_bitwise_parity.py:929depends on the batch-invariant one.- Banning would turn this CP PR into an RL regression. That violates principle 5 ("Protect battle-tested code paths").
Banning also wouldn't remove the helper. It has 5 callsites, and only 3 of them are vLLM-related:
| Callsite | Why it needs the inner kernel | Removed by the ban? |
|---|---|---|
rl/generator/vllm/model_wrapper.py:82 |
sharding_config |
yes |
rl/generator/generator.py:969 |
Flex/Varlen check | yes |
rl/generate.py:133 |
Flex/Varlen check | yes |
config/transform/batch_invariance.py:43 |
sets Flex kernel_options for training |
no |
experiments/graph_trainer/precompile_main.py:234 |
decides whether to build attention metadata | no |
Suggested alternative: drop get_attention_kernel_config in favor of the existing Configurable.traverse. MLAAttention.Config holds its kernel as an ordinary nested field, so the generic config walk (config/configurable.py:77, already used for AuxLoss.Config in precompile_main.py:254) finds it without any MLA-specific helper:
# batch_invariance.py
for _, flex_cfg, _, _ in layer_cfg.attention.traverse(FlexInnerAttention.Config):
flex_cfg.kernel_options.update(BACKEND="TRITON", BLOCK_M=..., BLOCK_N=...)
# generator.py / generate.py / precompile_main.py
kernel_types = (VarlenInnerAttention.Config, FlexInnerAttention.Config)
has_supported_kernel = next(attention.traverse(kernel_types), None) is not Nonetraverse takes type[_ConfigT], so a tuple might need a small widening or two traverse calls.
model_wrapper.py then only needs sharding_config. With fix 2, the vLLM kernel's sharding can be read from the replaced kernel directly (backend_cfg.inner_attention.sharding_config inside the MLA branch, attention_cfg.inner_attention.sharding_config otherwise). That's a single local isinstance, not a public helper.
After that, get_attention_kernel_config can come out of mla.py and __init__.py. This avoids both the ban and the MLA-aware helper leaking into config transforms and experiments.
Related, optional
config/transform/context_parallel.py:72-88 special-cases the two cp_mla classes so it can rewrite wkv_b's sharding. That's MLA knowledge inside a generic transform, much like the batch-invariance concern. It could move into KVAllGatherMLAFlexAttention.Config.__post_init__ / UlyssesMLAAttention.Config.__post_init__, e.g. self.wkv_b = replace(self.wkv_b, sharding_config=mla_kv_projection_sharding_config(cp=...)). The transform would then stay a plain type swap.
I didn't change any code (review only), and Python and the tests weren't run in this job.
fegin
left a comment
There was a problem hiding this comment.
-
I suggest that split the renaming part to another PR. This PR should focus on MLA CP optimization.
-
You need to rebase and address muse spark as well.
| # MLA attention is not supported by the TorchTitan vLLM wrapper yet. | ||
| disabled=True, |
| # The fused forward bypasses the MLA backend that owns CP communication. | ||
| if isinstance(config.inner_attention, CPInnerAttention.Config): | ||
| raise ValueError( | ||
| "FusedMLAAttention does not support context parallelism. " | ||
| "Remove the fused_mla override to use the configured CP attention backend." | ||
| ) |
There was a problem hiding this comment.
OK to unblock, but what's our long-term plan for this? Giving up fused MLA during CP sounds leaving room for improvement.
There was a problem hiding this comment.
I can follow up in another PR
|
Warning: Unknown label
Please add the new label to .github/pytorch-probot.yml |
| class AllGatherKVSlidingWindowFlexInnerAttention( | ||
| AllGatherKVFlex, SlidingWindowFlexInnerAttention | ||
| AllGatherKVFlex, | ||
| SlidingWindowFlexInnerAttention, |
| output = remat.region( | ||
| self.inner_attention, | ||
| self.remat_region_name("inner_attention"), | ||
| recompute=self.remat_should_recompute("inner_attention"), |
There was a problem hiding this comment.
ActivationRematConfig(
save_regions=["attention.inner_attention"],
)
This doesn't work anymore. The root cause, wkv_b, is a linear, another remat region. If we are going to save "attention.inner_attention", we must save "attention.inner_attention.wkv_b", otherwise, we will get a runtime error.
There was a problem hiding this comment.
One solution is to remove the remat.region here and move it into MLAAttention and separately wrap the kernel and cp_region. kernel recompute follows attention.inner_attention and cp_region recompute follows kernel (hence follow attention.inner_attention).
fegin
left a comment
There was a problem hiding this comment.
Also, this PR also break checkpoint BC, we should note in title [BC]. This is bigger than code BC if some people are already training dsv3.
| "CP attention requires an active multi-rank CP mesh axis." | ||
| ) | ||
|
|
||
| kv_THP = self.project_mla_kv(kv_c_normed_TL) |
There was a problem hiding this comment.
Move this after the redistribute below, this give us some chance to overlap the comm and comp.
| k_shared_TR, | ||
| cp_group, | ||
| src=spmd.S(_TOKEN_DIM), | ||
| dst=spmd.R, | ||
| backward_options={"op_dtype": k_shared_TR.dtype}, |
There was a problem hiding this comment.
Stack order: #5445 -> #4760.
Naming and file moves are in #5445; this PR focuses on MLA CP optimization.
The original optimization idea was inspired by this MLA Ulysses CP proposal.
Shape suffixes:
Ttokens,Hheads,LKV latent,Nnon-positional key,Rshared key,Vvalue;K=N+R,P=N+V,F=K+P. Suffixes describe logical dimensions; collectives change their sharding.K/V all-gather CP
k_shared_TRacross attention heads.
k_shared_TR, then applywkv_band materialize K/V locally. This tradesrepeated projection work for lower communication volume.
Ulysses CP
k_shared_TRacross attention heads.k_shared_TR, then use one all-to-allfor
q_THK+ packedkv_THP.Shared MLA backend and checkpoints
MLAAttentionownswkv_band delegates to a Q/K/V attention kernel;common all-gather communication lives in
distributed/context_parallel.py.attention.inner_attention.wkv_b.weight,replacing state-dict renaming hooks. HF keys remain compatible. Native
checkpoints use the nested key; old
attention.wkv_b.weightkeys are nolonger automatically remapped.
Sharding and backend compatibility
all-gather, token-sharded before Ulysses all-to-all. The CP transform copies
the nested projection config when selecting the backend; the local head
reshape preserves that layout.
fused_mlarejects CP backends at model construction because its fusedforward bypasses the compact MLA communication wrapper. Fused MLA with CP
is not supported; remove the override to use the configured CP backend.
batch-invariant settings traverse Flex configs directly, including those
nested inside MLA.
parity and RL integration tests are disabled until support is restored.
Tests
After rebase/split at
4d5064c40c: 197 CPU tests and the four-GPU MLA sharding test passed. Non-Pyrefly lint passed; Pyrefly has the same 53 diagnostics as main. GraphTrainer validation remains blocked by the installed PyTorch missing_is_cacheable_effect.Numerical comparison
3e6c5c2b1a: DeepSeek V3 and text-only Kimi K3 debug models, 2 H100s, CP=2, BF16/Flex, seed 42, deterministic, 256 tokens, 100 steps. Identical initial weights and a fixed synthetic batch; AdamW LR1e-3, gradient clipping1.0.Relative differences between the materialized-K/V reference and compact MLA on the same revision. Loss is global mean cross-entropy; grad norm is FP32 L2 before clipping.
All runs completed 100 steps with finite loss and gradients. First-step logits and loss are bitwise identical in all four comparisons, but gradients are not: first-step relative L2 gradient differences range from 0.1475% to 0.2716%. Changed BF16 gradient accumulation leads to different subsequent optimizer updates. This fixed-batch comparison does not establish convergence on a training dataset.
Performance
Retested at
3e6c5c2b1aon MAST: DeepSeek-V3 16B, 32 H100 GPUs, CP=8/FSDP=4/EP=8, BF16 mixed precision, Flex attention, C4 test data, CUDA graphs disabled, 100 steps. PyTorch2.15.0a0+git8646abd.The baseline materializes K/V before communication on the same revision and runtime. TPS/GPU is the rank-0 mean +/- step SD over steps 21-99, from logged throughput. One run per case.
K/V all-gather CP
32,768 tokens per DP rank; HeadTail load balancing.
Ulysses CP
8,192 tokens per DP rank; no CP load balancing.
Archived notes.