Skip to content

npu: Qwen2.5-VL's contiguous "mrope" is a position layout, so its text tower loads and answers (Refs #1786) - #2261

Merged
bong-water-water-bong merged 1 commit into
mainfrom
cl-flm/qwen25vl-mrope
Nov 12, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
cl-flm/qwen25vl-mrope

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

What

FastFlowLM's Qwen2.5-VL-3B-Instruct-NPU2 was refused at load: config.json declares rope scaling "mrope", which this forward does not implement. Refs #1786; host-side only, the NPU is off. It follows #2259, which fixed the same Qwen2 text model's q/k/v biases.

Qwen2.5-VL's rope_scaling: {"type": "mrope", "mrope_section": [16, 24, 24]} is not a frequency scaling. It is a position layout: plain RoPE frequencies, each taking its position from the t, h or w plane.

  • Qwen2-VL and Qwen2.5-VL lay the sections out contiguously: transformers' apply_multimodal_rotary_pos_emb splits cos by mrope_section * 2, and chunk k takes plane k % 3. That makes frequencies 0–15 t, 16–39 h and 40–63 w.
  • That is not Qwen3-VL's interleave, which the forward already reads (q4nx_mrope_axis).

Changes:

  • mrope_sectioned (semantics) and q4nx_mrope_axis_sectioned. mrope_plane() picks the rule by layout.
  • The forward accepts "mrope" only when its three sections sum to the rotary half-width, and otherwise refuses by name. The startup line names the sections.
  • Qwen3-VL's interleaved form is unchanged.

Evidence (host, 2026-11-12)

