Prevent crash if sync_dist=True on CPU #4626
Merged
Conversation
This file contains 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
SeanNaren
requested review from
ananyahjha93,
awaelchli,
Borda,
justusschock,
nateraw,
tchaton,
teddykoker and
williamFalcon
as code owners
|
Hello @SeanNaren! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-11-11 20:29:39 UTC |
tchaton
approved these changes
Nov 11, 2020
rohitgr7
approved these changes
Nov 11, 2020
Codecov Report
@@ Coverage Diff @@
## master #4626 +/- ##
======================================
Coverage 93% 93%
======================================
Files 116 116
Lines 8873 8879 +6
======================================
+ Hits 8254 8261 +7
+ Misses 619 618 -1 |
awaelchli
reviewed
Nov 11, 2020
|
I've decided to take @awaelchli advice and enforce the check across the accelerators, keeping the base class as is. Less liability of running into issues in the future imo. I'll give some time for people to re-review :) cc @tchaton @ananthsub @rohitgr7 |
Borda
reviewed
Nov 11, 2020
| @@ -682,3 +682,69 @@ def get_expected_output(func_attr, original_values): | |||
| assert func_name in trainer.logger_connector.progress_bar_metrics | |||
| else: | |||
| assert func_name not in trainer.logger_connector.progress_bar_metrics | |||
|
|
|||
|
|
|||
| def test_logging_sync_dist_true_cpu(tmpdir): | |||
There was a problem hiding this comment.
can we parametrize this test and test both True/False
also, it seems as the bellow is using the very same class, can we define it just once?
cc: @SeanNaren
Borda
pushed a commit
that referenced
this pull request
Nov 12, 2020
* Added test/fix for sync_dist raising NotImplementedError * Fixed comments/formatting * Revert base class change, enforce sync tensors across accelerators, added GPU test (cherry picked from commit 33470ba)
Borda
pushed a commit
that referenced
this pull request
Nov 12, 2020
* Added test/fix for sync_dist raising NotImplementedError * Fixed comments/formatting * Revert base class change, enforce sync tensors across accelerators, added GPU test (cherry picked from commit 33470ba)
Borda
pushed a commit
that referenced
this pull request
Nov 12, 2020
* Added test/fix for sync_dist raising NotImplementedError * Fixed comments/formatting * Revert base class change, enforce sync tensors across accelerators, added GPU test (cherry picked from commit 33470ba)
Borda
added a commit
that referenced
this pull request
Nov 12, 2020
* Added test/fix for sync_dist raising NotImplementedError * Fixed comments/formatting * Revert base class change, enforce sync tensors across accelerators, added GPU test (cherry picked from commit 33470ba)
rohitgr7
pushed a commit
that referenced
this pull request
Nov 21, 2020
* Added test/fix for sync_dist raising NotImplementedError * Fixed comments/formatting * Revert base class change, enforce sync tensors across accelerators, added GPU test
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.
What does this PR do?
As discussed in the PL slack, if
sync_dist=Trueon CPU the code currently crashes. This regresses behaviour before the latest horovod changes (this bug was introduced in #3775) and breaks lots of functionality whensync_dist=Trueneeds to cover all accelerator cases, including CPU.PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In in short, see following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