Fix DP Logging Aggregation #4138

justusschock · 2020-10-14T08:55:48Z

What does this PR do?

Fixes a bug in LoggerConnector, where the metric values that should be reduced are not on the same device in DP

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

pep8speaks · 2020-10-14T08:55:52Z

Hello @justusschock! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-12-04 17:30:46 UTC

codecov · 2020-10-14T09:22:44Z

Codecov Report

Merging #4138 (9ce9ea0) into master (ed5bda3) will increase coverage by 0%.
The diff coverage is 90%.

@@          Coverage Diff           @@
##           master   #4138   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         124     124           
  Lines        9294    9299    +5     
======================================
+ Hits         8608    8615    +7     
+ Misses        686     684    -2

justusschock · 2020-10-14T12:02:17Z

@Borda any clue on the failing Mac tests?

awaelchli · 2020-10-14T14:51:37Z

@justusschock It comes from master branch

Borda · 2020-10-14T15:40:26Z

@Borda any clue on the failing Mac tests?

It is related to latest packages...

@justusschock It comes from master branch

Yes, we can check if there was a package upgrade in last days...

edenlightning · 2020-10-19T23:44:44Z

@justusschock fix failing tests?

justusschock · 2020-10-20T05:29:39Z

@edenlightning they should not be related to this pr. But I will check again

SeanNaren · 2020-10-20T12:29:14Z

Can we get a test for this in as well?

awaelchli · 2020-10-27T09:19:38Z

@justusschock is this fix finished, do you need help?

justusschock · 2020-10-27T10:39:37Z

