[aDAG] Allow custom NCCL group for aDAG #47141
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ruisearch42
force-pushed
the
custom_nccl
branch
from
1fd9edf
to
d7d19b3
Compare
rkooo567
reviewed
Aug 16, 2024
| """ | ||
|
|
||
| @abstractmethod | ||
| def get_rank(self, actor: ray.actor.ActorHandle) -> int: |
There was a problem hiding this comment.
how does it find the next rank correctly? (is there any assumption in the rank order that could be different from communicator that's passed?)
There was a problem hiding this comment.
Why do we need to get the next rank? As long as there is a map from Actor to rank it will be sufficient?
ruisearch42
force-pushed
the
custom_nccl
branch
from
a212a01
to
9687899
Compare
rkooo567
reviewed
Aug 20, 2024
python/ray/dag/compiled_dag_node.py
Outdated
| @@ -776,7 +783,16 @@ def _preprocess(self) -> None: | |||
| if None in nccl_actors: | |||
| raise ValueError("Driver cannot participate in the NCCL group.") | |||
| if nccl_actors and self._nccl_group_id is None: | |||
| self._nccl_group_id = _init_nccl_group(nccl_actors) | |||
| if self._custom_nccl_group: | |||
| self._nccl_group_id = _set_nccl_group( | |||
There was a problem hiding this comment.
instead of having if/else here, is there any way to make the default cupy nccl group to follow the same interface?
There was a problem hiding this comment.
ChannelContext.nccl_groups has the type Dict[str, "ActorGroup"], that is sufficient?
ruisearch42
force-pushed
the
custom_nccl
branch
4 times, most recently
from
ef33b3e
to
bcaae80
Compare
ruisearch42
force-pushed
the
custom_nccl
branch
from
b1c5bc2
to
3f62bff
Compare
Signed-off-by: Rui Qiao <ruisearch42@gmail.com>
ruisearch42
force-pushed
the
custom_nccl
branch
from
7844bc8
to
c19d7a5
Compare
rkooo567
reviewed
Sep 4, 2024
There was a problem hiding this comment.
I have 3 questions; Let's merge it after addressing these.
- Is "custom_nccl_channel" necessary? I wonder if we can just make it overwrite default nccl channel (so remove the term custom nccl channel, and we just allow to overwrite nccl channel using a handle)?
- Can you add tests with torch distributed (similar to vllm)?
- I wonder how the API will be changed with overlap compute/comm.
ruisearch42
force-pushed
the
custom_nccl
branch
from
5e69f60
to
d1aff64
Compare
added 4 commits
September 5, 2024 21:53
rkooo567
approved these changes
Sep 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
Allow custom NCCL group for aDAG so that we can reuse what the user already created.
Marking
NcclGroupInterfaceas DeveloperAPI for now. After validation by using it in vLLM we can change to alpha stability.vLLM prototype: vllm-project/vllm#7568
Related issue number
Closes #45593
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.