Skip to content

Conversation

NickLucche
Copy link
Collaborator

Small follow-up to #25823.

Signed-off-by: NickLucche <[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 MoE kernel import logic by renaming try_import_moe_kernels to import_moe_kernels and removing its boolean return value. This is a good simplification. The corresponding check for MoE op availability in vllm/_custom_ops.py has been updated to be more robust by directly checking for the presence of torch.ops._moe_C.

While this is a positive change, I've identified that the same robustness improvement could be applied to checks for core operators from the _C extension throughout vllm/_custom_ops.py. Currently, there are several places where attributes of torch.ops._C are accessed without first checking if _C itself exists on torch.ops, which could lead to an AttributeError if the core kernel import fails. I've left a detailed comment on the relevant line with all the locations that could be improved. Addressing this in a follow-up PR would significantly improve the codebase's resilience.



if supports_moe_ops and hasattr(torch.ops._moe_C, "marlin_gemm_moe"):
if hasattr(torch.ops, "_moe_C") and hasattr(torch.ops._moe_C, "marlin_gemm_moe"):
Copy link
Contributor

Choose a reason for hiding this comment

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

high

This is a great change to make the check more robust against import failures of the _moe_C extension.

I noticed that similar potentially unsafe checks exist for the _C extension throughout this file. For example, hasattr(torch.ops._C, "gptq_gemm") on line 474. If vllm._C fails to import, torch.ops._C will not exist, and this will raise an AttributeError.

It would be beneficial to apply the same two-step check pattern (hasattr(torch.ops, "_C") and hasattr(torch.ops._C, "...")) to all such occurrences for consistency and robustness. Here are the locations I found:

  • line 474
  • line 512
  • line 633
  • line 653
  • line 700
  • line 1330
  • line 2310
  • line 2324
  • line 2346
  • line 2373

Since this is a follow-up PR, addressing this in another follow-up would be appropriate.

Copy link
Member

@youkaichao youkaichao left a comment

Choose a reason for hiding this comment

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

my plan is to merge these two functions import_core_kernels() and import_moe_kernels() into one single import_kernels()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants