Skip to content

[Bugfix][MiniMax-H3] Fix text-encoder TP group tiling and NPU packed varlen under AllGather-KV SP - #7889

Closed
brandneway wants to merge 2 commits into
vllm-project:mainfrom
brandneway:fix/h3-te-tiled-and-packed-cp
Closed

brandneway wants to merge 2 commits into
vllm-project:mainfrom
brandneway:fix/h3-te-tiled-and-packed-cp

Conversation

@brandneway

Copy link
Copy Markdown
Contributor

Purpose

Two independent startup/runtime fixes for MiniMax-H3 multi-degree parallel serving on NPU (e.g. 16 cards: DiT SP=16 + text encoder TP=8 + VAE patch parallel), both hit while bringing up Ulysses × AllGather-KV topologies:

1. Text encoder TP fails to start when text_encoder_tp_size < DiT world size.

_build_text_encoder_group builds a single GroupCoordinator covering only ranks [0, tp_size). GroupCoordinator.__init__ asserts that every constructing rank belongs to one of its groups, so with tp_size smaller than the DiT world — e.g. tp=8 on 16 devices, the only legal topology for Qwen3-VL's 64 Q / 8 KV heads — every rank outside [0, tp) fails the bare assert at startup.

Fix: tile the DiT world into dit_world / tp_size contiguous groups (_text_encoder_group_ranks). Every rank holds membership in exactly one group; only the first group encodes, and its output still reaches all DiT ranks through the existing world-group broadcast. tp_size == world yields the same single group as before, so existing deployments are unchanged. Non-divisible combinations now fail with a clear message.

2. Packed varlen attention rejects every request under AllGather-KV SP.

With allgather_degree > 1 the NPU attention backend sees local Q against global K/V. The square packed contract in _resolve_packed_seq_npu (used_q <= total_q) declines, and the mask-rebuild fallback then rejects the global valid_kv_length against the local query length, failing every packed H3 request with "packed metadata is unusable ... refusing to run unmasked attention over padding rows".

Fix: _resolve_packed_seq_npu accepts the query_ranges recorded by the AllGather-KV strategy. A single packed range means the local Q rows are a contiguous slice of the same global real document, so seq_q becomes the slice's [local_real, local_total] pair while seq_k keeps the global [real, pad] pair — the same "pad is a strict suffix, pad rows are never consumed" contract, with no mask materialized. Both packed NPU paths (varlen, laser prefix-slice) forward the ranges; square (non-SP) behavior is unchanged. The mask fallback now checks valid_kv_length against the K/V domain (key.shape[1]) so it stays a correct safety net for multi-range layouts.

Test Plan

  • New CPU unit tests: tests/diffusion/models/minimax_h3/test_minimax_h3_encoder_group.py (group layout tiling, divisibility rejection) and tests/diffusion/attention/test_flash_attn.py group A2 (per-rank packed boundary resolution under AllGather-KV, unsupported-range rejection, square-contract preservation).
  • pytest tests/diffusion/attention/test_flash_attn.py -k "packed_seq or cp_": 16 passed.
  • NPU serving validation (Ascend 950PR): 4-card ulysses_degree=2 × allgather_degree=2 end-to-end video generation verified bitwise-size-identical outputs against pure Ulysses baselines.

Test Result

All new unit tests pass on CPU; the serving path was validated end-to-end on Ascend NPU with the composed topology.


This contribution is made by the author with DCO sign-off.

… world works

The encoder TP group was built as a single group covering only the first
text_encoder_tp_size DiT ranks via init_world_group. GroupCoordinator
asserts that every constructing rank belongs to one of its groups, so with
tp_size smaller than the DiT world (e.g. tp=8 on 16 NPUs, the only legal
topologies for Qwen3-VL's 64 Q / 8 KV heads) every rank outside [0, tp)
failed the assert at startup.

Tile the DiT world into dit_world/tp_size contiguous groups instead. Every
rank holds membership in exactly one group; only the first group encodes,
and its output still reaches all DiT ranks through the existing world-group
broadcast. tp_size == world yields the same single group as before, so the
existing deployments are unchanged.

Signed-off-by: brandneway <gyuan4892@gmail.com>
Under AllGather-KV sequence parallelism (allgather_degree > 1), the
attention backend sees local Q against global K/V, so the square packed
contract (used_q <= total_q) declines and the mask fallback then rejects
the global valid_kv_length against the local query length, refusing every
packed request with 'packed metadata is unusable'.

_resolve_packed_seq_npu now accepts the query_ranges recorded by the
AllGather-KV strategy: a single packed range means the local Q rows are a
contiguous slice of the same global real document, so seq_q becomes the
slice's [local_real, local_total] pair while seq_k keeps the global
[real, pad] pair — the same 'pad is a strict suffix, pad rows are never
consumed' contract, no mask materialized. Both packed NPU paths (varlen,
laser prefix-slice) forward the ranges; their square behavior is unchanged.

The mask rebuild fallback keeps working as the safety net for multi-range
layouts by checking valid_kv_length against the K/V domain (key.shape[1])
instead of the local query length.

Signed-off-by: brandneway <gyuan4892@gmail.com>
@vllm-omni-review-bot

Copy link
Copy Markdown

This PR appears to belong to: docs/design/module/diffusion/index.md.

Module owners: @david6666666 @wtomin @Isotr0py

Routing: @david6666666 via module of the changed files, module named in the PR description, model owner; @wtomin via module of the changed files, module named in the PR description, CODEOWNERS; @Isotr0py via module of the changed files, module named in the PR description

@brandneway, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer.

Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment.

@brandneway

Copy link
Copy Markdown
Contributor Author

Superseded by #7937, which includes both fixes from this PR on top of the composed Ulysses x AllGather-KV topology they were built for.

@brandneway brandneway closed this Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants