Nothing Special   »   [go: up one dir, main page]

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

[DDP Communication Hook] Simplify the implementation of parseHookResult of PythonCommHook #62389

Closed
wants to merge 7 commits into from

Conversation

wayi1
Copy link
Contributor
@wayi1 wayi1 commented Jul 29, 2021

Stack from ghstack:

Simplify the implementation of parseHookResult of PythonCommHook, by not directly accepting the output of allreduce, which is a tensor list.

Address the comment on #62074 (comment)

Additionally, formatter is also applied to OptimizerHookState and hook_then_optimizer.

Differential Revision: D29982485

…lt of PythonCommHook

Simplify the implementation of `parseHookResult` of `PythonCommHook`, by not directly accepting the output of allreduce, which is a tensor list.

Address the comment on #62074 (comment)

Additionally, formatter is also applied to `OptimizerHookState` and `hook_then_optimizer`.

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

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor
facebook-github-bot commented Jul 29, 2021

🔗 Helpful links

💊 CI failures summary and remediations

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


💚 💚 Looks good so far! There are no failures yet. 💚 💚


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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

wayi1 pushed a commit that referenced this pull request Jul 29, 2021
…lt of PythonCommHook

Simplify the implementation of `parseHookResult` of `PythonCommHook`, by not directly accepting the output of allreduce, which is a tensor list.

Address the comment on #62074 (comment)

Additionally, formatter is also applied to `OptimizerHookState` and `hook_then_optimizer`.

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

ghstack-source-id: 134599912
Pull Request resolved: #62389
…rseHookResult of PythonCommHook"

Simplify the implementation of `parseHookResult` of `PythonCommHook`, by not directly accepting the output of allreduce, which is a tensor list.

Address the comment on #62074 (comment)

Additionally, formatter is also applied to `OptimizerHookState` and `hook_then_optimizer`.

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

[ghstack-poisoned]
…rseHookResult of PythonCommHook"

Simplify the implementation of `parseHookResult` of `PythonCommHook`, by not directly accepting the output of allreduce, which is a tensor list.

Address the comment on #62074 (comment)

Additionally, formatter is also applied to `OptimizerHookState` and `hook_then_optimizer`.

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

[ghstack-poisoned]
…rseHookResult of PythonCommHook"

Simplify the implementation of `parseHookResult` of `PythonCommHook`, by not directly accepting the output of allreduce, which is a tensor list.

Address the comment on #62074 (comment)

Additionally, formatter is also applied to `OptimizerHookState` and `hook_then_optimizer`.

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

[ghstack-poisoned]
…rseHookResult of PythonCommHook"

Simplify the implementation of `parseHookResult` of `PythonCommHook`, by not directly accepting the output of allreduce, which is a tensor list.

Address the comment on #62074 (comment)

Additionally, formatter is also applied to `OptimizerHookState` and `hook_then_optimizer`.

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

[ghstack-poisoned]
…rseHookResult of PythonCommHook"

Simplify the implementation of `parseHookResult` of `PythonCommHook`, by not directly accepting the output of allreduce, which is a tensor list.

Address the comment on #62074 (comment)

Additionally, formatter is also applied to `OptimizerHookState` and `hook_then_optimizer`.

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

[ghstack-poisoned]
wayi1 pushed a commit that referenced this pull request Jul 29, 2021
…lt of PythonCommHook

Pull Request resolved: #62389

Simplify the implementation of `parseHookResult` of `PythonCommHook`, by not directly accepting the output of allreduce, which is a tensor list.

Address the comment on #62074 (comment)

Additionally, formatter is also applied to `OptimizerHookState` and `hook_then_optimizer`.
ghstack-source-id: 134603664

Differential Revision: [D29982485](https://our.internmc.facebook.com/intern/diff/D29982485/)
…rseHookResult of PythonCommHook"

Simplify the implementation of `parseHookResult` of `PythonCommHook`, by not directly accepting the output of allreduce, which is a tensor list.

Address the comment on #62074 (comment)

Additionally, formatter is also applied to `OptimizerHookState` and `hook_then_optimizer`.

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

[ghstack-poisoned]
wayi1 pushed a commit that referenced this pull request Jul 29, 2021
…lt of PythonCommHook

Pull Request resolved: #62389

Simplify the implementation of `parseHookResult` of `PythonCommHook`, by not directly accepting the output of allreduce, which is a tensor list.

Address the comment on #62074 (comment)

Additionally, formatter is also applied to `OptimizerHookState` and `hook_then_optimizer`.
ghstack-source-id: 134626246

Differential Revision: [D29982485](https://our.internmc.facebook.com/intern/diff/D29982485/)
Copy link
Member
@rohan-varma rohan-varma left a comment

Choose a reason for hiding this comment

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

Have we updated all the docstrings to indicate the appropriate return value is now Future[Tensor]?

@wayi1
Copy link
Contributor Author
wayi1 commented Jul 29, 2021

Future[Tensor]

Good point! Will do in the next PR.

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in acba9b3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged oncall: distributed Add this issue/PR to distributed oncall triage queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants