Skip to content

[Qwen4] validate FP8 PLE weight scale after loading - #54722

Merged
Isotr0py merged 1 commit into
vllm-project:mainfrom
peakcrosser7:qwen4/validate_weight_scale
Sep 2, 2026
Merged

Isotr0py merged 1 commit into
vllm-project:mainfrom
peakcrosser7:qwen4/validate_weight_scale

Conversation

@peakcrosser7

@peakcrosser7 peakcrosser7 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Purpose

Fix a fail-open risk in the Qwen3.8 Flash Next FP8 PLE weight loading path.

If ngram_embedding.weight_scale is missing or incorrectly mapped, the registered parameter could remain uninitialized without being detected. This PR:

  • Stores the BF16 PLE global scale as FP32 so the existing sentinel initialization applies.
  • Validates the sentinel in process_weights_after_loading() after all checkpoint shards have been processed.

Thanks to @Dev-Jahn for identifying and reporting this issue.

Not a duplicate

Searched the open PRs for Qwen3.8 PLE weight_scale and FP8 PLE scale. I did not find an open PR addressing this missing post-load validation.

Test Plan

python -m pytest -q tests/models/qwen4_exp/test_ple.py

pre-commit run --files \
  vllm/models/qwen4_exp/nvidia/ple_layer.py \
  tests/models/qwen4_exp/test_ple.py

git diff --check

Test Result

  • Unit tests: 11 passed.
  • Pre-commit hooks: all passed.
  • git diff --check: passed.
  • Missing scale now raises ValueError during post-load processing.

AI assistance was used for code analysis, implementation, and test drafting. I reviewed the resulting changes and test results.


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • [ x The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.

Signed-off-by: huanghaoyan.hhy <huanghaoyan.hhy@alibaba-inc.com>

@claude claude 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.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@mergify mergify Bot added the qwen Related to Qwen models label Sep 1, 2026
@Dev-Jahn

Dev-Jahn commented Sep 1, 2026

Copy link
Copy Markdown

Looks right to me, thanks for turning it around quickly. Storing the scale as FP32 puts it on the path where create_fp8_scale_parameter applies its finfo.min sentinel, and the process_weights_after_loading check turns the old silent case into a load-time error; numerics are unchanged since the BF16 checkpoint value round-trips exactly through FP32 and the dequant already upcasts. The load_scale=False test pins the failure case.

One forward-looking note for the #53899 rebase: under VLLM_PLE_CPU_OFFLOAD the GPU process skips this method's create_weights, so this check will run only in the offload worker. The GPU-side retention path (load_weights keeping _offload_weight_scale) fails closed today through the dequant-time RuntimeError, so nothing is wrong, but adding the same post-load check to that branch would move the failure to startup there too.

@Isotr0py
Isotr0py enabled auto-merge (squash) September 2, 2026 02:52
@Isotr0py

Isotr0py commented Sep 2, 2026

Copy link
Copy Markdown
Member

/ci run

@github-actions github-actions Bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #86761 for commit 0d084422a50e.

@Isotr0py
Isotr0py merged commit 1e30089 into vllm-project:main Sep 2, 2026
75 of 76 checks passed
Trosfy added a commit to Trosfy/vllm that referenced this pull request Sep 2, 2026
Reconcile the PLE mmap series with the fused PLE kernels merged by
vllm-project#54517 and the FP8 scale validation from vllm-project#54722. Keep upstream's new
ops.ple dispatch (ple_ngram_ids, ple_gate, ple_conv) and its removal of
the NVIDIA positions_buffer/padded_buffer registrations and the
max_total_tokens/max_num_reqs constructor parameters, then layer the
mmap delta onto those seams.

Resolves four hunks in ple_layer.py and model.py: union of the ple_mmap
and ops.ple imports, the mmap embedding branch without the deleted
buffers, the mmap short-circuit in forward without the duplicated
ngram_ids allocation upstream removed, and the _HC_WEIGHTS_MAPPER to
_EXTRA_WEIGHTS_MAPPER rename around the retained reload preflight.

Adapt test_ple_mmap.py to the new Qwen4ExpNGramEmbedding signature. The
CPU-driven equivalence test now copies compute_ngram_ids' result into the
op's output buffer itself: the fused CUDA kernel writes in place, but the
eager fallback returns a fresh tensor and leaves output untouched.

Signed-off-by: Christopher Deodat <61226342+Trosfy@users.noreply.github.com>
Assisted-by: Claude Code (AI assistant)
vowstar added a commit to vowstar/vllm-sm80 that referenced this pull request Sep 2, 2026
The bulk of the migration (nvidia +798, amd +201). Brings in the FP8 PLE
path, per-shard weight_scale handling and validation, and the short-conv
corrections from peakcrosser7/vllm release/qwen38next_offload.

Our _short_conv_fallback survives unchanged, comment included, and gains a
second guard: their version also falls back when the per-layer metadata is
missing, not only when attn_metadata itself is None. That path is profiling
and cudagraph capture, not an sm_80 gate -- worth stating because the word
fallback made it look like one.

This file also carries the change merged upstream as vllm-project/vllm
vllm-project#54722, [Qwen4] validate FP8 PLE weight scale after loading, merged
2026-09-02 by the same author. Taking their branch subsumes it.

After this the whole qwen4_exp tree matches theirs except two deliberate
divergences: the PP=1 RuntimeError we reject (see 0f3ce9d) and a docstring
re-indentation of theirs that is a typo.
mylibrar pushed a commit to tanyuqian/vllm that referenced this pull request Sep 3, 2026
Signed-off-by: huanghaoyan.hhy <huanghaoyan.hhy@alibaba-inc.com>
vowstar added a commit to vowstar/vllm-sm80 that referenced this pull request Sep 3, 2026
The bulk of the migration (nvidia +798, amd +201). Brings in the FP8 PLE
path, per-shard weight_scale handling and validation, and the short-conv
corrections from peakcrosser7/vllm release/qwen38next_offload.

Our _short_conv_fallback survives unchanged, comment included, and gains a
second guard: their version also falls back when the per-layer metadata is
missing, not only when attn_metadata itself is None. That path is profiling
and cudagraph capture, not an sm_80 gate -- worth stating because the word
fallback made it look like one.

This file also carries the change merged upstream as vllm-project/vllm
vllm-project#54722, [Qwen4] validate FP8 PLE weight scale after loading, merged
2026-09-02 by the same author. Taking their branch subsumes it.

After this the whole qwen4_exp tree matches theirs except two deliberate
divergences: the PP=1 RuntimeError we reject (see 1e0d517) and a docstring
re-indentation of theirs that is a typo.
sheralskumar pushed a commit to sheralskumar/vllm that referenced this pull request Sep 8, 2026
Signed-off-by: huanghaoyan.hhy <huanghaoyan.hhy@alibaba-inc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

qwen Related to Qwen models ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants