-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
fix pynccl reduce_scatter #23648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix pynccl reduce_scatter #23648
Conversation
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run You ask your reviewers to trigger select CI tests on top of Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
There was a problem hiding this 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 correctly fixes a bug in the reduce_scatter
function where the wrong tensor was being passed to the underlying pynccl communication primitive. The fix ensures that the tensor with the rearranged dimensions is used. However, a similar bug exists in the reduce_scatterv
function within the same file, which has been overlooked. I've added a comment to highlight this critical issue, as fixing it would ensure correctness and consistency across related communication operations.
6ec2ec8
to
1126a84
Compare
cc @tlrmchlsmth |
Co-authored-by: hongchao <[email protected]> Signed-off-by: tc-mb <[email protected]>
Co-authored-by: hongchao <[email protected]>
Co-authored-by: hongchao <[email protected]> Signed-off-by: Xiao Yu <[email protected]>
Co-authored-by: hongchao <[email protected]>
Co-authored-by: hongchao <[email protected]>
Co-authored-by: hongchao <[email protected]>
This PR fix the reduce_scatter interface in pynccl-wrapper. The old implementation fails when scatter_dim != 0 because the input_tensor is incorrectly supplied, it doesn’t use the tensor rearranged by movedim.