Skip to content

[Quantization] Anchor should_convert_module regex to stop prefix-matching siblings - #47990

Open
Ilya-a-sergeyev-ger wants to merge 1 commit into
huggingface:mainfrom
Ilya-a-sergeyev-ger:fix-fp8-should-convert-prefix-match
Open

Ilya-a-sergeyev-ger wants to merge 1 commit into
huggingface:mainfrom
Ilya-a-sergeyev-ger:fix-fp8-should-convert-prefix-match

Conversation

@Ilya-a-sergeyev-ger

@Ilya-a-sergeyev-ger Ilya-a-sergeyev-ger commented Aug 15, 2026 •

Copy link
Copy Markdown

CI

What

should_convert_module in src/transformers/quantizers/quantizers_utils.py matches each modules_to_not_convert entry against the module's full name with an unanchored re.match(f"{key}", full_name).

Because re.match anchors at the start but not the end, a literal pattern such as ...mlp.gate also matches ...mlp.gate_proj. When gate_proj is not supposed to be converted, the result is inverted: gate_proj gets skipped when gate was the intended target (or vice versa), so the quantizer leaves it as a plain nn.Linear while its weight is FP8 — a BF16 × FP8 matmul then dies at forward with mat1 BFloat16 != mat2 Float8_e4m3fn.

Repro

Real checkpoint: Qwen/Qwen3.8-27B-FP8 (quant_method "fp8"), whose config lists model.language_model.layers.N.mlp.gate among 882 modules_to_not_convert entries. The MoE router mlp.gate must not be quantized, but mlp.gate_proj must be.

  • re.match("model.language_model.layers.0.mlp.gate", "model.language_model.layers.0.mlp.gate_proj.weight") → True (bug: skips gate_proj)
  • After anchoring: re.match(f"{key}$", full_name) → False, gate_proj converts correctly.

Fix

Anchor the regex to exact match, preserving the existing layer.* wildcard semantics:

re.match(f"{key}\\.", full_name) or re.match(f"{key}$", full_name) or full_name.endswith(key)

A regression test is added to tests/quantization/mxfp4/test_mxfp4.py::test_should_convert_module asserting that a literal pattern does not skip a longer sibling that only shares its prefix.

Why this matters

It is not a niche cosmetic issue: any finegrained-FP8 MoE checkpoint whose config names a router submodule that is a prefix of another module (e.g. mlp.gate vs mlp.gate_proj) either fails to load or silently mis-quantizes. The current logic has no way to express "exactly this module" for such names.

@github-actions

Copy link
Copy Markdown
Contributor

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

run-slow: mxfp4

…hing siblings

`re.match(f"{key}", full_name)` was unanchored, so a `modules_to_not_convert`
entry like `mlp.gate` also matched `mlp.gate_proj`, leaving it a plain
nn.Linear with FP8 weights -> "mat1 BFloat16 != mat2 Float8_e4m3fn" on
finegrained-fp8 MoE checkpoints. Anchor with `$`; exact and `layer.*`
wildcards still match.

Co-Authored-By: Claude <noreply@anthropic.com>
@Ilya-a-sergeyev-ger
Ilya-a-sergeyev-ger force-pushed the fix-fp8-should-convert-prefix-match branch from 8427f37 to 30c295f Compare August 15, 2026 17:07
@github-actions

Copy link
Copy Markdown
Contributor

CI recap

Dashboard: View test results in Grafana
Latest run: 31897425256:2
Result: success | Jobs: 16 | Tests: 181,622 | Failures: 0 | Duration: 17h 48m

Terrydaktal added a commit to Terrydaktal/vllm-coherence that referenced this pull request Sep 18, 2026
… and tokenizer boundary fixes

Preserve the upstream Qwen parser/streaming-engine and tokenizer corrections as attributed source patches. Keep parser state, configuration and token handling together so the integration can validate the complete compatible source set.

Provenance
Upstream PRs and authors:
- vllm-project/vllm#56113 — Svliverse-1
- vllm-project/vllm#56661 — JordiPosthumus
- huggingface/transformers#45422 — qgallouedec
- huggingface/transformers#47855 — ameerhmz
- huggingface/transformers#47990 — Ilya-a-sergeyev-ger
- huggingface/transformers#48708 — albertvillanova
Terrydaktal added a commit to Terrydaktal/vllm-coherence that referenced this pull request Sep 18, 2026
… and tokenizer boundary fixes

Preserve the upstream Qwen parser/streaming-engine and tokenizer corrections as attributed source patches. Keep parser state, configuration and token handling together so the integration can validate the complete compatible source set.

Provenance
Upstream PRs and authors:
- vllm-project/vllm#56113 — Svliverse-1
- vllm-project/vllm#56661 — JordiPosthumus
- huggingface/transformers#45422 — qgallouedec
- huggingface/transformers#47855 — ameerhmz
- huggingface/transformers#47990 — Ilya-a-sergeyev-ger
- huggingface/transformers#48708 — albertvillanova
@IlyasMoutawwakil

Copy link
Copy Markdown
Member

hey, thanks for the report and the repro! the gate/gate_proj case is a real bug, but anchoring with $ stops matching regex entries that end in their own boundary group: ^model\.audio_tower(?:\.|$) no longer covers model.audio_tower.layers.1.proj, and the same unanchored prefix also lets model.layers.1 skip layers 10-19. #49771 makes a match from the start end at a module boundary (a consumed ., or right before a . or the end of the name), which fixes both and keeps those regex entries; it covers your regression case. would you be ok closing this in favor of it? happy to add you as co-author there.

SMukherjee2004 pushed a commit to SMukherjee2004/transformers that referenced this pull request Oct 23, 2026
…uggingface#49771)

* Match a skip-list entry from the start only up to a module boundary

should_convert_module matched an entry from the start of the module name with
no boundary after it, so "model.layers.1" also skipped model.layers.10 to 19,
and Qwen3.8-27B-FP8's router entry "mlp.gate" also skipped the quantized
mlp.gate_proj (its FP8 weight then reached a bf16 linear). A match from the
start now has to end on a "." it consumed, or right before a "." or the end
of the name, which keeps regex entries that carry their own boundary such as
"^model\.audio_tower(?:\.|$)". The endswith clause Hub skip-lists rely on
("gate" covering shared_expert_gate) is unchanged.

Supersedes huggingface#47990, which anchors with "$" and so stops matching such regex
entries.

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

* Shorten the should_convert_module docstring

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

* Restore the matching-rules comment of should_convert_module

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

* Move the should_convert_module test to a general quantizer utils test file

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

* Explain why the skip-list key is grouped

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

* Fit the skip-list comment within the noisy-comment limits

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

* Name the skip-list test's exclusion lists modules_to_not_convert

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

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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