[Refactor][EAGLE] 1/N delete __init__ in mtp_proposer#5176
[Refactor][EAGLE] 1/N delete __init__ in mtp_proposer#5176wangxiyuan merged 2 commits intovllm-project:mainfrom
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 refactors the speculative decoding proposers by making MtpProposer inherit from EagleProposer, which in turn inherits from the base vllm EagleProposer. This is a good step towards simplifying the code and reducing duplication. The changes are mostly about moving initialization logic to the parent classes and updating attribute access (e.g., from self.name to self.method).
However, I've found a critical issue where the refactoring has inadvertently removed support for M-RoPE in MtpProposer. The initialization logic for M-RoPE was deleted but not moved to the new parent class, which will lead to runtime errors for models using this feature. I've provided a comment with a suggested fix to restore this functionality.
| self.hidden_size = vllm_config.speculative_config.draft_model_config.get_hidden_size( | ||
| ) | ||
| super().__init__(vllm_config, device, runner) | ||
|
|
There was a problem hiding this comment.
This refactoring appears to have dropped support for M-RoPE in MtpProposer. The initialization logic for self.uses_mrope and self.mrope_positions was removed from MtpProposer.__init__ but not moved to this parent class. This will cause a runtime error when using a model with M-RoPE.
Please add the M-RoPE initialization logic back into the __init__ method.
Additionally, other methods in MtpProposer that depend on this, such as _propose and dummy_run, will also need to be updated to correctly handle M-RoPE based on self.uses_mrope. For example, _propose should use self.mrope_positions when M-RoPE is enabled, but this logic seems to be missing from the current implementation in the branch.
self.uses_mrope = self.vllm_config.model_config.uses_mrope
if self.uses_mrope:
self.mrope_positions = torch.zeros((3, self.max_num_tokens),
dtype=torch.int64,
device=device)
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
slippersss
force-pushed
the
refactor_1
branch
from
6f4ef53 to
5ae54a7
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
slippersss
force-pushed
the
refactor_1
branch
from
f6ba1d5 to
18321f7
Compare
slippersss
force-pushed
the
refactor_1
branch
from
a735460 to
f145706
Compare
| self.vllm_config.scheduler_config.max_num_seqs = 32 | ||
| self.vllm_config.model_config.dtype = torch.float16 | ||
| self.vllm_config.model_config.max_model_len = 2048 | ||
| self.vllm_config.model_config.uses_mrope = False |
There was a problem hiding this comment.
why is mrope disabled?
There was a problem hiding this comment.
Related ut mainly focuses on testing language model instead of multimodal model. Since the following assertion involves positions, we have to disable uses_mrope here, otherwise mrope_positions will be initialized replacing positions. By the way, we should and will complement this ut in the near feature.
|
could you create a RFC to record this PR and further PR? This will help community know the process of refactoring mtp. |
Thank you for the suggestion. We are working on it and will release very soon. |
slippersss
force-pushed
the
refactor_1
branch
from
f145706 to
d9820d2
Compare
linfeng-yuan
added
ready-for-test
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
slippersss
force-pushed
the
refactor_1
branch
from
d9820d2 to
784de33
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: Zetong Li <slippersss@126.com>
Signed-off-by: Zetong Li <slippersss@126.com>
slippersss
force-pushed
the
refactor_1
branch
from
784de33 to
07d0f9b
Compare
…to FIA_rebase * 'main' of https://github.com/vllm-project/vllm-ascend: (88 commits) [1/N] Refactor nightly test structure (vllm-project#5479) Docs: Remove deprecated --task parameter for embedding models (vllm-project#5257) Revert "moe_gating_top_k" (vllm-project#5512) [Doc] Fix issue link for 0.12.0 (vllm-project#5500) [CI]update triton ascend version (vllm-project#5392) moe_gating_top_k (vllm-project#5271) [refactor] refactor model runner capture model (vllm-project#5230) Update corresponding vllm commit ID to 12 29 (vllm-project#5475) [Kernel]update csrc cmakelist for open-source cann (vllm-project#5458) [OP] add custom op aclnnMoeInitRoutingCustom (vllm-project#5251) [Refactor][EAGLE] 1/N delete __init__ in mtp_proposer (vllm-project#5176) [Refactor][Triton] Move reject sample triton kernels into ops/triton (vllm-project#5324) [Feature] support eager mode in model runner v2 (vllm-project#5210) [feature] fia support sliding windows (vllm-project#5239) Optimize some rejectsampler functions to make npu op launch non-blocking (vllm-project#4587) [Feature] Support to use fullgraph with eagle (vllm-project#5118) [EPLB][refactor] Modification of the initialization logic for expert_map and log2phy（depend on pr5285） (vllm-project#5311) [Refactor]6/N Extract common code of class AscendMLAImpl (vllm-project#5314) [Refactor] cache cos/sin in mla & remove parameter model in builder. (vllm-project#5277) update vllm pin to 12.27 (vllm-project#5412) ...
) ### What this PR does / why we need it? This PR aims to refactor eagle-related modules in vllm-ascend. This is the starting PR of eagle refactoring. Provided with vllm-eagle, ascend-eagle and ascend-mtp, we first let ascend-mtp inherit from ascend-eagle and let ascend-eagle inherit from vllm-eagle. As a initialization, we just delete `__init__` in mtp_proposer and simplify the corresponding logic in eagle_proposer. Based on "vllm-eagle <----- ascend-eagle <----- ascend-mtp", our target is to gradually delete ascend-mtp and enable ascend-eagle to converge to vllm-eagle. So the main workspace is eagle_proposer. In this way, we hope that contributors can concurrently refactor eagle. Incoming changes: 1. delete common methods in vllm-eagle & ascend-eagle & ascend-mtp 2. delete `load_model` in mtp_proposer 3. delete `dummy_run` and `propose` in mtp_proposer 4. ...... RFC: vllm-project#5467 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? by ci - vLLM version: v0.12.0 - vLLM main: vllm-project/vllm@ad32e3e --------- Signed-off-by: Zetong Li <slippersss@126.com>
) ### What this PR does / why we need it? This PR aims to refactor eagle-related modules in vllm-ascend. This is the starting PR of eagle refactoring. Provided with vllm-eagle, ascend-eagle and ascend-mtp, we first let ascend-mtp inherit from ascend-eagle and let ascend-eagle inherit from vllm-eagle. As a initialization, we just delete `__init__` in mtp_proposer and simplify the corresponding logic in eagle_proposer. Based on "vllm-eagle <----- ascend-eagle <----- ascend-mtp", our target is to gradually delete ascend-mtp and enable ascend-eagle to converge to vllm-eagle. So the main workspace is eagle_proposer. In this way, we hope that contributors can concurrently refactor eagle. Incoming changes: 1. delete common methods in vllm-eagle & ascend-eagle & ascend-mtp 2. delete `load_model` in mtp_proposer 3. delete `dummy_run` and `propose` in mtp_proposer 4. ...... RFC: vllm-project#5467 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? by ci - vLLM version: v0.12.0 - vLLM main: vllm-project/vllm@ad32e3e --------- Signed-off-by: Zetong Li <slippersss@126.com> Signed-off-by: zrj026 <zhangrunjiang026@gmail.com>
) ### What this PR does / why we need it? This PR aims to refactor eagle-related modules in vllm-ascend. This is the starting PR of eagle refactoring. Provided with vllm-eagle, ascend-eagle and ascend-mtp, we first let ascend-mtp inherit from ascend-eagle and let ascend-eagle inherit from vllm-eagle. As a initialization, we just delete `__init__` in mtp_proposer and simplify the corresponding logic in eagle_proposer. Based on "vllm-eagle <----- ascend-eagle <----- ascend-mtp", our target is to gradually delete ascend-mtp and enable ascend-eagle to converge to vllm-eagle. So the main workspace is eagle_proposer. In this way, we hope that contributors can concurrently refactor eagle. Incoming changes: 1. delete common methods in vllm-eagle & ascend-eagle & ascend-mtp 2. delete `load_model` in mtp_proposer 3. delete `dummy_run` and `propose` in mtp_proposer 4. ...... RFC: vllm-project#5467 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? by ci - vLLM version: v0.12.0 - vLLM main: vllm-project/vllm@ad32e3e --------- Signed-off-by: Zetong Li <slippersss@126.com>
) ### What this PR does / why we need it? This PR aims to refactor eagle-related modules in vllm-ascend. This is the starting PR of eagle refactoring. Provided with vllm-eagle, ascend-eagle and ascend-mtp, we first let ascend-mtp inherit from ascend-eagle and let ascend-eagle inherit from vllm-eagle. As a initialization, we just delete `__init__` in mtp_proposer and simplify the corresponding logic in eagle_proposer. Based on "vllm-eagle <----- ascend-eagle <----- ascend-mtp", our target is to gradually delete ascend-mtp and enable ascend-eagle to converge to vllm-eagle. So the main workspace is eagle_proposer. In this way, we hope that contributors can concurrently refactor eagle. Incoming changes: 1. delete common methods in vllm-eagle & ascend-eagle & ascend-mtp 2. delete `load_model` in mtp_proposer 3. delete `dummy_run` and `propose` in mtp_proposer 4. ...... RFC: vllm-project#5467 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? by ci - vLLM version: v0.12.0 - vLLM main: vllm-project/vllm@ad32e3e --------- Signed-off-by: Zetong Li <slippersss@126.com> Signed-off-by: zrj026 <zhangrunjiang026@gmail.com>
4 participants
What this PR does / why we need it?
This PR aims to refactor eagle-related modules in vllm-ascend.
This is the starting PR of eagle refactoring. Provided with vllm-eagle, ascend-eagle and ascend-mtp, we first let ascend-mtp inherit from ascend-eagle and let ascend-eagle inherit from vllm-eagle. As a initialization, we just delete
__init__in mtp_proposer and simplify the corresponding logic in eagle_proposer.Based on "vllm-eagle <----- ascend-eagle <----- ascend-mtp", our target is to gradually delete ascend-mtp and enable ascend-eagle to converge to vllm-eagle. So the main workspace is eagle_proposer. In this way, we hope that contributors can concurrently refactor eagle.
Incoming changes:
load_modelin mtp_proposerdummy_runandproposein mtp_proposerRFC: #5467
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
by ci