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

Add an asynchronous single GPU dataloader example #1521

Closed
wants to merge 14 commits into from

Conversation

HenryJia
Copy link
Contributor

@HenryJia HenryJia commented Apr 17, 2020

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?

Address #1454 #1404 #1316

This was the simplest way without adding an any extra complexity to pytorch-lightning itself. It's only possible to asynchronously load for single GPU training anyway. MultiGPU uses PyTorch's DataParallel.scatter() which seems to have synchronisation constraints

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
Copy link

pep8speaks commented Apr 17, 2020

Hello @HenryJia! Thanks for updating this PR.

Line 75:111: E501 line too long (112 > 110 characters)

Comment last updated at 2020-04-29 20:03:05 UTC

@mergify mergify bot requested a review from a team April 17, 2020 20:35
@HenryJia HenryJia changed the title Add an asynchronous single GPU dataloader sexample Add an asynchronous single GPU dataloader example Apr 17, 2020
@Borda Borda added the feature Is an improvement or enhancement label Apr 20, 2020
pl_examples/utils/loaders.py Outdated Show resolved Hide resolved
pl_examples/utils/loaders.py Outdated Show resolved Hide resolved
pl_examples/utils/loaders.py Outdated Show resolved Hide resolved
pl_examples/basic_examples/async_gpu_template.py Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team April 20, 2020 06:26
@Borda Borda added this to the 0.7.4 milestone Apr 20, 2020
@mergify mergify bot requested a review from a team April 20, 2020 07:08
@mergify
Copy link
Contributor

mergify bot commented Apr 20, 2020

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

@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #1521 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1521   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files          71      71           
  Lines        4175    4175           
======================================
  Hits         3692    3692           
  Misses        483     483           

@veritas9872
Copy link

@HenryJia The len attribute seems to be wrong. To the best of my knowledge, DataLoaders do not have len. To access the length, you should use DataLoader.Dataset to access the dataset of the data loader.

@justusschock
Copy link
Member

@veritas9872 That's not correct. The pytorch data loader does have a length that computes the number of samples based on batch size and dataset/sampler length. It's the dataset that does not necessarily have a length.

@mergify mergify bot requested a review from a team April 22, 2020 13:09
@mergify
Copy link
Contributor

mergify bot commented Apr 22, 2020

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

Copy link
Member

@justusschock justusschock left a comment

Choose a reason for hiding this comment

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

Looks good to me now :)

I think as an example this is good now. However, there are some issues, with DDP, where we automatically alter the sampler when necessary.
I'd say to merge this now, since currently your loader code is only in examples, but maybe add a new PR that add's the code to the framework and also takes care of these changes?

Thoughts @Borda @williamFalcon @PyTorchLightning/core-contributors

@mergify mergify bot requested a review from a team April 24, 2020 07:38
from pl_examples.models.lightning_template import LightningTemplateModel
from pl_examples.utils.loaders import AsynchronousLoader

SEED = 2334
Copy link
Member

Choose a reason for hiding this comment

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

this shall be fixed in #1572

Copy link
Member

@awaelchli awaelchli Apr 28, 2020

Choose a reason for hiding this comment

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

Is the seed actually needed? The example does not actually produce any meaningful results, it's just a demo for a dataloader, right? Otherwise I would wait until #1572 is merged.

@mergify mergify bot requested a review from a team April 24, 2020 08:39
@Borda Borda added example ready PRs ready to be merged labels Apr 24, 2020
@HenryJia
Copy link
Contributor Author

@justusschock I thought about adding it in to lightning itself instead of just as an example, but I'm not sure where exactly to stick it. It's not very generalisable as well as it's only single GPU compatible. To get it to work with infinite dataloaders would add a bit of complexity as I'd need to add in thread stopping conditions. I figured sticking with the PyTorch/PyTorch lightning philosophy of modularity, I'd keep it as an wrapper example with dataloader

@Borda Borda modified the milestones: 0.7.4, 0.7.5 Apr 26, 2020
@mergify
Copy link
Contributor

mergify bot commented Apr 26, 2020

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

@Borda Borda requested a review from awaelchli April 26, 2020 22:30
Copy link
Member

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

Looks good, I tried it myself and it worked fine.
The template filename could be renamed to gpu_async_template just for better lexicograhic ordering in the folder.
And maybe the dataloader could be moved to a folder "dataloaders" just like the models that are located in "models" folder.
There is a merge conflict, could you resolve it?

pl_examples/utils/loaders.py Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team April 28, 2020 22:29
@williamFalcon
Copy link
Contributor

@HenryJia this is awesome, but i think this belongs in bolts?
@PyTorchLightning/core-contributors thoughts?

@awaelchli
Copy link
Member

Could be a good place for these kind of examples

@awaelchli
Copy link
Member

maybe could go to bolts.dataloaders?

@HenryJia
Copy link
Contributor Author

HenryJia commented Apr 30, 2020

Following some discussions with William, we agreed that this would be best added to the datamodules or dataloader section of bolts instead. As such, I'm closing this PR for now unless anyone else has something to object about it. I will open a new PR with this for bolts soon once I get around my final year university exams.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
example feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants