revert(mtp): drop shared BaseMTPModel base, keep per-model is_mtp_draft_model (revert #1337)#1339
Conversation
…MTP draft models (ModelTC#1337)" This reverts commit e03ef9a.
Re-add the is_mtp_draft_model = True class marker (previously supplied by the reverted BaseMTPModel base) directly on each standalone MTP draft model, so the decode CUDA-graph / padding paths still detect them. Keeps the models self-contained with no shared base class.
![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 MTP draft models by removing the "BaseMTPModel" mixin class and inlining its initialization and manager-sharing logic directly into the individual model classes ("Deepseek3MTPModel", "Glm4MoeLiteMTPModel", "MistralMTPModel", and "Qwen3MOEMTPModel"). The review feedback recommends removing redundant explicit "return" statements from the newly added initializer and setup methods across these models to comply with PEP 8 guidelines and improve code readability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def __init__(self, kvargs: dict): | ||
| self._pre_init(kvargs) | ||
| super().__init__(kvargs) | ||
| return | ||
|
|
||
| def _pre_init(self, kvargs: dict): | ||
| self.main_model: TpPartBaseModel = kvargs.pop("main_model") | ||
| self.mtp_previous_draft_models: List[TpPartBaseModel] = kvargs.pop("mtp_previous_draft_models") | ||
| return | ||
|
|
||
| def _init_custom(self): | ||
| self._cos_cached = self.main_model._cos_cached | ||
| self._sin_cached = self.main_model._sin_cached | ||
| return | ||
|
|
||
| def _init_req_manager(self): | ||
| self.req_manager = self.main_model.req_manager | ||
| return | ||
|
|
||
| def _init_mem_manager(self): | ||
| self.mem_manager = self.main_model.mem_manager | ||
| return |
There was a problem hiding this comment.
According to PEP 8 and standard Python idioms, explicit return statements at the end of functions that do not return a value (such as __init__ and other initializer/setter methods) are redundant and should be omitted. Removing them improves code readability and ensures consistency with Glm4MoeLiteMTPModel which correctly omits them.
| def __init__(self, kvargs: dict): | |
| self._pre_init(kvargs) | |
| super().__init__(kvargs) | |
| return | |
| def _pre_init(self, kvargs: dict): | |
| self.main_model: TpPartBaseModel = kvargs.pop("main_model") | |
| self.mtp_previous_draft_models: List[TpPartBaseModel] = kvargs.pop("mtp_previous_draft_models") | |
| return | |
| def _init_custom(self): | |
| self._cos_cached = self.main_model._cos_cached | |
| self._sin_cached = self.main_model._sin_cached | |
| return | |
| def _init_req_manager(self): | |
| self.req_manager = self.main_model.req_manager | |
| return | |
| def _init_mem_manager(self): | |
| self.mem_manager = self.main_model.mem_manager | |
| return | |
| def __init__(self, kvargs: dict): | |
| self._pre_init(kvargs) | |
| super().__init__(kvargs) | |
| def _pre_init(self, kvargs: dict): | |
| self.main_model: TpPartBaseModel = kvargs.pop("main_model") | |
| self.mtp_previous_draft_models: List[TpPartBaseModel] = kvargs.pop("mtp_previous_draft_models") | |
| def _init_custom(self): | |
| self._cos_cached = self.main_model._cos_cached | |
| self._sin_cached = self.main_model._sin_cached | |
| def _init_req_manager(self): | |
| self.req_manager = self.main_model.req_manager | |
| def _init_mem_manager(self): | |
| self.mem_manager = self.main_model.mem_manager |
References
- PEP 8 recommends omitting explicit return statements at the end of functions that do not return a value. (link)
| def __init__(self, kvargs: dict): | ||
| self._pre_init(kvargs) | ||
| super().__init__(kvargs) | ||
| return | ||
|
|
||
| def _pre_init(self, kvargs: dict): | ||
| self.main_model: TpPartBaseModel = kvargs.pop("main_model") | ||
| self.mtp_previous_draft_models: List[TpPartBaseModel] = kvargs.pop("mtp_previous_draft_models") | ||
| return |
There was a problem hiding this comment.
According to PEP 8 and standard Python idioms, explicit return statements at the end of functions that do not return a value are redundant and should be omitted. Removing them improves code readability and ensures consistency with Glm4MoeLiteMTPModel.
| def __init__(self, kvargs: dict): | |
| self._pre_init(kvargs) | |
| super().__init__(kvargs) | |
| return | |
| def _pre_init(self, kvargs: dict): | |
| self.main_model: TpPartBaseModel = kvargs.pop("main_model") | |
| self.mtp_previous_draft_models: List[TpPartBaseModel] = kvargs.pop("mtp_previous_draft_models") | |
| return | |
| def __init__(self, kvargs: dict): | |
| self._pre_init(kvargs) | |
| super().__init__(kvargs) | |
| def _pre_init(self, kvargs: dict): | |
| self.main_model: TpPartBaseModel = kvargs.pop("main_model") | |
| self.mtp_previous_draft_models: List[TpPartBaseModel] = kvargs.pop("mtp_previous_draft_models") |
References
- PEP 8 recommends omitting explicit return statements at the end of functions that do not return a value. (link)
| def _init_custom(self): | ||
| self._cos_cached = self.main_model._cos_cached | ||
| self._sin_cached = self.main_model._sin_cached | ||
| return | ||
|
|
||
| def _init_req_manager(self): | ||
| self.req_manager = self.main_model.req_manager | ||
| return | ||
|
|
||
| def _init_mem_manager(self): | ||
| self.mem_manager = self.main_model.mem_manager | ||
| return |
There was a problem hiding this comment.
According to PEP 8 and standard Python idioms, explicit return statements at the end of functions that do not return a value are redundant and should be omitted. Removing them improves code readability and ensures consistency with Glm4MoeLiteMTPModel.
| def _init_custom(self): | |
| self._cos_cached = self.main_model._cos_cached | |
| self._sin_cached = self.main_model._sin_cached | |
| return | |
| def _init_req_manager(self): | |
| self.req_manager = self.main_model.req_manager | |
| return | |
| def _init_mem_manager(self): | |
| self.mem_manager = self.main_model.mem_manager | |
| return | |
| def _init_custom(self): | |
| self._cos_cached = self.main_model._cos_cached | |
| self._sin_cached = self.main_model._sin_cached | |
| def _init_req_manager(self): | |
| self.req_manager = self.main_model.req_manager | |
| def _init_mem_manager(self): | |
| self.mem_manager = self.main_model.mem_manager |
References
- PEP 8 recommends omitting explicit return statements at the end of functions that do not return a value. (link)
| def __init__(self, kvargs: dict): | ||
| self._pre_init(kvargs) | ||
| super().__init__(kvargs) | ||
| return | ||
|
|
||
| def _pre_init(self, kvargs: dict): | ||
| self.main_model: TpPartBaseModel = kvargs.pop("main_model") | ||
| self.mtp_previous_draft_models: List[TpPartBaseModel] = kvargs.pop("mtp_previous_draft_models") | ||
| return | ||
|
|
||
| def _init_custom(self): | ||
| self._cos_cached = self.main_model._cos_cached | ||
| self._sin_cached = self.main_model._sin_cached | ||
| return | ||
|
|
||
| def _init_req_manager(self): | ||
| self.req_manager = self.main_model.req_manager | ||
| return | ||
|
|
||
| def _init_mem_manager(self): | ||
| self.mem_manager = self.main_model.mem_manager | ||
| return |
There was a problem hiding this comment.
According to PEP 8 and standard Python idioms, explicit return statements at the end of functions that do not return a value are redundant and should be omitted. Removing them improves code readability and ensures consistency with Glm4MoeLiteMTPModel.
| def __init__(self, kvargs: dict): | |
| self._pre_init(kvargs) | |
| super().__init__(kvargs) | |
| return | |
| def _pre_init(self, kvargs: dict): | |
| self.main_model: TpPartBaseModel = kvargs.pop("main_model") | |
| self.mtp_previous_draft_models: List[TpPartBaseModel] = kvargs.pop("mtp_previous_draft_models") | |
| return | |
| def _init_custom(self): | |
| self._cos_cached = self.main_model._cos_cached | |
| self._sin_cached = self.main_model._sin_cached | |
| return | |
| def _init_req_manager(self): | |
| self.req_manager = self.main_model.req_manager | |
| return | |
| def _init_mem_manager(self): | |
| self.mem_manager = self.main_model.mem_manager | |
| return | |
| def __init__(self, kvargs: dict): | |
| self._pre_init(kvargs) | |
| super().__init__(kvargs) | |
| def _pre_init(self, kvargs: dict): | |
| self.main_model: TpPartBaseModel = kvargs.pop("main_model") | |
| self.mtp_previous_draft_models: List[TpPartBaseModel] = kvargs.pop("mtp_previous_draft_models") | |
| def _init_custom(self): | |
| self._cos_cached = self.main_model._cos_cached | |
| self._sin_cached = self.main_model._sin_cached | |
| def _init_req_manager(self): | |
| self.req_manager = self.main_model.req_manager | |
| def _init_mem_manager(self): | |
| self.mem_manager = self.main_model.mem_manager |
References
- PEP 8 recommends omitting explicit return statements at the end of functions that do not return a value. (link)
1 participant
Reverts the
BaseMTPModelshared-base refactor from #1337 — per review feedback that consolidating the existing MTP models onto a shared base class isn't the right tradeoff.What this does
lightllm/models/base_mtp_model.pyand its unit test.is_mtp_draft_model = Trueas an explicit class marker on each of the four models (previously inherited fromBaseMTPModel). The MTP decode CUDA-graph / padding paths use this marker to detect a draft model, so detection keeps working without the shared base.Net result: the four MTP models are standalone again, each carrying its own wiring plus a one-line marker — no shared base class.
Testing
pre-commit(black 21.12b0 + flake8 6.1.0) clean.