Add transfer_batch_to_device hook to DataModule

[nateraw]

What does this PR do?

If transfer_batch_to_device is defined in datamodule, we'll use it instead of the default logic in trainer.

Fixes # (issue)

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 🙃

[awaelchli]

I still don't understand why we need to duplicate all data hooks from LightningModule into the DataModules. What prevents us from defining a data-interface and inherit in both classes? #2707

[codecov]

Codecov Report

Merging #3038 into master will increase coverage by 1%.
The diff coverage is 83%.

@@           Coverage Diff            @@
##           master   #3038     +/-   ##
========================================
+ Coverage      89%     90%     +1%     
========================================
  Files          81      81             
  Lines        7796   10870   +3074     
========================================
+ Hits         6959    9787   +2828     
- Misses        837    1083    +246     

[nateraw]

I still don't understand why we need to duplicate all data hooks from LightningModule into the DataModules. What prevents us from defining a data-interface and inherit in both classes? #2707

@awaelchli I felt the same thing as I submitted this. At this point (right before major release), I don't want to make major refactor to hooks. After 0.9.0 we can knock this out, because you're right...it seems pointless to copy over.

Does that sound reasonable?

[awaelchli]

I hope we can pick this up immediately after 0.9. I believe we otherwhise risk that these two defintions diverge and at some point we will have bugs because the Trainer does not call the hooks in one class the same way as in the other etc. This of course translates into maintenance effort that should not be.

[nateraw]

Agreed!

[nateraw]

@awaelchli - this test look good? Borrowed logic from you

[awaelchli]

well, given my previous comment I also have to answer no here 🤣 . Can we not parametrize the original test to use the LightningModule class and the DataModule class as inputs to the test?

[nateraw]

well, given my previous comment I also have to answer no here 🤣 . Can we not parametrize the original test to use the LightningModule class and the DataModule class as inputs to the test?

opted to put it separately in datamodule file because I think it makes more sense to live there.

[williamFalcon]

generally we are decoupling the data from the lightningmodule. but keeping the option to keep the data linked.

the last missing key here is the ability for a datamodule to define how it transfers data in case you have really weird formats

[williamFalcon]

@awaelchli we can do that after 0.9 but we need to think about it carefully as well to make sure we don't break functionality or bleed abstractions over.

[awaelchli]

yes of course, don't want to block this. thanks for discussion and considerations 😃 ❤️