add ddp sync for logging in result step

[justusschock]

What does this PR do?

Adds the possibility to sync between DDP Processes with the result object

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]

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-08-05 08:07:18 UTC

[justusschock]

I will fix pep8 and retriever tests once #2568 is merged :)

[Borda]

I will fix pep8 and retriever tests once #2568 is merged :)

it has been merged...

[codecov]

Codecov Report

Merging #2822 into master will decrease coverage by 0%.
The diff coverage is 62%.

@@          Coverage Diff           @@
##           master   #2822   +/-   ##
======================================
- Coverage      90%     90%   -0%     
======================================
  Files          78      78           
  Lines        6921    6925    +4     
======================================
+ Hits         6238    6241    +3     
- Misses        683     684    +1     

[justusschock]

Okay, I'll revert those changes. That was automatically formatted, since there were some pep8 issues :D

[Borda]

Okay, I'll revert those changes. That was automatically formatted, since there were some pep8 issues :D

I think that pep8, is not soo strict about " and ' and as there is no wider consensus which shall be used I would drop this format changes... but you are free to re-open the question and et a conclusion :]

[xiadingZ]

Will this sync_ddp_op in Result conflict with reduce_op in TensorMetric?
If I set reduce_op of TensorMetric as SUM, then set sync_ddp_op in Result as 'mean', what will happen?

[justusschock]

@xiadingZ When you reduce your metric with SUM, you will get what you expected (a sum across all processes in DDP) on every node. When you now sync it again with Result it will now give you the mean across all projects (which should not change anything in this case, since all values should be identical now, however if you use an additional SUM here, it changes something).

The general way to go here is: Whenever something has built-in support for DDP Sync, use this one, since we may have customised it (currently we are working on providing a more useful way to sync certain metrics). If not, use the sync from results.