Repository navigation
fix qwen4exp-fp8 ple embedding - #48368
Conversation
ArthurZucker
left a comment
ArthurZucker
left a comment
There was a problem hiding this comment.
Ty! its a great addition, let's make sur it follows our standard API! Happy to do it myself
| "ple.ple_embedding.ngram_embedding.weight_scale", | ||
| ] | ||
| # FP8 releases ship this table quantized with one per-tensor scale. | ||
| _quantizable_embeddings = ["ple.ple_embedding.ngram_embedding"] |
There was a problem hiding this comment.
the modeling should never have anything related to quantization. We can update the FP8 quantizer tho.
Also for quantizing the embeddings, this should be classic with modules_to_convert in the quantization_config
There was a problem hiding this comment.
got it, let's use explicit modules_to_convert🫡 it requires updating the hub ckpt's config, I'll do it asap done
| WeightConverter( | ||
| source_patterns="ngram_embedding.shard_*.weight", | ||
| target_patterns="ngram_embedding.weight", | ||
| target_patterns="ngram_embedding.weight$", |
There was a problem hiding this comment.
The other models do not require this. Might cause issues when saving
There was a problem hiding this comment.
we need $ when saving fp8 ckpt. without $, ngram_embedding.weight_scale also matches the target_patterns and would be saved as ngram_embedding.shard_*.weight_scale, which is not expected.
There was a problem hiding this comment.
Yes, but: https://github.com/huggingface/transformers/blob/add-h4/src/transformers/quantizers/quantizer_finegrained_fp8.py#L266-L268 ! It should "automatically" happen here as the equivalent is done for linear case
There was a problem hiding this comment.
done! the quantizer now anchors target patterns ending in .weight here. this should not affect other checkpoints, as weights and their corresponding scales are always handled separately.
ArthurZucker
added
for patch
ArthurZucker
left a comment
ArthurZucker
left a comment
There was a problem hiding this comment.
GG great PR 😉
One nit !
| modules_to_convert (`list`, *optional*): | ||
| A list of additional module names, such as embedding tables, that should be converted during quantization. | ||
| scale_fmt (`str`, *optional*, defaults to `"float"`): |
There was a problem hiding this comment.
We really needed this granularity, indeed! ty for that.
cc @SunMarc it allows 0 changes to other quantization config!
| WeightConverter( | ||
| source_patterns="ngram_embedding.shard_*.weight", | ||
| target_patterns="ngram_embedding.weight", | ||
| target_patterns="ngram_embedding.weight$", |
There was a problem hiding this comment.
Yes, but: https://github.com/huggingface/transformers/blob/add-h4/src/transformers/quantizers/quantizer_finegrained_fp8.py#L266-L268 ! It should "automatically" happen here as the equivalent is done for linear case
|
[For maintainers] Suggested jobs to run (before merge) run-slow: qwen4_exp, finegrained_fp8 |
CI recapDashboard: View test results in Grafana |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
* fix qwen4exp-fp8 ple embedding * fix weight_scale resave * add modules_to_convert * unnecessary dtype * lets use super * automatically anchor weight targets
What does this PR do?
fixes https://huggingface.co/Qwen/Qwen3.8-Flash-Next-FP8/discussions/2
add a new class
FP8Embedding(nn.Embedding with per-tensor weight scale), also add a private attr_quantizable_embeddingsto determine the embedding which may be quantizedCode Agent Policy
3.8max
Before submitting
Pull Request checks?
to it if that's the case.
Who can review?
@ArthurZucker @Cyrilvallez @vasqu @zucchini-nlp