Skip to content

Conversation

@wangxiyuan
Copy link
Collaborator

@wangxiyuan wangxiyuan commented Dec 4, 2025

patach_config is useless now. Let's remove it

@github-actions
Copy link

github-actions bot commented Dec 4, 2025

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

Signed-off-by: wangxiyuan <[email protected]>
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request proposes removing a monkey patch for SpeculativeConfig. While removing monkey patches is generally beneficial for maintainability, this patch contains critical logic for configuring speculative decoding on Ascend platforms. Removing it without ensuring this logic is handled elsewhere (either upstream in vllm or within the vllm-ascend codebase) could break speculative decoding functionality. I have raised a critical issue regarding the potential loss of this essential configuration logic.

I am having trouble creating individual review comments. Click here to see my feedback.

vllm_ascend/patch/platform/patch_config.py (1-234)

critical

Removing this monkey patch for SpeculativeConfig.__post_init__ is concerning as it contains critical logic for configuring speculative decoding on Ascend. This includes auto-detecting the speculative method (e.g., 'eagle', 'medusa', 'deepseek_mtp') and setting up the draft_model_config.

If this logic has not been fully integrated into the upstream vllm's SpeculativeConfig or moved to another part of the vllm-ascend codebase, removing this file will likely break speculative decoding functionality.

For example, the following logic for method detection is being removed:

            elif "eagle-" in self.draft_model_config.model.lower():
                self.method = "eagle"
            elif "eagle3" in self.draft_model_config.model.lower():
                self.method = "eagle3"
            elif self.draft_model_config.hf_config.model_type == "medusa":
                self.method = "medusa"

Additionally, the logic for creating draft_model_config and other method-specific configurations is also being deleted.

Given the importance of this configuration logic, removing it without a clear replacement poses a significant risk. This change should be reverted unless it can be confirmed that this functionality is now handled elsewhere.

@wangxiyuan wangxiyuan added ready read for review ready-for-test start test by label for PR labels Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready read for review ready-for-test start test by label for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant