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.

Sign up for GitHub

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

Jump to bottom

Fix DeepSpeedPlugin with IterableDataset #7362

Merged
merged 11 commits into from
May 7, 2021
Merged

Fix DeepSpeedPlugin with IterableDataset #7362

merged 11 commits into from
May 7, 2021

Conversation

leezu
Copy link
Contributor

@leezu leezu commented May 4, 2021 •
edited by SeanNaren
Loading

Fixes #7345

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • 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?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

@codecov
Copy link

codecov bot commented May 4, 2021 •
edited
Loading

Codecov Report

Merging #7362 (e91c4be) into master (28103c6) will decrease coverage by 5%.
The diff coverage is 10%.

@@           Coverage Diff           @@
##           master   #7362    +/-   ##
=======================================
- Coverage      92%     87%    -5%     
=======================================
  Files         200     200            
  Lines       12983   12992     +9     
=======================================
- Hits        11937   11359   -578     
- Misses       1046    1633   +587

@Borda Borda added the bug Something isn't working label May 4, 2021
@Borda Borda added this to the v1.3 milestone May 4, 2021
@SeanNaren
Copy link
Contributor

Thanks @leezu! Let me know your thoughts on the proposed change, and I can help make a test for this to get this merged.

@SeanNaren
Copy link
Contributor

I added a test, and modified the code to now use auto. Auto will try to use the batch sampler if it exists else will default to 1.

SeanNaren
SeanNaren reviewed May 6, 2021
View reviewed changes
pytorch_lightning/plugins/training_type/deepspeed.py
Comment on lines +432 to +435
if hasattr(self.lightning_module, 'train_dataloader'):
train_dataloader = self.lightning_module.train_dataloader()
if hasattr(train_dataloader, 'batch_sampler'):
batch_size = train_dataloader.batch_sampler.batch_size
Copy link
Contributor

Choose a reason for hiding this comment

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

If anyone can suggest anything cleaner please do :)

Copy link
Member

Choose a reason for hiding this comment

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

probably not much as train_dataloader() is callable not just an attribute...

Copy link
Contributor

Choose a reason for hiding this comment

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

This could break if the user provides several dataloaders to the CombinedLoader.

Copy link
Member

Choose a reason for hiding this comment

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

@leezu ^^

@Borda
Copy link
Member

Borda commented May 6, 2021

@leezu pls use and follow the bullet list from template

tchaton
tchaton approved these changes May 6, 2021
View reviewed changes
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGTM !

pytorch_lightning/plugins/training_type/deepspeed.py
Comment on lines +432 to +435
if hasattr(self.lightning_module, 'train_dataloader'):
train_dataloader = self.lightning_module.train_dataloader()
if hasattr(train_dataloader, 'batch_sampler'):
batch_size = train_dataloader.batch_sampler.batch_size
Copy link
Contributor

Choose a reason for hiding this comment

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

This could break if the user provides several dataloaders to the CombinedLoader.

@leezu
Copy link
Contributor Author

leezu commented May 6, 2021

Thank you @SeanNaren. LGTM

@leezu leezu mentioned this pull request May 6, 2021
Merged
11 tasks
@Borda Borda removed this from the v1.3 milestone May 6, 2021
@Borda Borda added this to the v1.3.x milestone May 6, 2021
@SeanNaren SeanNaren merged commit 98b94b8 into Lightning-AI:master May 7, 2021
@SeanNaren
Copy link
Contributor

Thanks @leezu :)

@leezu leezu deleted the deepspeed branch May 7, 2021 14:24
@carmocca carmocca mentioned this pull request May 10, 2021
Closed
Borda pushed a commit that referenced this pull request May 11, 2021
@leezu @Borda
Fix DeepSpeedPlugin with IterableDataset (#7362)
5888abd 
* deepspeed add train_micro_batch_size_per_gpu argument

* Update naming and doc

* Modify to use auto naming convention, add test

* Add iterable tests

* Fix tests, attempt by mocking

* Import correct package

* Fix comparison

* Set as special test

* Remove import

* Add Changelog

Co-authored-by: SeanNaren <[email protected]>

(cherry picked from commit 98b94b8)
Borda added a commit that referenced this pull request May 11, 2021
@Borda
Fix DeepSpeedPlugin with IterableDataset (#7362)
7fa6e51 
* deepspeed add train_micro_batch_size_per_gpu argument

* Update naming and doc

* Modify to use auto naming convention, add test

* Add iterable tests

* Fix tests, attempt by mocking

* Import correct package

* Fix comparison

* Set as special test

* Remove import

* Add Changelog

Co-authored-by: SeanNaren <[email protected]>

(cherry picked from commit 98b94b8)
Borda pushed a commit that referenced this pull request May 11, 2021
@leezu @Borda
Fix DeepSpeedPlugin with IterableDataset (#7362)
aa6f711 
* deepspeed add train_micro_batch_size_per_gpu argument

* Update naming and doc

* Modify to use auto naming convention, add test

* Add iterable tests

* Fix tests, attempt by mocking

* Import correct package

* Fix comparison

* Set as special test

* Remove import

* Add Changelog

Co-authored-by: SeanNaren <[email protected]>

(cherry picked from commit 98b94b8)
Borda pushed a commit that referenced this pull request May 11, 2021
@leezu @Borda
Fix DeepSpeedPlugin with IterableDataset (#7362)
a4d676d 
* deepspeed add train_micro_batch_size_per_gpu argument

* Update naming and doc

* Modify to use auto naming convention, add test

* Add iterable tests

* Fix tests, attempt by mocking

* Import correct package

* Fix comparison

* Set as special test

* Remove import

* Add Changelog

Co-authored-by: SeanNaren <[email protected]>

(cherry picked from commit 98b94b8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaushikb11 kaushikb11 kaushikb11 left review comments

@SeanNaren SeanNaren SeanNaren approved these changes

@tchaton tchaton tchaton approved these changes

@ananthsub ananthsub ananthsub approved these changes

@awaelchli awaelchli Awaiting requested review from awaelchli

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@Borda Borda Awaiting requested review from Borda Borda is a code owner

@carmocca carmocca Awaiting requested review from carmocca

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
pl:1.3.x
Development

Successfully merging this pull request may close these issues.

DeepSpeedPlugin only works for Datasets with BatchSampler not IterableDataset
6 participants
@leezu @SeanNaren @Borda @ananthsub @tchaton @kaushikb11