Skip to content

[Model] Add opt-in HF-numerics mode for the Qwen3-Omni thinker - #8186

Merged
amy-why-3459 merged 9 commits into
vllm-project:mainfrom
cr-gao:feat/thinker-hf-numerics
Oct 27, 2026
Merged

amy-why-3459 merged 9 commits into
vllm-project:mainfrom
cr-gao:feat/thinker-hf-numerics

Conversation

@cr-gao

@cr-gao cr-gao commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

Opt-in hf_numerics mode for the Qwen3-Omni thinker stage, for true on-policy RL (verl-project/verl-omni#637, the rollout-side piece of that split). With additional_config: {hf_numerics: true} the text decoder computes RMSNorm, the residual add, rotary embedding and MoE routing op for op as the transformers implementation does, and runs the experts through vLLM's fused_experts, which the trainer registers as the experts implementation of the HF model (transformers ExpertsInterface). Under VLLM_BATCH_INVARIANT=1 fused_experts gives the same rows for the rollout's decode batch and the trainer's packed batch, so an HF trainer running under batch-invariant kernels recomputes the rollout log-probs bitwise instead of within a tolerance. Attention, dense GEMMs, embedding and LM head are unchanged: they already match HF bitwise under VLLM_BATCH_INVARIANT=1.

The rotary frequencies are an elementwise product of positions and inv_freq, as in transformers; a matmul there would go through vLLM's batch-invariant bmm override, which runs fp32 at TF32 precision. inv_freq is computed on the CPU, as transformers does: vLLM builds the model under a CUDA default device, and the CUDA pow differs from the CPU one in the last bit for 2 of the thinker's 64 frequencies, which flips bf16 cos/sin from position 459 on.

Earlier revisions ran the experts through transformers' grouped_mm path to mirror the trainer. Profiling that revision (nsys, 48-layer thinker, vLLM 0.28.0) put 79% of the mode's rollout cost on that kernel; sharing fused_experts in both directions removes it and the torch._grouped_mm dependency, including the earlier SM90/SM100-only CUDA-graph restriction.

Each replaced op is a torch.library custom op with a fake kernel, so the mode works under torch.compile and CUDA graphs. Wiring: Qwen3MoeLLMModel passes HFNumericsDecoderLayer as the stock decoder_layer_type; the layer and the model construct their norms and the rotary module as the HF-numerics subclasses (weights are loaded by name afterwards). The MoE block keeps the stock instance and only its class is switched to the forward-only subclass, because the stock layer constructor builds the block itself; vllm-project/vllm#58859 adds a moe_block_type argument to Qwen3MoeDecoderLayer and removes that swap once picked up. Weight loading is untouched (the MoE op reads w13_weight / w2_weight in place). Off by default; no change to the default path.

Precedent: SGLang --rl-on-policy-target fsdp. Not covered by #4864 / #5512 / #5935 / #6166, which are diffusion-side.

Test Plan

Everything below is on this head (8787420cfc) with vLLM 0.32.0, torch 2.13.0+cu130, transformers 5.19.0 (5.18.0 where noted), H20 GPUs, unless marked historical.

  • Unit tests: pytest tests/model_executor/models/qwen3_omni/test_hf_numerics.py. Each op vs the transformers module at rtol=atol=0 (fp32/bf16, CPU/CUDA, 1-D/3-D positions); rotary under VLLM_BATCH_INVARIANT=1; inv_freq built under a CUDA default device vs transformers; MoE op vs the HF router followed by fused_experts (bitwise) and vs the HF grouped_mm block (tolerance); fused_experts rows under VLLM_BATCH_INVARIANT=1 for chunks 1/8/32 vs the whole batch; CUDA-graph capture/replay vs eager.
  • Rollout vs trainer forward, full model. Model: the 48-layer thinker of Qwen3-Omni-30B-A3B-Instruct (its thinker.* weights, unchanged). Rollout: the thinker stage on 1×H20, hf_numerics: true, VLLM_BATCH_INVARIANT=1, TP=1, CUDA graphs (FULL_AND_PIECEWISE), max_num_seqs=8; 4 concurrent requests (prompts of 10/16/16/24 tokens), T=1, top-p 1, top-k −1, ignore_eos, 1000 tokens each, plus seq0 alone. Trainer side: transformers Qwen3OmniMoeForConditionalGeneration.thinker on another H20 under init_batch_invariance(), attn_implementation=hub_fa3 and experts_implementation=vllm_fused_moe from verl-omni (hub_fa3_attention.py, vllm_fused_moe_experts.py), teacher-forced on the sampled tokens one sequence at a time and packed 4 together. The sampled tokens' log-probs are compared element by element. The negative control changes only the trainer's experts to transformers' default grouped_mm; with routing, attention and every other op still shared, any divergence it shows comes from expert execution.
  • Real RL closed loop through verl. Trainer: verl fefb0802 (verl-omni's pin) with [fsdp, trainer, cfg, doc, tests] feat: batch-invariant actor forward and sampler log-prob formula verl-project/verl#8026 (6a7110b4) applied, verl-omni true_on_policy (branch at 279afe1), FSDP2 actor on 2×H20 with fsdp_config.batch_invariant=True and model_dtype=bfloat16; rollout = this head, CUDA graphs. The verl-omni true on-policy GSPO recipe with box-sized overrides: 16 prompts × n=2, prompt ≤1024 / response ≤1024, dynamic batches of 30720 tokens, synthetic prompts with a hash reward, experts frozen to fit 2 GPUs (attention, router, norms, embedding and LM head train). 5 steps, each with an optimizer update and a weight sync. Gate: verl training/rollout_logprobs_mismatch_count (rollout log-probs vs the actor's recompute after each sync, per token, bitwise) and actor/old_logprobs_mismatch_count (update forward vs recompute). Weight sync was checked by sha256 of selected tensors on the actor and both rollout replicas at each sync. A long-sequence variant forces every response to 1024 tokens (ignore_eos).
  • Throughput: the same loop without the digest hook, on (true_on_policy) vs off (true_on_policy=False, batch_invariant=False, hf_numerics off, the same hub FA3 attention), 5 steps each, mean of steps 2–5.
  • Ready online selector on the default path (hf_numerics off), full Qwen3-Omni-30B-A3B-Instruct from a local checkpoint, 3 stages on 3×H20: pytest -s -v tests/e2e/online_serving/test_qwen3_omni.py tests/e2e/online_serving/test_qwen3_omni_thinking.py -m core_model --run-level core_model.

Test Result

  • Unit tests: 28 passed (CUDA, H20, vLLM 0.32.0).
  • Rollout vs trainer forward (48 layers, CUDA graphs, 1000 tokens):
trainer experts rollout seq0 alone vs in batch of 4 HF single vs decode, seq0–3 HF packed-4 vs decode, seq0–3
vllm_fused_moe 0/1000 0/1000 ×4 0/1000 ×4
grouped_mm (negative control) 0/1000 993 / 991 / 1000 / 958 of 1000, first at token 0 same as single

Before the inv_freq fix the same probe gave 548/549/555/533 of 1000, every first mismatch at absolute position 460.

  • Closed loop, rollout_logprobs_mismatch_count / old_logprobs_mismatch_count per step:
run steps response tokens per step per step
transformers 5.19, recipe lengths (longest response 190), no digest hook 5 ≈3000–3900 0 / 0 at every step
transformers 5.19, every response 1024 tokens 5 32768 0 / 0 at every step, diff_max 0
transformers 5.18, every response 1024 tokens 5 32768 0 / 0 at every step, diff_max 0

grad_norm was finite and nonzero at every step (0.40–1.39). In the two 1024-token runs the digests changed at every sync and matched between the actor and both rollout replicas.

  • Throughput, steps 2–5 (same model, hardware, workload and configuration in both arms): step 15.08 s off vs 16.68 s on (+10.6%), perf/throughput 140.5 vs 126.3 (−10.1%); the difference is in generation (2.53 s vs 4.53 s; verl reports timing_s/gen at 2 s granularity). This prices the whole mode, rollout and actor together, against a run without batch-invariant kernels.
  • Historical, vLLM 0.28.0 twin of this branch, engine-only benchmark (48-layer thinker, 1×H20, CUDA graphs, VLLM_BATCH_INVARIANT=1 in both arms, 32 × 512 tokens, median of 3), not re-measured on 0.32: off 1281.8 tok/s vs on 1112.5 (−13.2%) for the fused_experts revision, off and on in the same session; the grouped_mm revision measured −44.7% against its own same-session off arm on an earlier day.
  • Online selector (vLLM 0.32.0, this head): 11 passed, 14 skipped (rocm/npu variants), 9 deselected, 673 s, the mixed video+image+audio and the audio-output cases included.

AI assistance was used for this change. I reviewed every line and ran the tests.

@vllm-omni-review-bot

Copy link
Copy Markdown

This PR appears to belong to: docs/design/module/model_integration.md, docs/design/module/ar_runtime.md.

Module owners: @tzhouam @fake0fan @Gaohan123

Routing: @tzhouam via module of the changed files, CODEOWNERS; @fake0fan via module of the changed files; @Gaohan123 via module of the changed files

@cr-gao, 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.

@hsliuustc0106 hsliuustc0106 added omni code related to omni models enhancement New feature or request high priority high priority issue, needs to be done asap labels Sep 26, 2026

@0z5a 0z5a left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested head ebf6269ff073bb6ad25aa9b5e22d1fdea9bbf3bd on RTX 5090 (SM120), PyTorch 2.13.0+cu130, vLLM 0.29.0 and Transformers 5.10.4, with BF16, TP=1/EP=1 and VLLM_BATCH_INVARIANT=1.

The original 22 CPU/CUDA operator tests passed. The two inline findings come from additional execution-path tests against the original implementation, not from inspection alone:

Check Observed result
Actual FusedMoE loading, Triton, before and after process_weights_after_loading Both exactly match HF grouped MM; 0 differing elements.
Same loading probe, explicit FlashInfer CUTLASS Exact before post-load processing; afterwards 288/288 output elements differ, maximum absolute difference 2.078125.
Same loading probe, explicit auto resolving to FlashInfer CUTLASS Same post-load failure: 288/288 elements differ, maximum absolute difference 2.078125.
Loaded Triton block under torch.compile, three different inputs Eager and compiled outputs exactly match HF; repeated compiled execution agrees.
Same loaded Triton block under direct CUDA Graph capture after warm-up Capture fails inside HF grouped MM with a CPU/CUDA copy error.

Omni's default backend selection uses Triton; the layout finding concerns an explicit auto or CUTLASS selection. The graph finding is specific to the tested SM120/runtime combination and does not contradict the reported H20 result. Full-model numerical differences whose cause has not been isolated are not included as additional defects in this review. No code changes are being pushed as part of this review.

Comment on lines +167 to +172
experts = self.experts.routed_experts
out = torch.ops.vllm_omni.hf_moe(
hidden_states.view(-1, hidden_states.shape[-1]),
self.gate.weight,
experts.w13_weight,
experts.w2_weight,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Validate the post-load expert layout before using raw weights

These tensors belong to the selected FusedMoE backend and are not guaranteed to retain HF's [expert, gate/up, hidden] layout after weight post-processing. With explicit moe_backend="flashinfer_cutlass" (or "auto" resolving to it on SM120), this forward still treats the repacked tensors as ordinary HF matrices and silently produces incorrect values. The constructor currently accepts this configuration.

Reproduced with the real FusedMoEFactory loader: BF16, hidden size 32, 4 experts, top-k 2, intermediate size 8, 9 input tokens; load HF gate/up/down weights and compare this block to HF grouped_mm. Before experts.quant_method.process_weights_after_loading(experts), the outputs are bitwise equal. After that call, CUTLASS gives 288/288 differing output elements with maximum absolute difference 2.078125. Repeating the same probe with Triton remains exactly equal before and after processing. This isolates the failure to the post-load backend path, which the current tests bypass by passing HF-owned tensors directly to hf_moe.

Please constrain HF-numerics mode to a validated weight layout/backend at initialization, or retain a canonical expert-weight representation, and add a loader/post-load regression test.

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, reproduced and fixed. The layer now requires the unquantized Triton MoE backend at construction and raises with --moe-backend triton otherwise.

if norm_topk_prob:
top_value = top_value / top_value.sum(dim=-1, keepdim=True)
top_value = top_value.to(router_logits.dtype)
return grouped_mm_experts_forward(_Experts(w13, w2, hidden_act), x, top_index, top_value)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] Gate CUDA Graph support on the actual grouped-MM path

Wrapping this call in an opaque custom op does not make its internals graph-capturable. On the tested RTX 5090/SM120 runtime, HF grouped MM reaches PyTorch's per-expert fallback and CPU routing offsets. A direct capture of a warmed-up, loaded Triton MoE block fails at this line with RuntimeError: Cannot copy between CPU and CUDA tensors during CUDA graph capture unless the CPU tensor is pinned. The same block matches HF in eager execution and under torch.compile for three distinct inputs, so this is specifically a capture failure rather than a compile or weight-loading failure.

The PR documentation currently says the mode is captured into CUDA graphs unchanged and does not require enforce_eager, without restricting the hardware/runtime. Please validate this backend path and either provide a capture-compatible implementation or reject the unsupported graph configuration early with an actionable eager-mode message. Add capture/replay coverage on the supported path; a fake custom-op implementation only covers shape tracing.

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.

Done. With CUDA graphs on, construction now requires bf16 on SM90/SM100 (torch's native grouped_mm path) and asks for enforce_eager otherwise.

@cr-gao
cr-gao force-pushed the feat/thinker-hf-numerics branch from ebf6269 to aa99787 Compare September 28, 2026 00:10

@SamitHuang SamitHuang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The Triton-backend check and the SM90/SM100 CUDA-graph gate close the two issues from the earlier review. The op-level bitwise tests against the transformers modules hold for text. Two holes still break the on-policy contract this mode is for: visual deepstack is added on the wrong side of the residual split, and the MoE stub is missing an attribute that current transformers main already reads.

prefix=prefix,
decoder_layer_type=HFNumericsDecoderLayer if hf_numerics else Qwen3MoeDecoderLayer,
)
if hf_numerics:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

hf_numerics makes the layer's add+norm match HF, but the thinker forward below still does hidden_states = hidden_states + deepstack after layer() returns. In this split hidden_states is the MLP output and residual is the pre-MLP stream, so the next norm sees (mlp + deepstack) + residual. HF _deepstack_process adds the visual embed only after the layer has formed (residual + mlp), i.e. ((residual + mlp) + deepstack). Those bf16 associations differ on image/video tokens, so those rollouts will not recompute log-probs bitwise. The reported 0-mismatch prefill is a text sequence and does not catch this. Keep the default path as it is; when this flag is on, form the bf16 sum first, add deepstack into that sum, and feed that sum to the next norm.

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.

Fixed

has_bias = False
is_transposed = False
is_concatenated = True
_apply_gate = _default_apply_gate

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

grouped_mm_experts_forward on current transformers main reads self._is_expert_parallel unconditionally (the sentinel-mask path from huggingface/transformers#48201). This stub never sets it, so hf_moe raises AttributeError once that code is installed. requirements/common.txt is transformers >= 5.5.3 with no ceiling, and 5.10.3 simply does not touch the attribute, which is why the unit tests pass today. Set _is_expert_parallel = False on _Experts. The import is also unconditional from the thinker module, so a removal of this private helper would take down the default Qwen3-Omni path, not only this mode.

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.

Fixed

@cr-gao
cr-gao force-pushed the feat/thinker-hf-numerics branch from aa99787 to 07ed9d7 Compare September 30, 2026 03:39
@vllm-omni-review-bot

Copy link
Copy Markdown
Omni ReviewBot routing record

Assigned Direct under experiment vllm-omni-strict-5050-20260829.

@vllm-omni-review-bot vllm-omni-review-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Omni ReviewBot review

PR description

This PR adds an opt-in hf_numerics mode for the Qwen3-Omni thinker text decoder so rollout log-probs can match an HF trainer bitwise for on-policy RL. When additional_config.hf_numerics is true, RMSNorm, residual/deepstack adds, M-RoPE, and MoE routing/experts follow the transformers formulas via torch.library custom ops; attention, dense GEMMs, embeddings, and the LM head stay on the stock path. The mode is off by default, rejects unsupported MoE backends and non-capturable CUDA-graph setups at init, and documents the throughput cost as RL-only.

Change flow

flowchart TD
  cfg["[EXISTING] additional_config.hf_numerics"]:::existing --> model["[CHANGED] Qwen3MoeLLMModel init/forward"]:::changed
  model --> layer["[NEW] hf_numerics custom ops and layer wrappers"]:::new
  model --> deep["[CHANGED] deepstack add after residual merge"]:::changed
  layer --> out["[EXISTING] Bitwise HF log-prob recompute for RL"]:::existing
  deep --> out
  classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
  classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
  classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
  classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
Loading

CI at 07ed9d70c3c9 (2026-09-30T21:56:39.231900+00:00): required check(s) blocking: buildkite/vllm-omni (missing).

See inline comments below.


🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!

}
)
class Qwen3MoeLLMModel(_Qwen3MoeLLMModel):
def __init__(self, *, vllm_config: VllmConfig, prefix: str = ""):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P1] Path-triggered Qwen3-Omni ready e2e selector was not run

