Fix Ascend NPU RMSNorm and fused-attention mask shapes - #14288
Fix Ascend NPU RMSNorm and fused-attention mask shapes#14288mengchengTang wants to merge 2 commits into
Conversation
d77a8f5 to
609e5b1
Compare
609e5b1 to
04c4ed7
Compare
|
Hi @mengchengTang, thanks for the PR! It does not appear to link an issue it fixes. If this PR addresses an existing issue, please add a closing keyword (e.g. |
ErenAta16
left a comment
There was a problem hiding this comment.
Ran both conditions over a spread of mask shapes. The [B, N, 1, Skv] case the PR is after does get picked up, and the added shape[-1] == key.shape[1] check is a genuine tightening — but two rows behave in ways the new comment doesn't cover.
query is [B, Sq, N, D] so query.shape[1] == Sq; B=2, N=4, Sq=7, Skv=11:
| mask shape | before | after | result shape (after) |
|---|---|---|---|
[B, 1, 1, Skv] |
expand | expand | (2, 1, 7, 11) |
[B, N, 1, Skv] — the target |
skip | expand | (2, 4, 7, 11) |
[B, 1, 1, Skv+3] |
expand | skip | (2, 1, 1, 14) |
[B, N, 1, Skv+3] |
skip | skip | (2, 4, 1, 14) |
[B, 99, 1, Skv] — nonsense head count |
skip | expand | (2, 99, 7, 11) |
[B, 1, Sq, Skv] — already correct |
skip | skip | unchanged |
[B+5, 1, 1, Skv] — wrong batch |
expand | expand | (7, 1, 7, 11) |
Row 3 is a behaviour change worth stating: a 4-D mask whose last dim doesn't match key.shape[1] used to be expanded anyway and now falls through to npu_fusion_attention as-is. That's more correct — silently expanding a mismatched mask was the worse option — but it moves where the failure surfaces from this function to the kernel.
Row 5 is the one I'd tighten. expand(-1, -1, Sq, -1) accepts any shape[1], so dropping shape[1] == 1 without replacing it means a mask with a head count that matches nothing gets reshaped and handed to the kernel. shape[1] in (1, query.shape[2]) would keep the intent ("1 or N") and reject the rest here rather than downstream.
Row 7 is pre-existing on both sides — the 2-D branch checks attn_mask.shape[0] == query.shape[0] and the 4-D branch never has. Not this PR's problem, just noting the asymmetry since the 4-D branch is being touched.
On the normalization.py half: the fix looks right, and self.bias can only be non-None when elementwise_affine=True (it's created inside that branch), so the if self.bias is not None left in the NPU path is consistent with self.weight is not None now gating it. Worth saying out loud in the PR body that NPU users with elementwise_affine=False now take the pure-PyTorch path rather than the fused kernel — that's the correct trade, but it's a silent perf change for anyone who was previously… well, crashing, so probably nobody. The else branch ends with hidden_states.to(input_dtype), so the dtype contract holds on the fallback.
Shape table produced with plain torch tensors on CPU (_maybe_modify_attn_mask_npu's reshaping is device-independent). I have no Ascend hardware, so I can't confirm what npu_fusion_attention does with the row-3 and row-5 masks it now receives — that's the part worth a check from someone with an NPU.
dc1421e to
2152d96
Compare
Thanks for the careful shape table — very helpful. Row 5: Agreed. I’ve tightened the 4-D branch to require attn_mask.shape[1] in (1, query.shape[2]), so we only expand head dim 1 (broadcast) or N, and reject unrelated head counts here instead of handing them to the kernel. Row 3 / shape[-1] == key.shape[1]: I’ve dropped that check to avoid the behaviour change you flagged (mismatched last-dim masks falling through instead of being expanded). Expanding a wrong-Skv mask is still imperfect, but keeping the previous expand behaviour reduces surprise vs. silently moving the failure into npu_fusion_attention. Happy to revisit if maintainers prefer the stricter guard. Row 7: Acknowledged — leaving the pre-existing 2-D vs 4-D batch-check asymmetry alone in this PR. normalization.py: Thanks for confirming. I’ve updated the PR description to note that with elementwise_affine=False we take the pure-PyTorch path instead of the fused kernel, and that the fallback still restores dtype via hidden_states.to(input_dtype). I’ve also verified the updated path on Ascend NPU (including the LTX-2 _native_npu case this PR targets). |
0ba183b to
5c3f9da
Compare
|
@yiyixuxu @sayakpaul @zhtmike These two Ascend NPU fixes (#14288 and #14396) are issues I ran into while adapting verl-omni — could you take a look when you get a chance? Thanks! |
|
Hi @mengchengTang , I have replied in the issues |
What does this PR do?
Fixes two Ascend NPU incompatibilities hit when running LTX-2 with
attn_backend=_native_npu:RMSNorm with
elementwise_affine=Falseleavesweight=None.torch_npu.npu_rms_normrequires a gamma tensor, so we only use the fused kernel whenweight is not None, and otherwise fall back to the existing PyTorch path (same as CUDA). This is a silent perf change for that config; the previous path would crash, so it is unlikely anyone relied on it. The fallback still ends withhidden_states.to(input_dtype).Fused attention mask: Ascend FA does not broadcast a singleton query-length dim. Expand masks shaped
[B, 1|N, 1, Skv](e.g. LTX cross-attn[B, N, 1, Skv]) to[B, 1|N, Sq, Skv], generalizing the existing[B, 1, 1, Skv]handling. The head dim is restricted to1orN(query.shape[2]) so unrelated head counts are not expanded.Screenshots
Error 1 — RMSNorm /
gammais NoneError 2 — FA mask shape
[1, 32, 1, 1024]Fixes #14380
Testing
Environment
mainTest results
dg845/LTX-2.3-Diffuserswith verl-omni LTX-2.3 FlowGRPORMSNorm(elementwise_affine=False)from #14380npu_rms_normreceivesweight=Noneand crashes[B, N, 1, Skv]from #14380[B, N, Sq, Skv]and attention completes successfullyBefore submitting
self-reviewskill on the diff?documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@yiyixuxu @sayakpaul — Ascend NPU /
_native_npuattention + RMSNorm