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

[Gradient Compression] Refactor CommHookInterface and PythonCommHook. #46512

Closed
wants to merge 12 commits into from

Conversation

wayi1
Copy link
Contributor

@wayi1 wayi1 commented Oct 17, 2020

Stack from ghstack:

  1. Merge 1-line PythonCommHook constructor into the header for simplicity.
  2. Move the implementation of PythonCommHook destructor from the header file to cpp file.
  3. Rename processFuture method as parseHookResult for readability.
  4. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348

Differential Revision: D24374282

1. Let CommHookInterface::processFuture be a non-virtual method, which can be shared by both Python and C++ implementations.
2. Merge 1-line PythonCommHook constructor into the header for simplicity.
3. Rename processFuture method as parseFromHookResult for readability.
4. Simplify the comments.

Original PR issue: C++ DDP Communication Hook #46348

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

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 17, 2020

💊 CI failures summary and remediations

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


Commit d01a4d5 was recently pushed. Waiting for builds...


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 4 times.

@facebook-github-bot facebook-github-bot added the oncall: distributed Add this issue/PR to distributed oncall triage queue label Oct 17, 2020
…onCommHook."

1. Let CommHookInterface::processFuture be a non-virtual method, which can be shared by both Python and C++ implementations.
2. Merge 1-line PythonCommHook constructor into the header for simplicity.
3. Rename processFuture method as parseFromHookResult for readability.
4. Simplify the comments.

Original PR issue: C++ DDP Communication Hook #46348

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

[ghstack-poisoned]
wayi1 pushed a commit that referenced this pull request Oct 17, 2020
Pull Request resolved: #46512

1. Let CommHookInterface::processFuture be a non-virtual method, which can be shared by both Python and C++ implementations.
2. Merge 1-line PythonCommHook constructor into the header for simplicity.
3. Rename processFuture method as parseFromHookResult for readability.
4. Simplify the comments.

Original PR issue: C++ DDP Communication Hook #46348
ghstack-source-id: 114558527