Changed production paths under vllm_omni/model_executor/models/qwen3_omni/ are listed in omni_qwen3_omni_function (.buildkite/common/ci_source_file_dependencies.yml). Ready step "Omni · Qwen3-Omni Test" (.buildkite/cuda/test-ready.yml) runs pytest -s -v tests/e2e/online_serving/test_qwen3_omni.py -m 'core_model' --run-level 'core_model'. The PR Test Plan/Result reports only tests/model_executor/models/qwen3_omni/test_hf_numerics.py and a custom truncated-thinker E2E, not that online_serving selector. The new __init__ always reads additional_config and passes an explicit decoder_layer_type, so default-path startup is in the blast radius. Please run the ready e2e command or explicitly state in the Test Plan that default-path online e2e was not covered. Unit-test coverage of the new file aligns with the always-on model_executor guards as a named subset.

@vllm-omni-review-bot

Copy link
Copy Markdown

Omni ReviewBot: finding feedback

[p1] Path-triggered Qwen3-Omni ready e2e selector was not run — vllm_omni/model_executor/models/qwen3_omni/qwen3_omni_moe_thinker.py:557

Changed production paths under vllm_omni/model_executor/models/qwen3_omni/ are listed in omni_qwen3_omni_function (.buildkite/common/ci_source_file_dependencies.yml). Ready step "Omni · Qwen3-Omni Test" (.buildkite/cuda/test-ready.yml) runs pytest -s -v tests/e2e/online_serving/test_qwen3_omni.py -m 'core_model' --run-level 'core_model'. The PR Test Plan/Result reports only tests/model_executor/models/qwen3_omni/test_hf_numerics.py and a custom truncated-thinker E2E, not that online_serving selector. The new __init__ always reads additional_config and passes an explicit decoder_layer_type, so default-path startup is in the blast radius. Please run the ready e2e command or explicitly state in the Test Plan that default-path online e2e was not covered. Unit-test coverage of the new file aligns with the always-on model_executor guards as a named subset.

