Skip to content

[NCCL] Helper Function to Abort All outstanding NCCL Communicators #40945

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.

Sign up for GitHub

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

Jump to bottom
Closed
osalpekar wants to merge 4 commits into from
Closed

[NCCL] Helper Function to Abort All outstanding NCCL Communicators #40945

osalpekar wants to merge 4 commits into from

Conversation

@osalpekar
Copy link
Member

@osalpekar osalpekar commented Jul 2, 2020 •
edited
Loading

Stack from ghstack:

This adds a helper function to abort all NCCL Communicators associated with a WorkNCCL object

Differential Revision: D22127899

@osalpekar
[NCCL] Helper Function to Abort All outstanding NCCL Communicators
3fa242b 
This adds a helper function to abort all NCCL Communicators associated with a WorkNCCL object

Differential Revision: [D22127899](https://our.internmc.facebook.com/intern/diff/D22127899/)

[ghstack-poisoned]
This was referenced Jul 2, 2020
Closed
Closed
Closed
Closed
@dr-ci
Copy link

dr-ci bot commented Jul 2, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 4af9f51 (more details on the Dr. CI page):

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 14 times.

@jiayisuse
Copy link
Contributor

jiayisuse commented Jul 2, 2020

Wondering if this helper function is being used in this stack

@osalpekar
Copy link
Member Author

osalpekar commented Jul 2, 2020

Wondering if this helper function is being used in this stack

It was used in a prior version of #40946, but not anymore. Previously, we weren't overwriting the opTimeout_ with the user-supplied waitTimeout, so there was some separate logic to detect timed out WorkNCCL objects and abort associated communicators. Now that logic is simplified by just overwriting opTimeout_, though it still might be a good helper func to have.

osalpekar added 2 commits July 2, 2020 16:37
@osalpekar
Update on "[NCCL] Helper Function to Abort All outstanding NCCL Commu…
16508f2 
…nicators"

This adds a helper function to abort all NCCL Communicators associated with a WorkNCCL object

Differential Revision: [D22127899](https://our.internmc.facebook.com/intern/diff/D22127899/)

[ghstack-poisoned]
@osalpekar
Update on "[NCCL] Helper Function to Abort All outstanding NCCL Commu…
aaeefb1 
…nicators"

This adds a helper function to abort all NCCL Communicators associated with a WorkNCCL object

Differential Revision: [D22127899](https://our.internmc.facebook.com/intern/diff/D22127899/)

[ghstack-poisoned]
@mrshenli
Copy link
Contributor

mrshenli commented Jul 7, 2020

If this is not used, shall we move this PR out of this stack and only land it when necessary?

@osalpekar
Update on "[NCCL] Helper Function to Abort All outstanding NCCL Commu…
4af9f51 
…nicators"

This adds a helper function to abort all NCCL Communicators associated with a WorkNCCL object

Differential Revision: [D22127899](https://our.internmc.facebook.com/intern/diff/D22127899/)

[ghstack-poisoned]
@osalpekar osalpekar mentioned this pull request Jul 10, 2020
Closed
@osalpekar
Copy link
Member Author

osalpekar commented Jul 13, 2020

Closing this PR since aborting NCCL Comms from the WorkNCCL-level is no longer necessary given the updated design for work timeouts

@osalpekar osalpekar closed this Jul 13, 2020
@facebook-github-bot facebook-github-bot deleted the gh/osalpekar/49/head branch August 13, 2020 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrshenli mrshenli Awaiting requested review from mrshenli

@pietern pietern Awaiting requested review from pietern

@zhaojuanmao zhaojuanmao Awaiting requested review from zhaojuanmao

@jiayisuse jiayisuse Awaiting requested review from jiayisuse

@pritamdamania87 pritamdamania87 Awaiting requested review from pritamdamania87

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@osalpekar @jiayisuse @mrshenli