Differential Revision: [D24374282](https://our.internmc.facebook.com/intern/diff/D24374282/)
@dr-ci
Copy link

dr-ci bot commented Oct 17, 2020

💊 CI failures summary and remediations

As of commit 377c729 (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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 64 times.

torch/csrc/distributed/c10d/comm.h Outdated Show resolved Hide resolved
torch/csrc/distributed/c10d/comm.h Show resolved Hide resolved
torch/csrc/distributed/c10d/comm.h Show resolved Hide resolved
torch/csrc/distributed/c10d/comm.h Show resolved Hide resolved
torch/csrc/distributed/c10d/comm.cpp Outdated Show resolved Hide resolved
…onCommHook."

1. Let CommHookInterface::processFuture be a non-virtual method, which can be shared by both Python and C++ implementations.
2. Merge 1-line PythonCommHook constructor into the header for simplicity.
3. Rename processFuture method as parseFromHookResult for readability.
4. Simplify the comments.

Original PR issue: C++ DDP Communication Hook #46348

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

[ghstack-poisoned]
wayi1 pushed a commit that referenced this pull request Oct 22, 2020
Pull Request resolved: #46512

1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Move the implementation of PythonCommHook destructor from the header file to the cpp file.
3. Rename processFuture method as parseHookResult for readability.
4. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348
ghstack-source-id: 114899417

Differential Revision: [D24374282](https://our.internmc.facebook.com/intern/diff/D24374282/)
…onCommHook."


1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Move the implementation of PythonCommHook destructor from the header file to the cpp file.
3. Rename processFuture method as parseHookResult for readability.
4. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348

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

[ghstack-poisoned]
Copy link
Contributor

@pritamdamania87 pritamdamania87 left a comment

Choose a reason for hiding this comment

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

Code changes look good, although it seems like the windows tests are failing with an error like:

comm.cpp.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: virtual __cdecl c10d::CommHookInterface::~CommHookInterface(void)" (__imp_??1CommHookInterface@c10d@@UEAA@XZ) referenced in function "public: virtual __cdecl c10d::PythonCommHook::~PythonCommHook(void)" (??1PythonCommHook@c10d@@UEAA@XZ)
init.cpp.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __cdecl c10d::CommHookInterface::CommHookInterface(void)" (__imp_??0CommHookInterface@c10d@@QEAA@XZ) referenced in function "class std::unique_ptr<class c10d::PythonCommHook,struct std::default_delete<class c10d::PythonCommHook> > __cdecl std::make_unique<class c10d::PythonCommHook,class pybind11::object,class pybind11::object,0>(class pybind11::object &&,class pybind11::object &&)" (??$make_unique@VPythonCommHook@c10d@@Vobject@pybind11@@V34@$0A@@std@@YA?AV?$unique_ptr@VPythonCommHook@c10d@@U?$default_delete@VPythonCommHook@c10d@@@std@@@0@$$QEAVobject@pybind11@@0@Z)
bin\torch_python.dll : fatal error LNK1120: 2 unresolved externals

torch/csrc/distributed/c10d/comm.cpp Show resolved Hide resolved
…onCommHook."


1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Move the implementation of PythonCommHook destructor from the header file to the cpp file.
3. Rename processFuture method as parseHookResult for readability.
4. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348

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

[ghstack-poisoned]
…onCommHook."


1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Move the implementation of PythonCommHook destructor from the header file to the cpp file.
3. Rename processFuture method as parseHookResult for readability.
4. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348

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

[ghstack-poisoned]
…onCommHook."


1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Rename processFuture method as parseHookResult for readability.
3. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348

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

[ghstack-poisoned]
…onCommHook."


1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Rename processFuture method as parseHookResult for readability.
3. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348

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

[ghstack-poisoned]
wayi1 pushed a commit that referenced this pull request Oct 23, 2020
Pull Request resolved: #46512

1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Rename processFuture method as parseHookResult for readability.
3. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348
ghstack-source-id: 115011710

Differential Revision: [D24374282](https://our.internmc.facebook.com/intern/diff/D24374282/)
…onCommHook."


1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Rename processFuture method as parseHookResult for readability.
3. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348

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

[ghstack-poisoned]
…onCommHook."


1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Rename processFuture method as parseHookResult for readability.
3. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348

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

[ghstack-poisoned]
wayi1 pushed a commit that referenced this pull request Oct 23, 2020
Pull Request resolved: #46512

1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Rename processFuture method as parseHookResult for readability.
3. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348
ghstack-source-id: 115081664

Differential Revision: [D24374282](https://our.internmc.facebook.com/intern/diff/D24374282/)
…onCommHook."


1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Rename processFuture method as parseHookResult for readability.
3. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348

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

[ghstack-poisoned]
wayi1 pushed a commit that referenced this pull request Oct 24, 2020
Pull Request resolved: #46512

1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Rename processFuture method as parseHookResult for readability.
3. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348
ghstack-source-id: 115091355

Differential Revision: [D24374282](https://our.internmc.facebook.com/intern/diff/D24374282/)
…onCommHook."


1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Move the implementation of PythonCommHook destructor from the header file to cpp file.
3. Rename processFuture method as parseHookResult for readability.
4. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348

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

[ghstack-poisoned]
wayi1 pushed a commit that referenced this pull request Oct 26, 2020
Pull Request resolved: #46512

1. Merge 1-line PythonCommHook constructor into the header for simplicity.
2. Move the implementation of PythonCommHook destructor from the header file to cpp file.
3. Rename processFuture method as parseHookResult for readability.
4. Simplify some comments.

Original PR issue: C++ DDP Communication Hook #46348
ghstack-source-id: 115161086

Differential Revision: [D24374282](https://our.internmc.facebook.com/intern/diff/D24374282/)
@codecov
Copy link

codecov bot commented Oct 26, 2020

Codecov Report

Merging #46512 into gh/SciPioneer/15/base will increase coverage by 0.00%.
The diff coverage is n/a.

@@                  Coverage Diff                   @@
##           gh/SciPioneer/15/base   #46512   +/-   ##
======================================================
  Coverage                  68.96%   68.96%           
======================================================
  Files                        434      434           
  Lines                      55996    55996           
======================================================
+ Hits                       38619    38620    +1     
+ Misses                     17377    17376    -1     

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in a6cd294.

@facebook-github-bot facebook-github-bot deleted the gh/SciPioneer/15/head branch October 30, 2020 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

4 participants