This finding appears in the bot's COMMENT review, but GitHub could not place it as an inline diff comment. If you are the PR author and disagree, react 👎 to this comment. The disagreement will be shown to the maintainer; it does not approve or merge the PR.

@vllm-omni-review-bot

Copy link
Copy Markdown

Omni ReviewBot: no human activity for 7 days

@cr-gao this pull request has had no human commit, comment or review since 2026-09-30. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state.

To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline.

@cr-gao
cr-gao requested a review from SamitHuang October 8, 2026 04:18

@SamitHuang SamitHuang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@SamitHuang SamitHuang added ready label to trigger buildkite CI cuda-test Used to trigger vllm-omni cuda CI separately. labels Oct 9, 2026
@vllm-omni-review-bot

vllm-omni-review-bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Omni ReviewBot: superseded

The CI failure noted on 07ed9d70c3c9 refers to an earlier head; the pull request now points at cbbcbbeeb06e.

@cr-gao
cr-gao force-pushed the feat/thinker-hf-numerics branch from 07ed9d7 to cbbcbbe Compare October 10, 2026 04:17

@vllm-omni-review-bot vllm-omni-review-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Omni ReviewBot review

Changes since the previous review

  • 1 new inline finding(s); 0 finding(s) below.
  • Still needs attention: Path-triggered Qwen3-Omni ready e2e selector was not run — vllm_omni/model_executor/models/qwen3_omni/qwen3_omni_moe_thinker.py

