Remove dead code in ddp.{h, cpp} #37990
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The code in `ddp.{h, cpp}` and the corresponding pybind implementations are no longer used. The pybinded calls were all private APIs and only ran in unittests, so we should remove these unused APIs.
#20234 from a year ago also mentioned that we should delete `_dist_broadcast_coalesced`
Verified that all tests pass with cuda by running `test_c10d` on a gpu-enabled machine.
Differential Revision: [D21443879](https://our.internmc.facebook.com/intern/diff/D21443879/)
**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21443879/)!
[ghstack-poisoned]
rohan-varma
requested review from
mrshenli,
pietern,
pritamdamania87 and
zhaojuanmao
as code owners
The code in `ddp.{h, cpp}` and the corresponding pybind implementations are no longer used. The pybinded calls were all private APIs and only ran in unittests, so we should remove these unused APIs.
#20234 from a year ago also mentioned that we should delete `_dist_broadcast_coalesced`
Verified that all tests pass with cuda by running `test_c10d` on a gpu-enabled machine.
Differential Revision: [D21443879](https://our.internmc.facebook.com/intern/diff/D21443879/)
**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21443879/)!
[ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
May 7, 2020
Pull Request resolved: #37990 The code in `ddp.{h, cpp}` and the corresponding pybind implementations are no longer used. The pybinded calls were all private APIs and only ran in unittests, so we should remove these unused APIs. #20234 from a year ago also mentioned that we should delete `_dist_broadcast_coalesced` Verified that all tests pass with cuda by running `test_c10d` on a gpu-enabled machine. ghstack-source-id: 103638483 Differential Revision: [D21443879](https://our.internmc.facebook.com/intern/diff/D21443879/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21443879/)!
💊 CI failures summary and remediationsAs of commit fb06ad8 (more details on the Dr. CI page):
Extra GitHub checks: 1 failed
ci.pytorch.org: 1 failedThis 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. This comment has been revised 20 times. |
The code in `ddp.{h, cpp}` and the corresponding pybind implementations are no longer used. The pybinded calls were all private APIs and only ran in unittests, so we should remove these unused APIs.
#20234 from a year ago also mentioned that we should delete `_dist_broadcast_coalesced`
Verified that all tests pass with cuda by running `test_c10d` on a gpu-enabled machine.
Differential Revision: [D21443879](https://our.internmc.facebook.com/intern/diff/D21443879/)
**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21443879/)!
[ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
May 7, 2020
Pull Request resolved: #37990 The code in `ddp.{h, cpp}` and the corresponding pybind implementations are no longer used. The pybinded calls were all private APIs and only ran in unittests, so we should remove these unused APIs. #20234 from a year ago also mentioned that we should delete `_dist_broadcast_coalesced` Verified that all tests pass with cuda by running `test_c10d` on a gpu-enabled machine. ghstack-source-id: 103647579 Differential Revision: [D21443879](https://our.internmc.facebook.com/intern/diff/D21443879/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21443879/)!
mrshenli
approved these changes
May 7, 2020
Contributor
Author
|
@mrshenli Actually it looks like some of the test issues are related. In |
rohan-varma
added a commit
that referenced
this pull request
May 7, 2020
`common_distributed` and `test_distributed` have some error codes that overlap but are for different reasons, for example, code 75 in `test_distributed` is "no cuda available" but in common_distributed it is "need at least 2 CUDA devices". This is an issue because the tests in `test_distributed` now use the utils in `common_distributed`, so we could get the wrong reason for skipping tests. It is also the source of test failures in #37990. This diff makes it so that the test skipping logic is deduped and put into `common_distributed.py`, where it can be reused and then imported into `test_distributed` Differential Revision: [D21466768](https://our.internmc.facebook.com/intern/diff/D21466768/) [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
May 7, 2020
…buted" `common_distributed` and `test_distributed` have some error codes that overlap but are for different reasons, for example, code 75 in `test_distributed` is "no cuda available" but in common_distributed it is "need at least 2 CUDA devices". This is an issue because the tests in `test_distributed` now use the utils in `common_distributed`, so we could get the wrong reason for skipping tests. It is also the source of test failures in #37990. This diff makes it so that the test skipping logic is deduped and put into `common_distributed.py`, where it can be reused and then imported into `test_distributed` Differential Revision: [D21466768](https://our.internmc.facebook.com/intern/diff/D21466768/) [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
May 7, 2020
Pull Request resolved: #38078 `common_distributed` and `test_distributed` have some error codes that overlap but are for different reasons, for example, code 75 in `test_distributed` is "no cuda available" but in common_distributed it is "need at least 2 CUDA devices". This is an issue because the tests in `test_distributed` now use the utils in `common_distributed`, so we could get the wrong reason for skipping tests. It is also the source of test failures in #37990. This diff makes it so that the test skipping logic is deduped and put into `common_distributed.py`, where it can be reused and then imported into `test_distributed` ghstack-source-id: 103713465 Differential Revision: [D21466768](https://our.internmc.facebook.com/intern/diff/D21466768/)
The code in `ddp.{h, cpp}` and the corresponding pybind implementations are no longer used. The pybinded calls were all private APIs and only ran in unittests, so we should remove these unused APIs.
#20234 from a year ago also mentioned that we should delete `_dist_broadcast_coalesced`
Verified that all tests pass with cuda by running `test_c10d` on a gpu-enabled machine.
Differential Revision: [D21443879](https://our.internmc.facebook.com/intern/diff/D21443879/)
**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21443879/)!
[ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
May 8, 2020
Pull Request resolved: #37990 The code in `ddp.{h, cpp}` and the corresponding pybind implementations are no longer used. The pybinded calls were all private APIs and only ran in unittests, so we should remove these unused APIs. #20234 from a year ago also mentioned that we should delete `_dist_broadcast_coalesced` Verified that all tests pass with cuda by running `test_c10d` on a gpu-enabled machine. ghstack-source-id: 103716041 Differential Revision: [D21443879](https://our.internmc.facebook.com/intern/diff/D21443879/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21443879/)!
rohan-varma
added a commit
that referenced
this pull request
May 8, 2020
…buted" `common_distributed` and `test_distributed` have some error codes that overlap but are for different reasons, for example, code 75 in `test_distributed` is "no cuda available" but in common_distributed it is "need at least 2 CUDA devices". This is an issue because the tests in `test_distributed` now use the utils in `common_distributed`, so we could get the wrong reason for skipping tests. It is also the source of test failures in #37990. This diff makes it so that the test skipping logic is deduped and put into `common_distributed.py`, where it can be reused and then imported into `test_distributed` Differential Revision: [D21466768](https://our.internmc.facebook.com/intern/diff/D21466768/) [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
May 8, 2020
…buted" `common_distributed` and `test_distributed` have some error codes that overlap but are for different reasons, for example, code 75 in `test_distributed` is "no cuda available" but in common_distributed it is "need at least 2 CUDA devices". This is an issue because the tests in `test_distributed` now use the utils in `common_distributed`, so we could get the wrong reason for skipping tests. It is also the source of test failures in #37990. This diff makes it so that the test skipping logic is deduped and put into `common_distributed.py`, where it can be reused and then imported into `test_distributed` Differential Revision: [D21466768](https://our.internmc.facebook.com/intern/diff/D21466768/) [ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
May 8, 2020
Pull Request resolved: #38078 `common_distributed` and `test_distributed` have some error codes that overlap but are for different reasons, for example, code 75 in `test_distributed` is "no cuda available" but in common_distributed it is "need at least 2 CUDA devices". This is an issue because the tests in `test_distributed` now use the utils in `common_distributed`, so we could get the wrong reason for skipping tests. It is also the source of test failures in #37990. This diff makes it so that the test skipping logic is deduped and put into `common_distributed.py`, where it can be reused and then imported into `test_distributed` ghstack-source-id: 103782583 Differential Revision: [D21466768](https://our.internmc.facebook.com/intern/diff/D21466768/)
facebook-github-bot
pushed a commit
that referenced
this pull request
May 9, 2020
Summary: Pull Request resolved: #38078 `common_distributed` and `test_distributed` have some error codes that overlap but are for different reasons, for example, code 75 in `test_distributed` is "no cuda available" but in common_distributed it is "need at least 2 CUDA devices". This is an issue because the tests in `test_distributed` now use the utils in `common_distributed`, so we could get the wrong reason for skipping tests. It is also the source of test failures in #37990. This diff makes it so that the test skipping logic is deduped and put into `common_distributed.py`, where it can be reused and then imported into `test_distributed` ghstack-source-id: 103782583 Test Plan: CI Differential Revision: D21466768 fbshipit-source-id: 53b5af36672ebd8b51ba8b42709d87e96cadef20
The code in `ddp.{h, cpp}` and the corresponding pybind implementations are no longer used. The pybinded calls were all private APIs and only ran in unittests, so we should remove these unused APIs.
#20234 from a year ago also mentioned that we should delete `_dist_broadcast_coalesced`
Verified that all tests pass with cuda by running `test_c10d` on a gpu-enabled machine.
Differential Revision: [D21443879](https://our.internmc.facebook.com/intern/diff/D21443879/)
**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21443879/)!
[ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
May 11, 2020
Pull Request resolved: #37990 The code in `ddp.{h, cpp}` and the corresponding pybind implementations are no longer used. The pybinded calls were all private APIs and only ran in unittests, so we should remove these unused APIs. #20234 from a year ago also mentioned that we should delete `_dist_broadcast_coalesced` Verified that all tests pass with cuda by running `test_c10d` on a gpu-enabled machine. ghstack-source-id: 103871258 Differential Revision: [D21443879](https://our.internmc.facebook.com/intern/diff/D21443879/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21443879/)!
The code in `ddp.{h, cpp}` and the corresponding pybind implementations are no longer used. The pybinded calls were all private APIs and only ran in unittests, so we should remove these unused APIs.
#20234 from a year ago also mentioned that we should delete `_dist_broadcast_coalesced`
Verified that all tests pass with cuda by running `test_c10d` on a gpu-enabled machine.
Differential Revision: [D21443879](https://our.internmc.facebook.com/intern/diff/D21443879/)
**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21443879/)!
[ghstack-poisoned]
rohan-varma
added a commit
that referenced
this pull request
May 11, 2020
Pull Request resolved: #37990 The code in `ddp.{h, cpp}` and the corresponding pybind implementations are no longer used. The pybinded calls were all private APIs and only ran in unittests, so we should remove these unused APIs. #20234 from a year ago also mentioned that we should delete `_dist_broadcast_coalesced` Verified that all tests pass with cuda by running `test_c10d` on a gpu-enabled machine. ghstack-source-id: 103885383 Differential Revision: [D21443879](https://our.internmc.facebook.com/intern/diff/D21443879/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21443879/)!
Contributor
|
This pull request has been merged in 906c50e. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
The code in
ddp.{h, cpp}and the corresponding pybind implementations are no longer used. The pybinded calls were all private APIs and only ran in unittests, so we should remove these unused APIs.#20234 from a year ago also mentioned that we should delete
_dist_broadcast_coalescedVerified that all tests pass with cuda by running
test_c10don a gpu-enabled machine.Differential Revision: D21443879
NOTE FOR REVIEWERS: This PR has internal Facebook specific changes or comments, please review them on Phabricator!