Skip to content

Conversation

shikang-hangzhou
Copy link

What this PR does / why we need it?

Does this PR introduce any user-facing change?

How was this patch tested?

Signed-off-by: shikang-hangzhou <[email protected]>
Signed-off-by: shikang-hangzhou <[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 refactors the validation logic for an unsupported feature combination by moving the check from the worker initialization to the platform configuration setup. This is a good improvement as it centralizes configuration validation and provides a clearer, more actionable error message to the user earlier in the execution flow. My feedback focuses on improving the exception type for better semantic correctness.

Comment on lines 149 to 150
raise NotImplementedError(
"DBO and mtp can't work at the same time. Please `export VLLM_ASCEND_ENABLE_DBO=0`")
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Using NotImplementedError here might not be the most semantically correct choice. This exception is typically raised when an abstract method that must be implemented by a subclass has not been, or when a feature is not yet implemented. Since this check validates a combination of user-provided configurations, a ValueError would be more appropriate. It clearly signals that the provided configuration values are incompatible.

Suggested change
raise NotImplementedError(
"DBO and mtp can't work at the same time. Please `export VLLM_ASCEND_ENABLE_DBO=0`")
raise ValueError(
"DBO and mtp can't work at the same time. Please `export VLLM_ASCEND_ENABLE_DBO=0`")

Signed-off-by: shikang-hangzhou <[email protected]>
Signed-off-by: shikang-hangzhou <[email protected]>
@shikang-hangzhou shikang-hangzhou closed this by deleting the head repository Aug 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant