Skip to content

[Qwen3-VL] Bound vision RoPE inference FP32 intermediates - #49665

Closed
YaoleWu wants to merge 1 commit into
huggingface:mainfrom
YaoleWu:yaole/vision-rope-chunked-scratch
Closed

YaoleWu wants to merge 1 commit into
huggingface:mainfrom
YaoleWu:yaole/vision-rope-chunked-scratch

Conversation

@YaoleWu

@YaoleWu YaoleWu commented Oct 16, 2026

Copy link
Copy Markdown

What does this PR do?

The Qwen3-VL vision RoPE helper converts the entire concatenated Q/K sequence to FP32 and builds full-sequence FP32 products before restoring the input dtype. For long image/video inputs, those temporary tensors grow with the total number of visual tokens.

For the standard (tokens, heads, head_dim) layout under no_grad, process sequences longer than 4096 rows in chunks and write the results into output tensors with the original dtypes. Each chunk keeps the existing FP32 arithmetic and elementwise operation order. The dense path remains for gradient-enabled execution, short sequences, and other broadcast layouts.

The change is defined in the Qwen3-VL modular source. Regenerated copies also update Qwen3-VL-MoE, Qwen3.5, Qwen3.5-MoE, Qwen3-Omni-MoE, Qwen4-Exp, and Cohere-Compass; their helper implementation is identical. No new dependency is required.

Validation

  • Ran the new Qwen3VLVisionRotaryEmbeddingTest class in isolation on CPU with real PyTorch 2.14 and the modified repository module: exact output equality against the unchanged dense Qwen2-VL helper for FP16/BF16, contiguous/noncontiguous inputs, and lengths 4095/4096/4097; gradient equality for the dense training fallback; and FP32 intermediate tensor row bounds through TorchDispatchMode.
  • The scratch-bound regression fails against the helper extracted from the fixed base commit and passes with this change.
  • torch.compile(..., backend="eager", fullgraph=True, dynamic=True) captures the changed helper for lengths 4097 and 8193 and matches dense outputs. This checks Dynamo fullgraph capture only; Inductor/CUDA were not tested.
  • Regenerated the modular dependency graph and checked that generated changes are confined to the RoPE helper. Focused Ruff lint/format checks pass for the changed files.

The repository command for these tests is pytest tests/models/qwen3_vl/test_modeling_qwen3_vl.py -k Qwen3VLVisionRotaryEmbeddingTest. It was not fully collected in this partial local checkout because unrelated test imports were unavailable; the new unit class was executed directly from that test file. make style was attempted but its repository-wide targets include absent directories in this partial checkout. No GPU peak-memory, latency, encoder/model integration (including the derived models), or CUDA backend results are claimed. This is a draft pending those checks and author review.

Related work checked: #48105 changes rotary frequency layout, and #47703 changes control-tensor synchronization; neither implements this row-chunked helper.

Code Agent Policy

This draft and its code were prepared with a code agent at the author's explicit request. The author has not yet reviewed this new diff line by line or personally run the reported checks. No maintainer approval is claimed.

  • (First-time contributors only): I confirm that this PR description and code is not written by an LLM or code agent

Before submitting

  • Read the contributor guideline and relevant modular-model instructions.
  • Discussed/approved via a GitHub issue or forum thread.
  • Added focused regression coverage for the changed helper.
  • Author review and GPU/model integration validation completed.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: LeslieWylie <112309453+LeslieWylie@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

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

run-slow: cohere_compass, qwen3_5, qwen3_5_moe, qwen3_omni_moe, qwen3_vl, qwen3_vl_moe, qwen4_exp

@github-actions

Copy link
Copy Markdown
Contributor

Thank you for your contribution 🤗!

CI Security Gate — automatic approval blocked

This PR was not automatically approved for CI because the security gate failed.

Possible reasons:

  • The PR touches 50 or more files — only PRs with fewer than 50 changed files are automatically approved
  • A changed file is outside the allowed directories (src/, tests/, docs/, utils/), has a disallowed extension (only .py, .txt, .md permitted outside tests/ and docs/), or is not .md/.yml inside docs/ — this covers files the PR deletes or renames, not only the ones it edits
  • A new security issue was detected in the changed Python files (Bandit check) — a high-severity one, shell execution, exec/eval, a URL fetch, or disabled TLS verification
  • The PR touches a path this repository protects from untrusted PRs, such as the file that decides who reviews it — a maintainer must make that change in a separate PR

See the workflow run for the exact violations.

A maintainer can review and manually approve CI if a finding is a false positive.

@YaoleWu
YaoleWu marked this pull request as ready for review October 19, 2026 10:54
@zucchini-nlp

zucchini-nlp commented Oct 19, 2026 •

Copy link
Copy Markdown
Member

looks like code ai slop 😢
can you summarize the goal of PR in short format if not AI

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.

2 participants