data transfer model hook (+ refactor) #1756

awaelchli · 2020-05-07T16:45:33Z

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 to update the docs?
Did you write any new necessary tests?
If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Part 1 of feature #1245 by introducing a hook.

Refactored data transfer method such that all device information is contained in torch.device (necessary for model hook)
Removed .cuda() call since it is obsolete (can use .to() for every tensor)
Added a model hook which is called for all device transfers and by default implements the supported datatypes. The user can override this model hook to implement transfers of custom batch types.

Open Questions:

What do we call the hook? transfer_data_to_device? transfer_batch_to_device? transfer_batch?

Other PRs that are blocked by this: #1729, #1526

Link to test TPU works with this branch:
https://colab.research.google.com/drive/1wy6sbl8Bh6S3QaHzBsJNbhQ6UX1IC864?usp=sharing

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 🙃

codecov · 2020-05-09T02:22:15Z

Codecov Report

Merging #1756 into master will increase coverage by 0%.
The diff coverage is 76%.

@@          Coverage Diff           @@
##           master   #1756   +/-   ##
======================================
  Coverage      86%     86%           
======================================
  Files          74      75    +1     
  Lines        4713    4705    -8     
======================================
- Hits         4070    4064    -6     
+ Misses        643     641    -2

mergify · 2020-05-09T03:32:35Z