CI at cbbcbbeeb06e (2026-10-10T04:23:20.739814+00:00): required check(s) blocking: buildkite/vllm-omni (missing), docs/readthedocs.org:vllm-omni (pending).

Full review analysis

PR description

This change adds an opt-in thinker mode, additional_config.hf_numerics, so a Qwen3-Omni rollout can recompute text-decoder log-probs with the same RMSNorm, residual and deepstack adds, M-RoPE, and MoE block as Transformers. The thinker model selects HFNumericsDecoderLayer only when the flag is set, swaps in matching norm and rotary modules, and replaces the MoE forward while leaving loaded Triton expert weights in place. The flag defaults off. Attention, dense GEMMs, the embedding, and the LM head stay on the existing kernels. CUDA graphs are allowed only for bf16 on SM90/SM100; other backends and graph setups fail at construction.

Change flow

flowchart LR
  cfg["[EXISTING] thinker additional_config"]:::existing
  thinker["[CHANGED] Qwen3MoeLLMModel init and deepstack"]:::changed
  layer["[NEW] HFNumericsDecoderLayer"]:::new
  ops["[NEW] hf_rms_norm, hf_mrope, hf_moe ops"]:::new
  result["[CHANGED] opt-in HF residual stream"]:::changed
  cfg --> thinker
  thinker --> layer
  layer --> ops
  ops --> result
  classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
  classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
  classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
  classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
