[Bugfix] Fix bug with establishing the flashcomm2 and pp communication domains. #4458
Conversation
![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 correctly fixes a bug in the flashcomm2 communication domain setup by incorporating pipeline parallelism (pp). The logic for calculating global ranks and forming communication groups has been updated to account for the pipeline parallel size. My review includes one suggestion to improve the readability and maintainability of the complex rank calculation logic, which I consider important for this critical part of the code.
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
Signed-off-by: zzhx1 <[email protected]>
Signed-off-by: zzhx1 <[email protected]>
…ifications. Co-authored-by: Levi-JQ <[email protected]> Signed-off-by: zzhx1 <[email protected]>
Signed-off-by: zzhx1 <[email protected]>
|
@ApsarasX Please check this PR and help to merge. |
|
@wangxiyuan Please help merge this pr, this is a bugfix for the flashcomm2 communication domain. |
…n domains. (vllm-project#4458) ### What this PR does / why we need it? The previous implementation of the flashcomm2 communication domain did not consider pp(pipeline parallel), which caused problems when enabling pp and flashcomm2. This PR fixes this issue. - vLLM version: v0.11.2 - vLLM main: https://github.com/vllm-project/vllm/commit/v0.11.2 --------- Signed-off-by: zzhx1 <[email protected]> Co-authored-by: Levi-JQ <[email protected]>
2 participants
What this PR does / why we need it?
The previous implementation of the flashcomm2 communication domain did not consider pp(pipeline parallel), which caused problems when enabling pp and flashcomm2. This PR fixes this issue.