Skip to content
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.

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

Support torchtext on a single GPU #2379

Merged
merged 5 commits into from
Jun 27, 2020
Merged

Support torchtext on a single GPU #2379

merged 5 commits into from
Jun 27, 2020

Conversation

elkotito
Copy link
Contributor

@elkotito elkotito commented Jun 26, 2020

What does this PR do?

Allows using torchtext on a single GPU e.g. via torch.text.BucketIterator object which in turn generate torchtext.data.Batch objects. It seems like it works only on a single GPU due to #2350.

Fixes #1245

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 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 🙃

@mergify mergify bot requested a review from a team June 26, 2020 20:42
@awaelchli
Copy link
Contributor

@mateuszpieniak I wonder why torchtext.Batch doesn't have a .to() method. Wouldn't that be a logical choice? It seems so obvious. It would make the support much easier.

@Borda Borda added the feature Is an improvement or enhancement label Jun 26, 2020
@Borda Borda added this to the 0.8.x milestone Jun 26, 2020
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot requested a review from a team June 26, 2020 22:33
@mergify mergify bot requested a review from a team June 26, 2020 22:40
@williamFalcon
Copy link
Contributor

awesome! let's finalize the changes requested so we can merge :)
Great PR

@elkotito
Copy link
Contributor Author

elkotito commented Jun 27, 2020

  1. I need some help with the docs. I updated the docs with Batch class info (code/hooks.py), but for some reason, it is grayed out. Is there anything else I have to do to make it work?
    The data types listed below (and any arbitrary nesting of them) are supported out of the box:

    - :class:`torch.Tensor` or anything that implements `.to(...)`
    - :class:`list`
    - :class:`dict`
    - :class:`tuple`
    - :class:`torchtext.data.batch.Batch`
  1. Is the requirements/extra.txt correct to put torchtext dependency?

@awaelchli awaelchli self-assigned this Jun 27, 2020
@williamFalcon williamFalcon merged commit e82d9cd into Lightning-AI:master Jun 27, 2020
@@ -7,3 +7,4 @@ tensorboard>=1.14
future>=0.17.1 # required for builtins in setup.py
# pyyaml>=3.13
PyYAML>=5.1 # OmegaConf requirement
torchtext>=0.3.1
Copy link
Member

Choose a reason for hiding this comment

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

Torchtext as mandatory dependency? When we refused torchvision.... @williamFalcon

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, should go to extras. I was not fast enough to properly review :(

Copy link
Member

@Borda Borda Jun 27, 2020

Choose a reason for hiding this comment

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

fixing in #2395

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, didn't catch that. i thought it was in extras

@mergify mergify bot requested a review from a team June 27, 2020 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve moving data / model to GPU using torchtext
4 participants