Skip to content

Add quickreduce as alternative to custom allreduce #16804

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ilmarkov
Copy link
Contributor

@ilmarkov ilmarkov commented Apr 17, 2025

Add quickreduce alternative to custom allreduce.
The collective is only enabled on AMD. The kernels support quantized all reduce collective int4/int8 symmetric and asymmetric quantization algorithms.
The kernels can be enabled by quick_reduce_allreduce_algo config parameter. On AMD we first check if it is profitable to use quickreduce, otherwise we fallback to custom allreduce.

Copy link

👋 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 fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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 ready label to the PR or enable auto-merge.

🚀

@mergify mergify bot added the ci/build label Apr 17, 2025
@lixixicommute
Copy link

hi, @ilmarkov ,
Thank you for your great work.
Does this program have any test data and how well does it work?
It looks like it's still a draft at the moment, will it be refined afterward?

@ilmarkov ilmarkov force-pushed the experimental/quick_reduce branch from 5b81d85 to 96e1a3e Compare May 13, 2025 13:27
Copy link

mergify bot commented May 14, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @ilmarkov.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label May 14, 2025
@ilmarkov ilmarkov force-pushed the experimental/quick_reduce branch from 96e1a3e to d92ccc8 Compare May 19, 2025 09:42
ilmarkov added 6 commits May 20, 2025 13:41
WIP
Signed-off-by: ilmarkov <[email protected]>
Signed-off-by: ilmarkov <[email protected]>
WIP
Signed-off-by: ilmarkov <[email protected]>
Signed-off-by: ilmarkov <[email protected]>
Signed-off-by: ilmarkov <[email protected]>
@ilmarkov ilmarkov force-pushed the experimental/quick_reduce branch from d92ccc8 to 6f17424 Compare May 20, 2025 13:41
@ilmarkov ilmarkov marked this pull request as ready for review May 20, 2025 13:42
@youkaichao
Copy link
Member

youkaichao commented May 20, 2025

On AMD we first check if it is profitable to use quickreduce, otherwise we fallback to custom allreduce.

what are the cases when custom allreduce performs better than quickreduce? It would better if quickreduce can surpass custom allreduce in all cases, then we can use quickreduce as a drop-in replacement of custom allreduce without a new user-facing flag.

@ilmarkov
Copy link
Contributor Author

@youkaichao It is slower for smaller input sizes. We could do the similar approach as custom allreduce has - use one shot for small buffers and two shot for larger ones.

@youkaichao
Copy link
Member

@youkaichao It is slower for smaller input sizes. We could do the similar approach as custom allreduce has - use one shot for small buffers and two shot for larger ones.

that would be great, can you implement it? we can use either quickreduce or custom allreduce at the engine level, instead of dynamically switching based on the input size.

@ilmarkov
Copy link
Contributor Author

Yes, we can try to implement this approach.
Although, custom allreduce setup and implementation is more suitable for low latency small input sizes, whereas quick reduce performs well for bandwidth bottlenecked workloads. At the moment, we use custom allreduce or nccl based on input size.
Also, we will still need the new uder-facing flag as we need to provide switch between quantization regimes allowing user to find a trade-off between accuracy and performance.

@youkaichao
Copy link
Member

Also, we will still need the new uder-facing flag as we need to provide switch between quantization regimes allowing user to find a trade-off between accuracy and performance.

you can use an environment variable, like VLLM_ROCM_CA_BACKEND.

Copy link

mergify bot commented May 23, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @ilmarkov.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label May 23, 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.

3 participants