Loading

See inline comments below.


🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!

)
class Qwen3MoeLLMModel(_Qwen3MoeLLMModel):
def __init__(self, *, vllm_config: VllmConfig, prefix: str = ""):
hf_numerics = bool(vllm_config.additional_config.get("hf_numerics", False))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P1] Ready Qwen3-Omni online selector was not run

Evidence and suggested fix

Changed files under vllm_omni/model_executor/models/qwen3_omni/ are in omni_qwen3_omni_function (.buildkite/common/ci_source_file_dependencies.yml). Ready step "Omni · Qwen3-Omni Test" (.buildkite/cuda/test-ready.yml) runs pytest -s -v tests/e2e/online_serving/test_qwen3_omni.py tests/e2e/online_serving/test_qwen3_omni_thinking.py -m 'core_model' --run-level 'core_model'. Qwen3MoeLLMModel.__init__ always reads additional_config and passes an explicit decoder_layer_type, so default-path startup is in the blast radius. The Test Plan reports tests/model_executor/models/qwen3_omni/test_hf_numerics.py and a truncated 4-layer thinker comparison, which is a different selector, and it does not state that this online core_model command was not run. Please run that command or record the omission in the Test Plan. The new unit file does match the always-on tests/model_executor core_model CPU/CUDA sweeps as a subset.

