Skip to content

Conversation

realliujiaxu
Copy link
Contributor

@realliujiaxu realliujiaxu commented Sep 11, 2025

What this PR does / why we need it?

The current linear.py has the following issues:

  • There is redundant conditional logic in the comm_group and forward selection for classes such as AscendMergedColumnParallelLinear.

  • Inconsistent comm_group selection logic exists among AscendMergedColumnParallelLinear, AscendColumnParallelLinear, and AscendQKVParallelLinear.

To address these two issues, this PR encapsulates comm_group and forward into classes and extracts the classes selection logic into common functions. For future additions of custom communication groups or forward methods, it will only be necessary to extend CustomColumnParallelOp or CustomRowParallelOp and add new selection logic.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Copy link

👋 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.

Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@realliujiaxu realliujiaxu changed the title refactor linear [Draft] refactor linear Sep 11, 2025
@weijinqian0
Copy link
Collaborator

We have discussed a refactoring plan that will encapsulate the differing parts while keeping the patch sections unchanged.

@realliujiaxu realliujiaxu force-pushed the refactor_linear branch 2 times, most recently from 5999110 to 5e45c5d Compare September 12, 2025 14:27
@realliujiaxu
Copy link
Contributor Author

@weijinqian0 This PR is ready for review now

@realliujiaxu realliujiaxu changed the title [Draft] refactor linear refactor linear Sep 15, 2025
realliujiaxu and others added 7 commits September 15, 2025 20:39
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Co-authored-by: weijinqian0 <weijinqian@huawei.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
@realliujiaxu realliujiaxu marked this pull request as draft September 16, 2025 08:43
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
@realliujiaxu realliujiaxu marked this pull request as ready for review September 17, 2025 08:32
@weijinqian0
Copy link
Collaborator

approved

@wangxiyuan wangxiyuan added ready read for review ready-for-test start test by label for PR labels Sep 18, 2025
3. Override the apply method according to requirements, which will replace the original linear.forward
4. Add selection logic for MyColumnParallelOp in the get_column_parallel_op method, typically based on prefix and configuration judgments
Row parallel op follows a similar approach - inherit from RowColumnParallelOp and register the new class in get_row_parallel_op.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice description

@wangxiyuan wangxiyuan merged commit af2a886 into vllm-project:main Sep 18, 2025
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:ops module:tests 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.

4 participants