Skip to content

Fix Pixtral Flash Attention image boundaries after axial RoPE refactor - #49373

Merged
zucchini-nlp merged 3 commits into
huggingface:mainfrom
oliverholworthy:oholworthy/fix-pixtral-flash-attention-image-boundaries
Oct 7, 2026
Merged

zucchini-nlp merged 3 commits into
huggingface:mainfrom
oliverholworthy:oholworthy/fix-pixtral-flash-attention-image-boundaries

Conversation

@oliverholworthy

@oliverholworthy oliverholworthy commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

CPU CI GPU run-slow

What does this PR do?

Fixes #49371

Pixtral passes axial RoPE coordinates shaped [total_patches, 2] to the Flash Attention packed-sequence detector. It flattens both coordinates, producing false image boundaries and a terminal cumulative length twice the number of Q/K/V tokens. A checkpoint-free Pixtral model raises a CUDA illegal memory access on Transformers 5.17.0, 5.18.0, and main at 571c5cc; smaller configurations can return incorrect output without crashing. The same reproduction passes on 5.16.1.

This change derives cumulative lengths and maximum lengths from the per-image patch grids, and passes them explicitly in the flash attention case. Keeping axial coordinates for rotary embeddings and preserve the existing block mask for other attention implementations.

This follows the Qwen2-VL/Qwen3-VL vision-attention approach:

key_states,
value_states,
attention_mask=None,
scaling=self.scaling,
dropout=0.0 if not self.training else self.attention_dropout,
cu_seq_lens_q=cu_seqlens,
cu_seq_lens_k=cu_seqlens,
max_length_q=max_seqlen,
max_length_k=max_seqlen,
is_causal=False,
**kwargs,
)

Related work / coordination

Who can review?

@zucchini-nlp

Signed-off-by: Oliver Holworthy <1216955+oliverholworthy@users.noreply.github.com>
Signed-off-by: Oliver Holworthy <1216955+oliverholworthy@users.noreply.github.com>
@zucchini-nlp
zucchini-nlp self-requested a review October 6, 2026 16:38
Comment on lines -440 to 451
# We only rely on position_ids when using flash attention
# Axial RoPE positions have two coordinates per patch, so they cannot
# describe the packed image boundaries expected by Flash Attention.
sequence_lengths = [p.shape[-2] * p.shape[-1] for p in patch_embeds_list]
cu_seqlens = torch.tensor([0, *sequence_lengths], device=patch_embeds.device, dtype=torch.int32)
cu_seqlens = cu_seqlens.cumsum(dim=0, dtype=torch.int32)
kwargs.update(
cu_seq_lens_q=cu_seqlens,
cu_seq_lens_k=cu_seqlens,
max_length_q=max(sequence_lengths),
max_length_k=max(sequence_lengths),
)
attention_mask = None

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm quite the same as vision utils but oke, not really worth materializing image_grid only to infer FA kwargs

def get_vision_attention_seqlens(
grid_thw: torch.Tensor,
config: PreTrainedConfig,
merge_temporal: bool = False,
kwargs: dict | None = None,
) -> tuple[torch.Tensor, int | None]:
"""Get cumulative and maximum sequence lengths for packed vision attention. ``merge_temporal`` is
forwarded to [`get_vision_cu_seqlens`] (``True`` for clip-level attention, e.g. kimi_k25)."""
cu_seqlens = get_vision_cu_seqlens(grid_thw, merge_temporal=merge_temporal, kwargs=kwargs)
max_seqlen = get_max_seqlen(cu_seqlens, config, kwargs=kwargs)
return cu_seqlens, max_seqlen

Comment on lines +133 to +138
@require_flash_attn
@require_torch_gpu
def test_flash_attention_2_packed_images(self):
config = PixtralVisionConfig(
hidden_size=256,
intermediate_size=512,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually we have a test tests/models/pixtral/test_modeling_pixtral.py::PixtralVisionModelModelTest::test_flash_attn_2_inference_equivalence

and i see it failing on main so lets just make sure it passes now, this new test is not needed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the workflow is all green so lets just delete the test and i'll merge

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review and running the tests. I've removed the new test.

@zucchini-nlp

Copy link
Copy Markdown
Member

run-slow: pixtral

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

AMD CI

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs on AMD:

models: ["models/pixtral"]

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Nvidia CI

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs on Nvidia:

models: ["models/pixtral"]
quantizations: []

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

CI Results (AMD)

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 0f9114b5 workflow commit (merge commit)
PR 621dcd13 branch commit (from PR)
main cbde22f4 base commit (on main)

✅ No failing test specific to this PR 🎉 👏 !

@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

CI Results (Nvidia)

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 0f9114b5 workflow commit (merge commit)
PR 621dcd13 branch commit (from PR)
main cbde22f4 base commit (on main)

✅ No failing test specific to this PR 🎉 👏 !

Signed-off-by: Oliver Holworthy <1216955+oliverholworthy@users.noreply.github.com>
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: pixtral

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

CI recap

Dashboard: View test results in Grafana
Latest run: 37611695664:1
Result: success | Jobs: 1 | Tests: 46 | Failures: 0 | Duration: 1m 52s

@zucchini-nlp
zucchini-nlp enabled auto-merge October 7, 2026 11:38
@zucchini-nlp
zucchini-nlp added this pull request to the merge queue Oct 7, 2026
Merged via the queue into huggingface:main with commit ff704ff Oct 7, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pixtral Flash Attention 2 uses axial RoPE coordinates as packed sequence boundaries (regression in 5.17.0)

3 participants