This pull request is now in conflict... :(

mergify · 2020-05-14T21:57:07Z

This pull request is now in conflict... :(

pytorch_lightning/trainer/distrib_parts.py

mergify · 2020-05-17T20:31:51Z

This pull request is now in conflict... :(

pytorch_lightning/trainer/distrib_parts.py

awaelchli · 2020-05-19T23:50:49Z

@justusschock @Borda I factored out the transfer function to a utility and wanted to ask you if it is a good place to put it next to the apply_to_collections.
Motivation was to make it accessible to user when they do their custom batch transfer stuff in the model hook.

justusschock

LGTM, just some missing docstrings :)

pytorch_lightning/utilities/apply_func.py

pytorch_lightning/trainer/distrib_parts.py

Borda

LGTM 🚀

awaelchli · 2020-06-03T00:20:52Z

@williamFalcon true that is confusing.
I renamed it now.

ZhaofengWu · 2020-06-09T20:56:25Z

To confirm, this does not work for DDP, because in DDP we use the default scatter to move the tensors, correct? Is there a way to similarly customize this behavior for DDP?
https://github.com/PyTorchLightning/pytorch-lightning/blob/16a7326e5259a3cdd20a508c34a0f84806d88f8e/pytorch_lightning/trainer/training_loop.py#L736-L737

awaelchli · 2020-06-23T00:38:01Z

@ZhaofengWu I did not know this. I thought the Trainer always called the same function to move data to the device. I searched in the PyTorch docs for DP and DDP but it seems to me it is not possible to override the scattering of custom batch objects. I guess the best we could do is add a note to our docs?

ZhaofengWu · 2020-06-23T00:58:39Z

I don't know enough about this to know if there's any workaround. It'd be great if this override consistently works in all scenarios, but I guess if it doesn't work, it doesn't work.

ZhaofengWu · 2020-06-23T00:59:07Z

But yes, in any case, at least there should be a note.

ZhaofengWu · 2020-06-24T06:19:19Z

@awaelchli Do you want me to open a separate issue?

awaelchli · 2020-06-24T12:06:53Z

yes good idea, could you do that please?

* refactor and added hook variant a variant b add test revert rename add changelog docs * resolve merge duplication * overridden typo * fix test * tpu id * raise if TPU not available * re-use apply_to_collection function for parsing collections * comment * make utility function available to user * documentation * move changelog entry to top * fix tpu transfer call * fix call * remove hardcoded string * improve test * call model hook by default * Apply suggestions from code review * rename utility function Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

mergify bot requested a review from a team May 7, 2020 16:46

awaelchli changed the title ~~refactor data transfer + hook~~ [blocked by #1729, #1526] refactor data transfer + hook May 9, 2020

awaelchli added the feature Is an improvement or enhancement label May 9, 2020

awaelchli marked this pull request as ready for review May 9, 2020 02:58

awaelchli force-pushed the feature/data-transfer-hook branch from 06c1ac2 to 7aa093d Compare May 14, 2020 21:47

awaelchli force-pushed the feature/data-transfer-hook branch from 7aa093d to cbdefc8 Compare May 14, 2020 22:40

justusschock reviewed May 15, 2020

View reviewed changes

pytorch_lightning/trainer/distrib_parts.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team May 15, 2020 05:58

awaelchli changed the title ~~[blocked by #1729, #1526] refactor data transfer + hook~~ [blocked by #1729, #1526] refactor data transfer + hook [wip] May 15, 2020

awaelchli changed the title ~~[blocked by #1729, #1526] refactor data transfer + hook [wip]~~ [blocked by #1729, #1526] refactor data transfer + hook May 15, 2020

awaelchli changed the title ~~[blocked by #1729, #1526] refactor data transfer + hook~~ [blocked by #1526] refactor data transfer + hook May 18, 2020

awaelchli force-pushed the feature/data-transfer-hook branch from 18dff98 to c7e4493 Compare May 18, 2020 03:20

awaelchli changed the title ~~[blocked by #1526] refactor data transfer + hook~~ refactor data transfer + hook May 18, 2020

awaelchli changed the title ~~refactor data transfer + hook~~ data transfer model hook (+ refactor) May 18, 2020

justusschock reviewed May 19, 2020

View reviewed changes

pytorch_lightning/trainer/distrib_parts.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team May 19, 2020 06:04

awaelchli force-pushed the feature/data-transfer-hook branch from c7e4493 to c884f68 Compare May 19, 2020 22:41

awaelchli mentioned this pull request May 19, 2020

Refactor model summary + generalize example input array #1773

Merged

5 tasks

awaelchli requested a review from justusschock May 19, 2020 23:47

awaelchli mentioned this pull request May 20, 2020

[blocked by #1756] Add decorator to auto-move data for inference #1526

Closed

5 tasks

justusschock approved these changes May 20, 2020

View reviewed changes

pytorch_lightning/utilities/apply_func.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/distrib_parts.py Outdated Show resolved Hide resolved

mergify bot requested a review from a team May 20, 2020 06:31

Borda approved these changes May 20, 2020

View reviewed changes

mergify bot requested a review from a team May 20, 2020 12:55

awaelchli requested a review from williamFalcon May 20, 2020 17:33

awaelchli and others added 16 commits June 3, 2020 01:00

overridden typo

0b4fc00

fix test

e367560

tpu id

365c2da

raise if TPU not available

1b8de29

re-use apply_to_collection function for parsing collections

e3de6e7

comment

1c1b155

make utility function available to user

7f31a78

documentation

2d348a9

move changelog entry to top

39eca5e

fix tpu transfer call

97ad5c7

fix call

e0f06ba

remove hardcoded string

225f4a0

improve test

0a6c7fc

call model hook by default

832da9a

Apply suggestions from code review

47b4693

rename utility function

ea9fab3

awaelchli force-pushed the feature/data-transfer-hook branch from 97c9fdb to ea9fab3 Compare June 2, 2020 23:10

williamFalcon merged commit 8211256 into master Jun 3, 2020

Borda deleted the feature/data-transfer-hook branch June 3, 2020 05:59

awaelchli mentioned this pull request Jun 23, 2020

Fix lost compatibility with custom datatypes implementing .to #2335

Merged

7 tasks

ZhaofengWu mentioned this pull request Jun 24, 2020

transfer_batch_to_device doesn't work under DP #2350

Closed

Tieftaucher mentioned this pull request Jul 2, 2020

support of other datatypes for Batches #2465

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data transfer model hook (+ refactor) #1756

data transfer model hook (+ refactor) #1756

awaelchli commented May 7, 2020 •

edited

Loading

codecov bot commented May 9, 2020 •

edited

Loading

mergify bot commented May 9, 2020

mergify bot commented May 14, 2020

mergify bot commented May 17, 2020

awaelchli commented May 19, 2020 •

edited

Loading

justusschock left a comment

Borda left a comment

awaelchli commented Jun 3, 2020

ZhaofengWu commented Jun 9, 2020

awaelchli commented Jun 23, 2020

ZhaofengWu commented Jun 23, 2020

ZhaofengWu commented Jun 23, 2020

ZhaofengWu commented Jun 24, 2020

awaelchli commented Jun 24, 2020

data transfer model hook (+ refactor) #1756

data transfer model hook (+ refactor) #1756

Conversation

awaelchli commented May 7, 2020 • edited Loading

Before submitting

What does this PR do?

PR review

Did you have fun?

codecov bot commented May 9, 2020 • edited Loading

Codecov Report

mergify bot commented May 9, 2020

mergify bot commented May 14, 2020

mergify bot commented May 17, 2020

awaelchli commented May 19, 2020 • edited Loading

justusschock left a comment

Choose a reason for hiding this comment

Borda left a comment

Choose a reason for hiding this comment

awaelchli commented Jun 3, 2020

ZhaofengWu commented Jun 9, 2020

awaelchli commented Jun 23, 2020

ZhaofengWu commented Jun 23, 2020

ZhaofengWu commented Jun 23, 2020

ZhaofengWu commented Jun 24, 2020

awaelchli commented Jun 24, 2020

awaelchli commented May 7, 2020 •

edited

Loading

codecov bot commented May 9, 2020 •

edited

Loading

awaelchli commented May 19, 2020 •

edited

Loading