Skip to content

[Bugfix] Fix MiniMax-H3 packed padding with Ring attention - #6995

Closed
heyuanliu-intel wants to merge 2 commits into
vllm-project:mainfrom
heyuanliu-intel:pr/h3-attn-ring-padding-mask
Closed

heyuanliu-intel wants to merge 2 commits into
vllm-project:mainfrom
heyuanliu-intel:pr/h3-attn-ring-padding-mask

Conversation

@heyuanliu-intel

@heyuanliu-intel heyuanliu-intel commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Two commits; the second depends on the first.

Commit 1: [Bugfix] Fix MiniMax-H3 packed padding with Ring attention

Purpose

Problem: MiniMax-H3 pads its packed sequence to an aligned length and passes valid_kv_length to the local attention backends. On the Ring path it instead builds an attn_mask at the global padded length (minimax_h3_transformer.py:514), which the Ring backends reject, so any padded request fails under Ring (the failure recorded in recipes/MiniMaxAI/MiniMax-H3.md).

Fix: both Ring backends accept an optional valid_kv_length and trim each K/V block to the valid prefix of its source rank (ring_utils.ring_kv_block_valid_length); a fully padded block launches no attention. H3 publishes valid_kv_length on the Ring path instead of the mask. causal=True together with valid_kv_length raises ValueError: on a rectangular tile the causal diagonal would silently shift. With valid_kv_length=None every path is byte-for-byte the previous behaviour.

Test Plan

Reproduce: serve MiniMax-H3 with Ring sequence parallel and send a request whose packed length is not a multiple of the alignment.

pytest -q tests/diffusion/attention/test_ring_padding.py
pytest -q tests/diffusion/models/minimax_h3/test_minimax_h3_contract.py -k ring

vLLM Version: as pinned by requirements/. vLLM-Omni Commit: rebased onto current main.

Test Result

CPU tests pass (ring communicator and kernel stubbed, rank-2-of-3 block ordering covered). Reverting the K/V trim makes the ordering test observe [4, 4] instead of [1, 4]; removing the causal guard makes the two refusal tests fail with DID NOT RAISE. Not run on multi-rank hardware.


Commit 2: [Bugfix] Honor runtime SP state in H3 Ring padding

Purpose

Problem: MiniMaxH3Attention._run_packed_attention reads the static use_ring flag in three places to decide between the Ring and local packed paths. Whether Ring actually runs is a runtime decision (Attention._get_active_parallel_strategy also honours skip_sequence_parallel and forward_context.sp_active), so a layer that declares Ring but is not running it this step takes the Ring branch with the wrong inputs.

Fix: decide from the active strategy through one local helper; a layer not running Ring this step takes the local packed path (including NPU npu_attn_varlen). A layer genuinely running Ring behaves as before. Depends on the previous commit for the Ring branch.

Test Plan

Reproduce: configure Ring, then run a step with sp_active=False (or a layer with skip_sequence_parallel): the packed attention still takes the Ring branch.

pytest -q tests/diffusion/models/minimax_h3/test_minimax_h3_contract.py -k ring

vLLM Version: as pinned by requirements/. vLLM-Omni Commit: rebased onto current main.

Test Result

CPU tests pass for the three combinations (Ring active, Ring declared but inactive, no Ring).

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@vllm-omni-review-bot

Copy link
Copy Markdown

This PR appears to be related to model: MinimaxH3.

Model owners: @david6666666

@heyuanliu-intel, 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.

## Purpose

**Problem:** MiniMax-H3 pads its packed sequence to an aligned length and passes `valid_kv_length` to the local attention backends. On the Ring path it instead builds an `attn_mask` at the global padded length (`minimax_h3_transformer.py:514`), which the Ring backends reject, so any padded request fails under Ring (the failure recorded in `recipes/MiniMaxAI/MiniMax-H3.md`).

**Fix:** both Ring backends accept an optional `valid_kv_length` and trim each K/V block to the valid prefix of its source rank (`ring_utils.ring_kv_block_valid_length`); a fully padded block launches no attention. H3 publishes `valid_kv_length` on the Ring path instead of the mask. `causal=True` together with `valid_kv_length` raises `ValueError`: on a rectangular tile the causal diagonal would silently shift. With `valid_kv_length=None` every path is byte-for-byte the previous behaviour.

## Test Plan

**Reproduce:** serve MiniMax-H3 with Ring sequence parallel and send a request whose packed length is not a multiple of the alignment.

```bash
pytest -q tests/diffusion/attention/test_ring_padding.py
pytest -q tests/diffusion/models/minimax_h3/test_minimax_h3_contract.py -k ring
```

**vLLM Version:** as pinned by `requirements/`. **vLLM-Omni Commit:** rebased onto current `main`.

## Test Result

CPU tests pass (ring communicator and kernel stubbed, rank-2-of-3 block ordering covered). Reverting the K/V trim makes the ordering test observe `[4, 4]` instead of `[1, 4]`; removing the causal guard makes the two refusal tests fail with `DID NOT RAISE`. Not run on multi-rank hardware.

Signed-off-by: heyuanliu-intel <heyuan.liu@intel.com>
## Purpose

**Problem:** `MiniMaxH3Attention._run_packed_attention` reads the static `use_ring` flag in three places to decide between the Ring and local packed paths. Whether Ring actually runs is a runtime decision (`Attention._get_active_parallel_strategy` also honours `skip_sequence_parallel` and `forward_context.sp_active`), so a layer that declares Ring but is not running it this step takes the Ring branch with the wrong inputs.

**Fix:** decide from the active strategy through one local helper; a layer not running Ring this step takes the local packed path (including NPU `npu_attn_varlen`). A layer genuinely running Ring behaves as before. Depends on the previous commit for the Ring branch.

## Test Plan

**Reproduce:** configure Ring, then run a step with `sp_active=False` (or a layer with `skip_sequence_parallel`): the packed attention still takes the Ring branch.

```bash
pytest -q tests/diffusion/models/minimax_h3/test_minimax_h3_contract.py -k ring
```

**vLLM Version:** as pinned by `requirements/`. **vLLM-Omni Commit:** rebased onto current `main`.

## Test Result

CPU tests pass for the three combinations (Ring active, Ring declared but inactive, no Ring).

Signed-off-by: heyuanliu-intel <heyuan.liu@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working diffusion codes related to diffusion models high priority high priority issue, needs to be done asap

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants