[bug-fix] Call transfer_batch_to_device in DDPlugin #5195

tchaton · 2020-12-19T22:30:55Z

What does this PR do?

This PR moves transfer_batch_to_device from ddp_sharded to ddp_plugin in on_before_forward

Partially solves #2350

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.
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; Bugfixes should be including in bug-fix release milestones (m.f.X) and features should be included in (m.X.b) releases.

Did you have fun?

Make sure you had fun coding 🙃

codecov · 2020-12-19T22:33:04Z

Codecov Report

Merging #5195 (e0358df) into master (72525f0) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #5195   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         134     134           
  Lines        9994    9998    +4     
======================================
+ Hits         9309    9314    +5     
+ Misses        685     684    -1

pytorch_lightning/accelerators/ddp2_accelerator.py

SeanNaren · 2020-12-20T19:07:03Z

So a couple of things:

on_before_forward is moving towards hook based functionality across the plugins. This I think is preferred and in the original PR we moved from move_to_device to on_before_forward. It allows the plugins to decide what logic is appropriate before the forward function.

We should not enforce move to device. This wouldn't work in cases where custom distributed accelerators need to move device differently imo. It should be up to the plugin to do what's necessary. Also when using DDP we do not need to manually move tensors to the appropriate device?

…htning into pyg

pep8speaks · 2020-12-21T08:47:23Z

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-01-08 15:39:36 UTC

SeanNaren · 2020-12-23T10:43:31Z

@tchaton can you have a look at the above comment? I don't think we should merge this.

carmocca · 2021-01-06T11:51:38Z

Can you update the CHANGELOG? (after #5378 is merged)

awaelchli

not liking the device_ids assignment but we will be able to handle that more elegantly through the refactor. otherwise LGTM thanks for taking a look at this.

* hacking out * update * remove useless on_before_forward * update * remove overriden * iremove os * use on_before_forward * resolve flake8 * add test * update * add single_process_per_device * resolve flake8 * update * resolve * update * update * update * add comment * resolve bug with sharded * update * remove property * update * resolve test * resolve bug * update on comments * update doc * Update pytorch_lightning/core/hooks.py Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> * update on comments * Update pytorch_lightning/plugins/ddp_plugin.py Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> * Update pytorch_lightning/plugins/ddp_plugin.py Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> * resolve pep8 * add device_ids to pipe * update on comments * update * resolve * update * update * update Co-authored-by: Ubuntu <ubuntu@ip-172-31-62-109.ec2.internal> Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> Co-authored-by: Sean Naren <sean.narenthiran@gmail.com> (cherry picked from commit d510707)

Ubuntu added 2 commits December 19, 2020 21:05

hacking out

d156d8a

update

658dfcd

tchaton requested review from awaelchli, Borda, justusschock, SeanNaren and williamFalcon as code owners December 19, 2020 22:30

tchaton changed the title ~~Pyg~~ [bug-fix] Call transfer_batch_to_device in DDPlugin Dec 19, 2020

tchaton self-assigned this Dec 19, 2020

tchaton added this to the 1.1.x milestone Dec 19, 2020

tchaton added the priority: 1 Medium priority task label Dec 19, 2020

awaelchli reviewed Dec 19, 2020

View reviewed changes

pytorch_lightning/accelerators/ddp2_accelerator.py Outdated Show resolved Hide resolved

Ubuntu added 4 commits December 20, 2020 18:41

remove useless on_before_forward

526e9e6

update

7396760

remove overriden

dba7875

iremove os

895dc72

tchaton and others added 3 commits December 21, 2020 08:36

Merge branch 'master' into pyg

3b08bd9

use on_before_forward

ed0eb42

Merge branch 'pyg' of https://github.com/PyTorchLightning/pytorch-lig…

461a978

…htning into pyg

tchaton added 3 commits December 22, 2020 09:24

Merge branch 'master' into pyg

bfe3793

Merge branch 'master' into pyg

76fd6c9

resolve flake8

31587f9

Merge branch 'master' into pyg

bbea81a

tchaton closed this Dec 27, 2020

tchaton deleted the pyg branch December 27, 2020 17:58

tchaton restored the pyg branch December 28, 2020 15:18

tchaton added 3 commits January 6, 2021 09:17

update

525d26d

Merge branch 'master' into pyg

779a672

update

34def10

tchaton added 3 commits January 6, 2021 15:46

Merge branch 'master' into pyg

57b037e

update

f679440

Merge branch 'master' into pyg

37fb96e

SeanNaren mentioned this pull request Jan 6, 2021

DDP not moving batch to device? #4987

Closed

awaelchli approved these changes Jan 7, 2021

View reviewed changes

s-rog mentioned this pull request Jan 7, 2021

Add before_batch_transfer and after_batch_transfer hooks #3671

Merged

7 tasks

Merge branch 'master' into pyg

64ef06a

awaelchli mentioned this pull request Jan 8, 2021

WIP: Accelerator refactor #5385

Closed

9 tasks

williamFalcon approved these changes Jan 8, 2021

View reviewed changes

tchaton enabled auto-merge (squash) January 8, 2021 15:39

Merge branch 'master' into pyg

e0358df

tchaton merged commit d510707 into master Jan 8, 2021

tchaton deleted the pyg branch January 8, 2021 17:05

justusschock mentioned this pull request Jan 22, 2021

PoC: Accelerator refactor [wip] [skip ci] #5616

Closed

16 tasks

SeanNaren mentioned this pull request Jan 26, 2021

Regression between Lightning 1.1.3 and 1.1.5 #5656

Closed

justusschock mentioned this pull request Feb 2, 2021

PoC: Accelerator refactor #5743

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bug-fix] Call transfer_batch_to_device in DDPlugin #5195

[bug-fix] Call transfer_batch_to_device in DDPlugin #5195

tchaton commented Dec 19, 2020 •

edited

Loading

codecov bot commented Dec 19, 2020 •

edited

Loading

SeanNaren commented Dec 20, 2020

pep8speaks commented Dec 21, 2020 •

edited

Loading

SeanNaren commented Dec 23, 2020

carmocca commented Jan 6, 2021

awaelchli left a comment

[bug-fix] Call transfer_batch_to_device in DDPlugin #5195

[bug-fix] Call transfer_batch_to_device in DDPlugin #5195

Conversation

tchaton commented Dec 19, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented Dec 19, 2020 • edited Loading

Codecov Report

SeanNaren commented Dec 20, 2020

pep8speaks commented Dec 21, 2020 • edited Loading

Comment last updated at 2021-01-08 15:39:36 UTC

SeanNaren commented Dec 23, 2020

carmocca commented Jan 6, 2021

awaelchli left a comment

Choose a reason for hiding this comment

tchaton commented Dec 19, 2020 •

edited

Loading

codecov bot commented Dec 19, 2020 •

edited

Loading

pep8speaks commented Dec 21, 2020 •

edited

Loading