[Misc][Quantization] Clarify the intent of GGUF FusedMoE weight materialization
#30310
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a refactoring that clarifies the weight materialization process for GGUF FusedMoE layers. The changes introduce an assertion to ensure that GGUF weights for FusedMoE are loaded as 3D tensors, which makes an implicit assumption explicit and improves robustness. Additionally, comments have been added to explain the logic behind handling merged weights (w1 and w3), and a minor style improvement was made by using a set for membership testing. These changes improve the code's clarity and maintainability without altering the core functionality. The implementation is correct and I have no further recommendations.
| # Materialize GGUF UninitializedParameter accounting merged weights | ||
| if is_gguf_weight and isinstance(param, UninitializedParameter): | ||
| # GGUF currently requires full load (3D tensors). | ||
| assert full_load |
There was a problem hiding this comment.
Perhaps add a message to clarify what's happening when assertion is false?
There was a problem hiding this comment.
Hmm, rather, it would be better to improve the comment (i.e. what is truly necessary). I'll consider changing around this line.
There was a problem hiding this comment.
I changed the comment to note why full_load is required.
Before:
# GGUF currently requires full load (3D tensors).After:
# To materialize a tensor, we must have full shape including
# number of experts, making this portion to require `full_load`.
In the process of FusedMoE weight data materialization from GGUF files,
there is a magic number and some intents are not clear enough.
This commit clarifies some of them:
1. GGUF (currently) requires 3D tensor(s) for FusedMoE layer weights
as we have to know full tensor shape to materialize the parameter
(including number of experts).
2. w1 and w3 are merged per expert, i.e. the next dimension after
the expert ID is to be doubled to store both w1 and w3.
... and makes some minor adjustments.
Signed-off-by: Tsukasa OI <floss_llm@irq.a4lg.com>
a4lg
force-pushed
the
gguf-support-moe-weight-loader-refactor-20251209
branch
from
0fac684 to
7524b0f
Compare
…erialization (vllm-project#30310) Signed-off-by: Tsukasa OI <floss_llm@irq.a4lg.com>
…erialization (vllm-project#30310) Signed-off-by: Tsukasa OI <floss_llm@irq.a4lg.com> Signed-off-by: Joachim Studnia <joachim@mistral.ai>
2 participants
Purpose
This is a refactoring PR with no functional changes (unless the GGUF file is broken).
In the process of
FusedMoEweight data materialization from GGUF files, there is a magic number and some intents are not clear enough.This commit clarifies some of them:
full_load) forFusedMoElayer weights.w1andw3are merged per expert, i.e. the next dimension after the expert ID is to be doubled to store bothw1andw3.if is_gguf_weight...block).final_shape[1]) should be doubled forw1andw3, improving the clarity.... and makes some minor adjustments.
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.