[c10d] user to explicitly specify whether split semantics should be used in new_group #138518
Conversation
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/138518
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 7246122 with merge base ce63193 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
pytorch-bot
bot
added
oncall: distributed
shuqiangzhang
changed the title
…d be used in new_group" Summary: Eager init and split should be two different concepts. E.g., we should be able to support eager init but without using split mode. In this PR, one needs to explicitly specify use_split, in order to use the split comm functions in NCCL Test Plan: Added a test_comm_eager_without_split Reviewers: Subscribers: Tasks: Tags: cc H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 wconstab d4l3k c-p-i-o [ghstack-poisoned]
…d be used in new_group" Summary: Eager init and split should be two different concepts. E.g., we should be able to support eager init but without using split mode. In this PR, one needs to explicitly specify use_split, in order to use the split comm functions in NCCL Test Plan: Added a test_comm_eager_without_split Reviewers: Subscribers: Tasks: Tags: cc H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 wconstab d4l3k c-p-i-o [ghstack-poisoned]
…d be used in new_group" Summary: Eager init and split should be two different concepts. E.g., we should be able to support eager init but without using split mode. In this PR, we provide users an option to specify use_split or not, even if eager init is used. Test Plan: Added a test_comm_eager_without_split Reviewers: Subscribers: Tasks: Tags: cc H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 wconstab d4l3k c-p-i-o [ghstack-poisoned]
shuqiangzhang
changed the title
|
It depends on whether we see split as implementation detail or not. |
Similar to eager init/device id para input, for normal users who don't care much about init time or resources, they do not need to know or set this. But if they care, they have a choice without changing their current code structure of using new_group, and the are also encouraged to switch to split_group |
|
my thought is that use_split defaults to false, which may lead to less usage of split. I think we need to balance between good design to separate split/eager, but also using defaults to encourage the path we want to encourage. What are the tradeoffs of using new_group + use_split=True vs False? What about the tradeoffs of using new_group+use_split vs split_group? Separately, if you want to discourage new_group and encourage split_group, how do we do that? |
Right, Checking my commit V2, V3 and V4, you could see me debating myself by changing the default use_split from false to true and then to false again. The intention of changing the default to be false is exactly to encourage performance caring users to use split_group instead of new_group. split_group is mostly stable now and is better than new_group + use_split=True for all 2 strong reasons listed in #130407, that's why advanced users (including device mesh) are using split_group now. Of course, we can only suggest this to users (modified comment/doc) but not force it, that's why we still need to keep the old option viable of new_group. For users don't care about perf, not using split is fine. But once users feel the needs of better performance, they can follow the suggestion |
|
Shall we look at the There are a couple questions we'd need to answer imo:
|
From perf point of view, newgroup without split < newgroup with split (< split_group). For most OSS users, they don't need to know the details, that's why it's an optional para. But for advanced users （e.g., who only want to eager init a sub pg, but not using split for various reasons), we need to give them this option. |
…should be used in new_group" Summary: Eager init and split should be two different concepts. E.g., we should be able to support eager init but without using split mode. In this PR, we provide users an option to specify use_split or not, even if eager init is used. Also, in the future, it is recommended to use split_group instead of new_group if users want to use split. Test Plan: Added a test_comm_eager_without_split Reviewers: Subscribers: Tasks: Tags: cc H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 wconstab d4l3k c-p-i-o [ghstack-poisoned]
|
One more clarification- if you use new group with or without split, you still have to have all ranks call new group the same number of times in the same order, right? (No visible semantic changes based on the split flag)? |
That's a good question that I hadn't thought before. Indeed, if no split is used, I think there is no need of the requirement of calling new_group on every rank of the default PG, so this requirement can be relaxed. (Let me verify this by some tests). calling it on every ranks does not do any harm and just backward compatible |
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
Stack from ghstack (oldest at bottom):
Summary:
Eager init and split should be two different concepts. E.g., we should be able to support eager init but without using split mode.
In this PR, we provide users an option to specify use_split or not, even if eager init is used.
Also, in the future, it is recommended to use split_group instead of new_group if users want to use split.
Test Plan:
Added a test_comm_eager_without_split
Reviewers:
Subscribers:
Tasks:
Tags:
cc @H-Huang @awgu @kwen2501 @wanchaol @fegin @fduwjj @wz337 @wconstab @d4l3k @c-p-i-o