@SamitHuang SamitHuang added ready label to trigger buildkite CI cuda-test Used to trigger vllm-omni cuda CI separately. and removed ready label to trigger buildkite CI cuda-test Used to trigger vllm-omni cuda CI separately. labels Oct 21, 2026
@vllm-omni-review-bot

vllm-omni-review-bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Omni ReviewBot: superseded

The CI failure noted on c3af0f8cf499 refers to an earlier head; the pull request now points at d43438ac8ed4.

Signed-off-by: cr-gao <gaochenrui@sjtu.edu.cn>
Keep the HF router (fp32 softmax, top-k, renormalise, cast) and hand the
experts to vLLM's fused_experts, which a trainer registers as its experts
implementation. Drops the grouped_mm stub and the SM90/SM100-only CUDA
graph restriction.

Signed-off-by: cr-gao <gaochenrui@sjtu.edu.cn>
@cr-gao

cr-gao commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

@SamitHuang @hsliuustc0106 I recently tested the E2E RL throughtput (together with the modifications on verl and verl-omni) of the new numeric path and find that the HF-side grouped_mm MoE kernel contributed to a large drop of the throughput. So I pushed a new commit that removed the HF MoE in the new numerics path and will register vLLM fused_moe on verl-omni side instead. Pls take a look, thanks!

