Skip to content

[bugfix] Perform reduction for dict in training_step and DP #6324

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Mar 4, 2021
Merged

[bugfix] Perform reduction for dict in training_step and DP #6324

merged 14 commits into from
Mar 4, 2021

Conversation

@tchaton
Copy link
Contributor

@tchaton tchaton commented Mar 3, 2021 •
edited
Loading

What does this PR do?

See title.

Fixes #6193

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • 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?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

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 short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

@tchaton tchaton added the bug Something isn't working label Mar 3, 2021
@tchaton tchaton added this to the 1.2.x milestone Mar 3, 2021
@tchaton tchaton requested a review from awaelchli as a code owner March 3, 2021 15:56
@tchaton tchaton self-assigned this Mar 3, 2021
carmocca
carmocca reviewed Mar 3, 2021
View reviewed changes
@codecov
Copy link

codecov bot commented Mar 3, 2021 •
edited
Loading

Codecov Report

Merging #6324 (f44ded5) into master (49c579f) will decrease coverage by 47%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #6324     +/-   ##
========================================
- Coverage      93%     46%    -47%     
========================================
  Files         160     160             
  Lines       11415   11304    -111     
========================================
- Hits        10644    5247   -5397     
- Misses        771    6057   +5286

@tchaton tchaton enabled auto-merge (squash) March 3, 2021 17:11
@carmocca carmocca added the ready PRs ready to be merged label Mar 3, 2021
kaushikb11
kaushikb11 reviewed Mar 4, 2021
View reviewed changes
@Borda Borda force-pushed the bugfix/6193_dp_dont_reduce_dict branch from ab8a942 to 9b1e45f Compare March 4, 2021 19:52
@Borda Borda requested a review from kaushikb11 March 4, 2021 19:52
@mergify mergify bot added the has conflicts label Mar 4, 2021
@mergify mergify bot removed the has conflicts label Mar 4, 2021
@pep8speaks
Copy link

pep8speaks commented Mar 4, 2021 •
edited
Loading

Hello @tchaton! Thanks for updating this PR.

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

Comment last updated at 2021-03-04 22:49:41 UTC

@tchaton tchaton merged commit 248a8e8 into master Mar 4, 2021
@tchaton tchaton deleted the bugfix/6193_dp_dont_reduce_dict branch March 4, 2021 23:10
awaelchli
awaelchli reviewed Mar 8, 2021
View reviewed changes
tests/accelerators/test_dp.py
@RunIf(min_gpus=2)
def test_dp_training_step_dict(tmpdir):
"""
This test verify dp properly reduce dictionaries
Copy link
Contributor

@awaelchli awaelchli Mar 4, 2021

Choose a reason for hiding this comment

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

This test does not do what it says. It just makes sure that nothing crashes, but there are no assertions that the behavior is correct.
I bet if we change mean to something else, this test will not fail.

Copy link
Contributor

@awaelchli awaelchli Mar 8, 2021

Choose a reason for hiding this comment

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

besides, nothing gets reduced here anyway because BoringModel uses batch size 1 for all dataloaders and so this never runs on more than 1 gpu

@awaelchli awaelchli mentioned this pull request Mar 8, 2021
Merged
11 tasks
@tchaton tchaton mentioned this pull request Mar 8, 2021
Closed
SeanNaren pushed a commit that referenced this pull request Mar 16, 2021
@tchaton
[bugfix] Perform reduction for dict in training_step and DP (#6324)
d202151 
* fix

* update

* update

* add changelog

* Update CHANGELOG.md

Co-authored-by: Carlos Mocholí <[email protected]>

* Update tests/accelerators/test_dp.py

Co-authored-by: Carlos Mocholí <[email protected]>

* update changelog

Co-authored-by: Carlos Mocholí <[email protected]>

(cherry picked from commit 248a8e8)
Borda pushed a commit that referenced this pull request Mar 16, 2021
@tchaton @Borda
[bugfix] Perform reduction for dict in training_step and DP (#6324)
0560c97 
* fix

* update

* update

* add changelog

* Update CHANGELOG.md

Co-authored-by: Carlos Mocholí <[email protected]>

* Update tests/accelerators/test_dp.py

Co-authored-by: Carlos Mocholí <[email protected]>

* update changelog

Co-authored-by: Carlos Mocholí <[email protected]>

(cherry picked from commit 248a8e8)
lexierule pushed a commit that referenced this pull request Mar 16, 2021
@tchaton @lexierule
[bugfix] Perform reduction for dict in training_step and DP (#6324)
73ef543 
* fix

* update

* update

* add changelog

* Update CHANGELOG.md

Co-authored-by: Carlos Mocholí <[email protected]>

* Update tests/accelerators/test_dp.py

Co-authored-by: Carlos Mocholí <[email protected]>

* update changelog

Co-authored-by: Carlos Mocholí <[email protected]>

(cherry picked from commit 248a8e8)
This was referenced Mar 17, 2021
Closed
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Borda Borda Borda approved these changes

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

+3 more reviewers

@awaelchli awaelchli awaelchli left review comments

@SeanNaren SeanNaren SeanNaren approved these changes

@carmocca carmocca carmocca approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tchaton tchaton

Labels

bug Something isn't working ready PRs ready to be merged

Projects

None yet

Milestone

pl:1.2.x

Development

Successfully merging this pull request may close these issues.

Error in returning Dict from training_step with multiple GPUs

7 participants

@tchaton @pep8speaks @awaelchli @Borda @SeanNaren @carmocca @kaushikb11