Skip to content
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

[core][distributed] add stateless process group #10216

Merged
merged 13 commits into from
Nov 11, 2024

Conversation

youkaichao
Copy link
Member

No description provided.

Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Signed-off-by: youkaichao <[email protected]>
Copy link

👋 Hi! Thank you for contributing to the vLLM project.
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 do one of these:

  • Add ready label to the PR
  • Enable auto-merge.

🚀

@youkaichao
Copy link
Member Author

merge for rlhf integration

@youkaichao youkaichao merged commit e6de978 into vllm-project:main Nov 11, 2024
24 checks passed
@youkaichao youkaichao deleted the stateless_group branch November 11, 2024 17:02
Copy link

@kwen2501 kwen2501 left a comment

Choose a reason for hiding this comment

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

LGTM.

General question:
(1) Do you want the new PG to be able to handle object collectives? If so, it seems its method surface would be similar to today PG's method surface. If that's the case, can we (PyTorch) re-work the new_group API so that it does not need calling init_process_group before it? IIUC, init_process_group is the main API that creates a global "state".

(2) If you don't need object collectives, would creating separate "Backend" objects work for you? I guess the "Backend" object would be similar to the PyncclCommunicator here.

Comment on lines +39 to +40
assert dist.get_backend(group) != dist.Backend.NCCL, (
"PyNcclCommunicator should be attached to a non-NCCL group.")

Choose a reason for hiding this comment

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

typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's by design, we only use gloo group to initialize this nccl communicator. gloo for control-plane communication, while nccl for data plane.

rickyyx pushed a commit to rickyyx/vllm that referenced this pull request Nov 13, 2024
@youkaichao
Copy link
Member Author

can we (PyTorch) re-work the new_group API so that it does not need calling init_process_group before it?

yes that would be great!

would creating separate "Backend" objects work for you?

not sure how that works. i remember pytorch's nccl backend has some strange operations, like bucketing the transfer, and some logging wrapper for debugging. we don't want these.

omer-dayan pushed a commit to omer-dayan/vllm that referenced this pull request Nov 14, 2024
sumitd2 pushed a commit to sumitd2/vllm that referenced this pull request Nov 14, 2024
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.

2 participants