It is not yet finished. I was planning to get this done today or tomorrow (my old fix sin't valid anymore, we talked about this offline). If you have some time you can for sure give it a shot :)

pytorch_lightning/trainer/connectors/logger_connector.py

tchaton

Looks great ! Great catch !

tchaton

Any chance you could add a test ?

awaelchli · 2020-12-03T20:40:35Z

can I just say how frustrated I am how long it took me to realize that BoringModel uses batch size 1 so when running in DP mode it does not actually do anything on gpu 1 and so for a long time I was just not able to make the test fail on master 😩

now the reported bug RuntimeError: All input tensors must be on the same device. Received cuda:2 and cuda:3 is fixed and the test that I added fails on master with this error message.

rohitgr7 · 2020-12-03T21:34:07Z

@adityak2920 is this the issue you were facing?? Does it solve your use-case?

adityak2920 · 2020-12-03T21:43:06Z

@adityak2920 is this the issue you were facing?? Does it solve your use-case?

Yes, I was facing this issue and will solve my use case.

CHANGELOG.md

Borda · 2020-12-04T16:40:48Z

@awaelchli @SeanNaren mind review?

awaelchli · 2020-12-04T16:45:48Z

it requires also @tchaton review because he had requested changes.

pls re-review

move result stupid result object revert to master undo import add "to" method to result generalize to try a test try a test Revert "try a test" This reverts commit 22e3c10. Revert "try a test" This reverts commit 4d2d8fb. new test max epochs super epoch end log in test hanging test undo test initial test that fails on master step end pass step end step end epoch end print step check dev clean up test sanity check wtf is go ing on frustration debugging test test test test test test test test test unused import

pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py

tchaton

LGTM !

pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py

mergify bot requested a review from a team October 14, 2020 08:56

justusschock requested review from awaelchli, Borda, nateraw and williamFalcon October 14, 2020 08:58

justusschock self-assigned this Oct 14, 2020

justusschock added the bug Something isn't working label Oct 14, 2020

justusschock mentioned this pull request Oct 14, 2020

Data Parallel bug (return outputs not being moved to same device) #4073

Closed

edenlightning added this to the 1.0.3 milestone Oct 19, 2020

justusschock requested review from SeanNaren, ananyahjha93, rohitgr7, s-rog and tchaton October 20, 2020 11:57

awaelchli requested a review from teddykoker as a code owner October 25, 2020 07:14

EspenHa mentioned this pull request Oct 26, 2020

Metrics fail on DP and multiple GPU #4353

Closed

Shiaoming reviewed Nov 3, 2020

View reviewed changes

pytorch_lightning/trainer/connectors/logger_connector.py Outdated Show resolved Hide resolved

tchaton approved these changes Nov 4, 2020

View reviewed changes

tchaton previously requested changes Nov 4, 2020

View reviewed changes

edenlightning removed this from the 1.0.x milestone Nov 10, 2020

Borda added logging Related to the `LoggerConnector` and `log()` priority: 0 High priority task labels Dec 3, 2020

Borda approved these changes Dec 3, 2020

View reviewed changes

Borda requested a review from tchaton December 3, 2020 20:37

rohitgr7 approved these changes Dec 3, 2020

View reviewed changes

rohitgr7 reviewed Dec 3, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Borda added the ready PRs ready to be merged label Dec 4, 2020

awaelchli approved these changes Dec 4, 2020

View reviewed changes

justusschock and others added 7 commits December 4, 2020 18:04

add option to step result to do aggregation on a specific device

c7148e5

in dp: do aggregation on root gpu

096ea27

Update CHANGELOG.md

46df9db

pep8

8e8c6fe

trailing whitespace

cb23dee

move chlog entry

ec1f526

Borda force-pushed the fix_dp_logging_aggregation branch from bfe5395 to ec1f526 Compare December 4, 2020 17:16

tchaton reviewed Dec 4, 2020

View reviewed changes

pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py Outdated Show resolved Hide resolved

clean

da623a3

tchaton approved these changes Dec 4, 2020

View reviewed changes

awaelchli reviewed Dec 4, 2020

View reviewed changes

pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py Outdated Show resolved Hide resolved

remove outdated changes

9ce9ea0

awaelchli mentioned this pull request Dec 4, 2020

Weird behaviour multi-GPU (dp, gpus > 1) tensors not converted to cuda #4423

Closed

Borda merged commit f23f5e5 into master Dec 4, 2020

Borda deleted the fix_dp_logging_aggregation branch December 4, 2020 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DP Logging Aggregation #4138

Fix DP Logging Aggregation #4138

justusschock commented Oct 14, 2020

pep8speaks commented Oct 14, 2020 •

edited

Loading

codecov bot commented Oct 14, 2020 •

edited

Loading

justusschock commented Oct 14, 2020

awaelchli commented Oct 14, 2020

Borda commented Oct 14, 2020

edenlightning commented Oct 19, 2020

justusschock commented Oct 20, 2020

SeanNaren commented Oct 20, 2020

awaelchli commented Oct 27, 2020

justusschock commented Oct 27, 2020

tchaton left a comment

tchaton left a comment •

edited

Loading

awaelchli commented Dec 3, 2020

rohitgr7 commented Dec 3, 2020 •

edited

Loading

adityak2920 commented Dec 3, 2020

Borda commented Dec 4, 2020

awaelchli commented Dec 4, 2020

tchaton left a comment

Fix DP Logging Aggregation #4138

Fix DP Logging Aggregation #4138

Conversation

justusschock commented Oct 14, 2020

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Oct 14, 2020 • edited Loading

Comment last updated at 2020-12-04 17:30:46 UTC

codecov bot commented Oct 14, 2020 • edited Loading

Codecov Report

justusschock commented Oct 14, 2020

awaelchli commented Oct 14, 2020

Borda commented Oct 14, 2020

edenlightning commented Oct 19, 2020

justusschock commented Oct 20, 2020

SeanNaren commented Oct 20, 2020

awaelchli commented Oct 27, 2020

justusschock commented Oct 27, 2020

tchaton left a comment

Choose a reason for hiding this comment

tchaton left a comment • edited Loading

Choose a reason for hiding this comment

awaelchli commented Dec 3, 2020

rohitgr7 commented Dec 3, 2020 • edited Loading

adityak2920 commented Dec 3, 2020

Borda commented Dec 4, 2020

awaelchli commented Dec 4, 2020

tchaton left a comment

Choose a reason for hiding this comment

pep8speaks commented Oct 14, 2020 •

edited

Loading

codecov bot commented Oct 14, 2020 •

edited

Loading

tchaton left a comment •

edited

Loading

rohitgr7 commented Dec 3, 2020 •

edited

Loading