Bugfix/torchtext include lengths

[thschaaf]

What does this PR do?

It fixes a bug when using torchtex and torchtext.data.Field with include_lengths=True that arises when transferring data to GPU.
It adds tests to check if Batches created by torchtext with include_lengths=True and include_lengths=False are processed by the Trainer.fit().

The fix checks if the data is a Tensor, tuple, or list before sending it to the device. If it is a tuple, or list, it iterates over the elements and sends them to the device. (Implementation changed and it uses now move_data_to_device recursively.)

Fixes #2688

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 @thschaaf! Thanks for updating this PR.

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

Comment last updated at 2020-07-30 01:53:43 UTC

[awaelchli]

@thschaaf Thanks for the PR. How much value do you see in this support? I was told by torchtext member that they will drop the Batch class from torchtext moving forward.
pytorch/text#861
We recently added the support for Batch class in PL because of a requrest but at that time I was not aware of its legacy state.
What do you think about this?

[thschaaf]

@thschaaf Thanks for the PR. How much value do you see in this support? I was told by torchtext member that they will drop the Batch class from torchtext moving forward.
pytorch/text#861
We recently added the support for Batch class in PL because of a requrest but at that time I was not aware of its legacy state.
What do you think about this?

@awaelchli Hopefully when torchtext removes the Batch class they do it without breaking too much code from people (in a substantial way). There is enough value to support this until torchtext actually does change their implementation. In my case the Batch object of torchtext is used behind the curtain, and it caused my Skip-Thought model training to fail on GPUs. I am sure others might run into similar issues, and was quite happy that the change to Pytorch-Lightning is quite compact. With this change RNN training works on a GPU.

What do you think about the added tests? They are agnostic to the underlying Batch class, only making sure that the the torchtext.data.Field paramter include_length=True is tested. They might be useful in the future, even after the code dealing with the torchtext Batch class is removed.

The change torchtext is planing seems sensible, but more people might use Pytorch-Lightning together with torchtext earlier. It is not intuitive if a model training runs on the CPU but throws such an exception on GPU.

Of course having the PR changes become part of PL would make my Skip-Thought model train faster and my life easier.

[thschaaf]

ci/circleci: TPU-tests is constantly failing with ERROR: (gcloud.auth.activate-service-account) Could not read json file /home/circleci/gcloud-service-key.json: No JSON object could be decoded.
This seems unrelated to code changes of PR. How can that be fixed?

the issue is that somehow it is running in your CircleCI env which is missing GKE credentials...

@Borda Thanks! For some reasons, which I don't remember exactly, I followed the pytorch-lightning project. Just to unfollow the project did the trick.

[awaelchli]

looks good, thanks for the fix @thschaaf

[mergify]

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