Paris prompt, the pack's own chat template
before (main) refused at load
after `Paris<

On a text-only prompt the three planes are equal, so this is plain RoPE. The vision tower is not loaded here; it has its own item.

Tests

  • q4nx_mrope_test, +6 checks:
    • the type: mrope spelling is read with its sections;
    • all 64 frequencies match the reference's own mrope_section * 2 split;
    • the run boundaries 15/16/39/40;
    • it differs from the interleaved layout;
    • Qwen3-VL stays interleaved.
  • New CI arm batch_check_state_qwen2vl: npu: Qwen2's q/k/v biases are read from the container, so Qwen2.5-3B-Instruct-NPU2 answers (Refs #1786) #2259's fabricated Qwen2 (q/k/v biases, no attention_bias key) declaring this block. Its logits match the numpy plain-RoPE Qwen2 reference: corr 1.0000000, max |d| 6.6e-06. With "mrope" refused again (main's behaviour) the arm fails (init refused).
  • Full ctest: 268/268, device tests excluded. Every batch_check_state_ci.py arm (qwen3, lfm2, lfm3, qwen3_5, qwen2, qwen2vl) was run by hand: 0 FAIL. tools/copyright.py --check rc 0.
  • These ran on a tree identical to this head outside scripts/ (scripts: usage texts stop leaking code, and hw-gate's no-argument path stops taking a GPU ticket #2258 landed between the run and the rebase; git diff outside scripts/ is empty).

🤖 Generated with Claude Code

…t tower loads and answers (Refs #1786)

Qwen2.5-VL-3B-Instruct-NPU2 was refused at load: rope_scaling {"type": "mrope", "mrope_section":
[16, 24, 24]} was read as a frequency scaling this forward lacks. It is the Qwen2-VL position layout:
plain frequencies, CONTIGUOUS sections (transformers apply_multimodal_rotary_pos_emb splits cos by
mrope_section * 2), t 0-15, h 16-39, w 40-63 -- not Qwen3-VL's interleave. New mrope_sectioned and
q4nx_mrope_axis_sectioned; a block whose sections do not sum to the rotary half-width is refused.
With the attention-bias fix of the previous commit the text tower answers 'Paris<|im_end|>' on the host.

q4nx_mrope_test: +6 checks (all 64 frequencies against the reference's split; Qwen3-VL unchanged).
batch_check_state_qwen2vl: the fabricated Qwen2 declaring that block gives the plain-RoPE reference's
logits (corr 1.0000000); with "mrope" refused again it fails. ctest 268/268.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at 48a088d

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1786 - Partially compliant

Compliant requirements:

  • Every FastFlowLM/*-NPU2 model source imports and serves on --device npu
  • It imports with zero gaps and its self-check passes against a named reference
  • It serves on --device npu
  • It answers a known-answer prompt with the model's own chat template, 3 identical requests returning identical text

Non-compliant requirements:

  • Decode tok/s and time-to-first-token measured under npu-hold
  • The row names the kernel set and the engine commit that produced it

Requires further human verification:

  • Decode tok/s and time-to-first-token measured under npu-hold
  • The row names the kernel set and the engine commit that produced it
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect mrope handling

The code introduces handling for Qwen2.5-VL's "mrope" rope_scaling type, which is a position layout rather than a frequency scaling. However, the validation logic in init_impl checks that the sum of mrope_section values equals rotary_dim / 2, but this check is only performed when mrope_declared_ is true. If the model config declares "mrope" but the sections don't sum correctly, it should refuse loading, but the current logic might not catch all invalid configurations, especially if the sections are not properly initialized or if there's a mismatch in how the sections are interpreted.

if (mrope_declared_) {
    const int s3 = sem_.mrope_section[0] + sem_.mrope_section[1] + sem_.mrope_section[2];
    const int rot = sem_.rotary_dim > 0 ? sem_.rotary_dim : c_.HD;
    if (!sem_.mrope_sectioned || sem_.n_mrope_section != 3 || s3 != rot / 2) {
        err_ = "config.json declares rope scaling \"mrope\" and its mrope_section (" +
               std::to_string(sem_.n_mrope_section) + " values, summing to " + std::to_string(s3) +
               ") does not cover the " + std::to_string(rot / 2) + " rotary frequencies; the (t, h, w) "
               "layout would be a guess";
        return false;
    }
    fprintf(stderr, "  [semantics] mrope: contiguous sections t=%d h=%d w=%d (Qwen2-VL form)  <- "
                    "rope_scaling.type \"mrope\"\n",
            sem_.mrope_section[0], sem_.mrope_section[1], sem_.mrope_section[2]);
}
Test coverage for mrope

The test script batch_check_state_ci.py now includes support for qwen2vl family models, but the test coverage for the new mrope handling is limited to the specific case of Qwen2.5-VL with sections [16, 24, 24]. It would be beneficial to add more comprehensive tests covering different mrope section configurations to ensure robustness.

lfm3 = family == "lfm3"
qwen2 = family in ("qwen2", "qwen2vl")
q35 = family == "qwen3_5"

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY 48a088d

This is the best-shaped test I have reviewed in this stretch, and it is in territory I have worked in, so I want to be specific about why.

It builds the reference from transformers' own rule instead of asserting the implementation against itself. The test constructs ref[i] from "cos split by mrope_section * 2, chunk k takes plane k % 3" and then requires agree == 64 across a 128-wide head's 64 frequencies. That is an independent oracle: a wrong split cannot agree with it, and a refactor that "simplifies" the sections cannot keep agreeing by construction.

It pins the boundaries, not just the middle. axis(15) == 0, axis(16) == 1, axis(39) == 1, axis(40) == 2 — the three exact transitions for [16, 24, 24], which is where an off-by-one in a contiguous split actually lives. A test that only checked the counts would pass with every frequency shifted by one.

And it proves the two layouts are distinguishable: differ > 0 against q4nx_mrope_axis (the interleaved form). That matters more than it looks — Qwen3-VL interleaves its sections and Qwen2/2.5-VL do not, so a forward that accepted both would either mis-place one family's positions (producing plausible, wrong answers) or refuse one at load. The test asserts the implementations disagree where the families disagree, and separately that qwen3_vl still reads as interleaved (mrope_sectioned == 0).

The validation is in the right place — a block whose three sections do not sum to the rotary half-width is refused rather than padded or truncated, which is the same conservative choice the gemma3 linear-without-a-factor path makes.

One note for the docs, since it connects two checks that already exist: this test pins which axis each frequency takes, while the Qwen3-VL MRoPE work verified the positions against HF's get_rope_index (14 of 14 cases). Together they cover layout and position source; a line in docs/npu.md naming both would tell the next person which one to extend when a new MRoPE family arrives, instead of rediscovering that the two properties are separately tested.

Not verified by me, as with the rest of this series: the pack itself on a device — the NPU is off. Everything above is host-only and runs without one.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Hash correction for the ping channel only: my ready-message named 48a089d, which is not this PR's head — it is 48a088d. The verdict itself was written against the right commit; this note exists because the merge procedure matches the seven hex characters exactly and a typo would be skipped rather than noticed.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 48a088d — the distinction it draws is the one that matters: mrope is a position layout, not a frequency scaling, and there are two layouts of it.

Not a scaling, and not the one already implemented

"Qwen2.5-VL's rope_scaling: {"type": "mrope", "mrope_section": [16, 24, 24]} is not a frequency scaling. It is a position layout… Qwen2-VL and Qwen2.5-VL lay the sections out contiguously… That is not Qwen3-VL's interleave, which the forward already reads (q4nx_mrope_axis)."

That is a genuinely dangerous ambiguity handled correctly: the same key (mrope) names two different position layouts depending on the family, and the forward already implements one of them. Applying Qwen3-VL's interleave to a Qwen2.5-VL pack would produce plausible but wrong positions — the failure mode that survives a correlation check on short prompts.

The resolution is the right shape: mrope_sectioned in the semantics layer and q4nx_mrope_axis_sectioned in the forward, with mrope_plane() picking the rule by layout — one decision point, named, rather than a flag threaded through the code.

And the precondition is checked, with a named refusal

"The forward accepts "mrope" only when its three sections sum to the rotary half-width, and otherwise refuses by [name]."

[16, 24, 24] = 64, so the rotary half-width must be 64 — a checkable invariant, and refusing by name rather than silently mis-copying is what turns a future pack with [16, 16, 16] into a one-line diagnosis instead of a wrong answer.

Two asks

  1. say how the check reports — is the refusal message mrope_section sums to 56, expected 64 (rotary half-width)? A refusal that names the three numbers and the expected total is what makes the next pack's failure self-explanatory;
  2. state the device consequence, since this is host-only: Q4NX_LAYER_*/rope code shared with the NPU path may or may not inherit mrope_sectioned. One line saying whether the device forward refuses mrope by name (and until which slice) keeps the family's status from reading as done — the same point as npu: the per-op forward reads LFM3, so LFM3-1.7B-NPU2 loads and answers (Refs #1786) #2254's LFM3 ask.

Why this family of PRs is going well

#2242 (Gemma3's three config facts), #2254 (LFM3's four rules), #2259/#2261 (Qwen2.5-VL's biases then its position layout): each lands with the reference rule named, the borrowed rule's previous home stated (Gemma4's proportional RoPE, Qwen3-VL's interleave), and a refusal that names the offending value. That combination is what makes a growing model matrix reviewable at this rate.

@bong-water-water-bong
bong-water-water-bong merged commit c27b40d into main Nov 12, 2026
10 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the cl-flm/qwen25vl-mrope branch November 12, 2026 07:45
This was referenced Nov 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant