-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Move init_ddp_connection
to distributed utilities
#9044
Conversation
for more information, see https://pre-commit.ci
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.
fyi @four4fish as collective initialization could be part of #7534
if the plugin initializes the global procss group, then we have higher confidence that the global process group can be destroyed in the teardown
Codecov Report
@@ Coverage Diff @@
## master #9044 +/- ##
======================================
Coverage 93% 93%
======================================
Files 175 175
Lines 14379 14361 -18
======================================
- Hits 13322 13312 -10
+ Misses 1057 1049 -8 |
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.
LGMT !
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.
LGTM 😃
Hi @kaushikb11. Mind addressing the failing mypy job in a new PR? |
What does this PR do?
It is duplicated in DDP & DDPSpawn plugins.
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