@amy-why-3459 amy-why-3459 added ready label to trigger buildkite CI and removed ready label to trigger buildkite CI labels Oct 22, 2026
@vllm-omni-review-bot

vllm-omni-review-bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Omni ReviewBot: superseded

The CI failure noted on 73b24dac9a4d refers to an earlier head; the pull request now points at 8787420cfc26.

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@cr-gao, reviewed the six-file change at 73b24dac9a4d38a09869d1219a3c9baaa94d54b9. The latest fused-experts replacement needs its numerical and performance evidence attributed to the corresponding implementation:

  • Please link the reproducible trainer/rollout invocation and result covering this replacement in the supported environment, or identify precisely which existing artifacts remain applicable. The Test Plan still identifies Omni 611e63d629/vLLM0.31, and full-model graph evidence comes from a vLLM0.28 twin branch, whereas this head targets vLLM0.32. Include model/checkpoint, trainer commit and ExpertsInterface implementation, software/hardware, batch-invariance and attention/kernel settings, inputs, and the log-prob comparison after weight synchronization. Acceptance: the corresponding trainer and rollout agree on the claimed path; explain how the test would expose mismatched expert execution rather than relying only on isolated operator passes.
  • For the throughput motivation, identify the exact grouped_mm baseline and fused-experts candidate behind the comparison, with matched model, hardware, workload and configuration; otherwise clearly label the older numbers as historical. Existing reproducible artifacts are sufficient if their revision/environment correspondence is established.

This is an evidence-attribution request, not a demonstrated code defect. Source inspection completed; no GPU tests or benchmarks executed here.

Under VLLM_BATCH_INVARIANT the frequency matmul goes through the
batch-invariant bmm override, which runs fp32 at TF32 precision and
breaks bitwise agreement with Transformers. Multiply positions and
inverse frequencies elementwise, as Transformers does.

Signed-off-by: cr-gao <gaochenrui@sjtu.edu.cn>
vLLM builds the model under a CUDA default device, while Transformers
computes inv_freq on the CPU. The CUDA pow differs from the CPU one in
the last bit for some frequencies at the thinker's head_dim and
rope_theta, which flips bf16 cos/sin at long positions. Compute it on
the CPU and move it to the default device.

Signed-off-by: cr-gao <gaochenrui@sjtu.edu.cn>
@cr-gao

cr-gao commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

@hsliuustc0106 Updated the description: all evidence now targets this head on vLLM 0.32 (48L probe with a grouped_mm negative control, 5-step closed loops incl. 1024-token responses, matched on/off E2E throughput, the ready online selector); the 0.28 numbers are marked historical. The head also adds two rope fixes found in that testing.

@SamitHuang SamitHuang added ready label to trigger buildkite CI cuda-test Used to trigger vllm-omni cuda CI separately. and removed ready label to trigger buildkite CI cuda-test Used to trigger vllm-omni cuda CI separately. labels Oct 23, 2026

@linyueqian linyueqian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 8787420. Main CUDA is red at this head (build 17866) because of two failures this change introduces, both inline: an existing deepstack test that builds the model without __init__ now hits the new hf_numerics attribute, and the float32 fused-experts test exceeds L4 shared memory. The opt-in gating looks contained otherwise, since the default path keeps Qwen3MoeDecoderLayer and the plain deepstack add. The evidence attribution that hsliuustc0106 asked for at 73b24da is still open.

Static review only; I did not run the tests locally.


if deepstack_input_embeds is not None and layer_idx in range(0, len(deepstack_input_embeds)):
hidden_states = hidden_states + deepstack_input_embeds[f"deepstack_input_embeds_{layer_idx}"]
if self.hf_numerics:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[blocking] forward now reads self.hf_numerics, but tests/model_executor/models/qwen3_omni/test_qwen3_omni_deepstack_device.py builds the model with object.__new__(Qwen3MoeLLMModel), so all four test_deepstack_buffers_use_execution_device cases fail with AttributeError: 'Qwen3MoeLLMModel' object has no attribute 'hf_numerics' (Simple · Model Executor Test, build 17866). Setting model.hf_numerics = False in that test helper fixes it; a class-level hf_numerics = False default would also cover other __new__-built stubs.


@hardware_test(res={"cuda": "L4"}, num_cards=1)
@_requires_cuda
@pytest.mark.parametrize("dtype", _DTYPES)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[blocking] The float32 case of this test fails on the L4 runner it is pinned to: fused_experts picks a Triton config that needs 122880 bytes of shared memory against the L4 limit of 101376 (OutOfResources, Engine&Model Executor&Work Test · L4, build 17866). Skipping float32 on GPUs with less shared memory, or passing a smaller explicit config for that dtype, should keep the HF-router check within the hardware limit.

@vllm-omni-review-bot

vllm-omni-review-bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Omni ReviewBot: superseded

The CI failure noted on 8787420cfc26 refers to an earlier head; the pull request now points at 0fce417d5ce7.

The deepstack device test builds Qwen3MoeLLMModel without __init__, so
forward now needs the hf_numerics attribute set on the stub. The fp32
case of the HF-router fused_experts test needs more shared memory than
an L4 has; keep it to bf16.

Signed-off-by: cr-gao <gaochenrui@sjtu.edu.cn>
@cr-gao

cr-gao commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

@linyueqian Thanks, both fixed in 0fce417: the deepstack test stub now sets hf_numerics = False, and the HF-router fused_experts test runs bf16 only. The evidence attribution hsliuustc0106 asked for is in the updated description (all on this head with vLLM 0.32).

@SamitHuang SamitHuang added ready label to trigger buildkite CI cuda-test Used to trigger vllm-omni cuda CI separately. and removed cuda-test Used to trigger vllm-omni cuda CI separately. ready label to trigger buildkite CI labels Oct 26, 2026

@linyueqian linyueqian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @cr-gao. Re-reviewed 0fce417. Both blocking points from my last review are fixed: the deepstack test stub now sets hf_numerics = False, and the HF-router fused_experts test runs bf16 only, so it stays within the L4 shared-memory limit. The new commit only touches those two test files, so the evidence in the description (measured at 8787420 on vLLM 0.32, with the 0.28 numbers marked historical) still applies to this head, and the default path is unchanged since hf_numerics is opt-in.

The fp32 case of the other test_hf_numerics.py tests is still covered; only the fused-experts router check dropped fp32, which matches the bf16 trainer setup in the description.

Static review of the delta; I did not run the tests. Main CUDA is green at this head and the branch merges cleanly with current main.

LGTM.

@amy-why-3459
amy-why-3459 merged commit bf48366 into vllm-project:main Oct 27, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cuda-test Used to trigger vllm-omni cuda CI separately. enhancement New feature or request high priority high priority issue, needs to be done asap omni code related to omni models ready label to trigger buildkite CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants