Avoid wrapping LightningModule in DDP plugins when not fitting by four4fish · Pull Request #9096 · Lightning-AI/pytorch-lightning

four4fish · 2021-08-24T21:43:56Z

What does this PR do?

Pull changes from #8632

Does your PR introduce any breaking changes? If yes, please list them.

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 list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
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 🙃

ananthsub

Thanks for carrying this forward!

codecov · 2021-08-25T21:53:09Z

Codecov Report

Merging #9096 (ca6ed38) into master (ff7305f) will decrease coverage by 4%.
The diff coverage is 67%.

@@           Coverage Diff           @@
##           master   #9096    +/-   ##
=======================================
- Coverage      92%     88%    -4%     
=======================================
  Files         176     176            
  Lines       14810   14817     +7     
=======================================
- Hits        13663   13050   -613     
- Misses       1147    1767   +620

awaelchli · 2021-08-25T22:08:08Z

@four4fish isn't this a subset of changes in #8632?

four4fish · 2021-08-25T22:16:30Z

@four4fish isn't this a subset of changes in #8632?

@awaelchli no, I have commandeered the #8632, as Ning is focus on data loader changes ~~

awaelchli · 2021-08-25T22:21:35Z

@four4fish are we closing #8632 then? the contents are virtually identical. at least we should mark one of them as draft

four4fish · 2021-08-25T22:26:03Z

@four4fish are we closing #8632 then? the contents are virtually identical. at least we should mark one of them as draft

@awaelchli Yeah! Will close #8632. Sorry I should have marked it as commandeer PR, thanks :)!

ananthsub

i don't think we need to pass wrap_model into configure_ddp purely for the debug message. i'd rather it be that configure_ddp only wraps the module. duplicating the debug-line while keeping the configure_ddp interace simple doesn't seem like a bad option to me here

tchaton

LGTM !

ananthsub · 2021-09-02T00:21:38Z

@four4fish - the tests are failing because the DeepSpeed plugin extends the DDP plugin. However, for DeepSpeed, we should always wrap the model, and not override the training/validation/test/predict steps (ie. DeepSpeed should continue calling self.model(*args, **args)

This is another motivation for not relying on inheritance between these plugins, as both the wrapping and checkpoint behavior differ.

carmocca · 2021-09-02T03:33:36Z

 - Fixed `DDP` "CUDA error: initialization error" due to a `copy` instead of `deepcopy` on `ResultCollection` ([#9239](https://github.com/PyTorchLightning/pytorch-lightning/pull/9239))


+- Fixed wrapping issue: avoid wrapping LightningModule with data-parallel modules when not fitting in `DDPPlugin`, `DDPSpawnPlugin`, `DDPShardedPlugin`, `DDPSpawnShardedPlugin` ([#9096]https://github.com/PyTorchLightning/pytorch-lightning/pull/9096)


The link is broken

Can be fixed with another PR - no need to open one just for this

four4fish requested review from Borda, SeanNaren, awaelchli, carmocca, justusschock, kaushikb11, tchaton and williamFalcon as code owners August 24, 2021 21:43

four4fish mentioned this pull request Aug 24, 2021

Remove reference to DistributedDataParallel from parallel plugin teardown #8943

Merged

12 tasks

ananthsub approved these changes Aug 24, 2021

View reviewed changes

Comment thread pytorch_lightning/plugins/training_type/ddp.py Outdated

Comment thread pytorch_lightning/plugins/training_type/ddp_spawn.py

Comment thread tests/plugins/test_ddp_plugin.py

ananthsub added distributed Generic distributed-related topic feature Is an improvement or enhancement labels Aug 24, 2021

ananthsub added this to the v1.5 milestone Aug 24, 2021

ananthsub reviewed Aug 25, 2021

View reviewed changes

Comment thread CHANGELOG.md Outdated

tchaton reviewed Aug 25, 2021

View reviewed changes

Comment thread pytorch_lightning/plugins/training_type/ddp.py

Comment thread pytorch_lightning/plugins/training_type/ddp_spawn.py

awaelchli changed the title ~~Avoid wrapping LightningModule in DDP plugins when not fitting~~ Avoid wrapping LightningModule in DDP plugins when not fitting [1/2] Aug 25, 2021

four4fish changed the title ~~Avoid wrapping LightningModule in DDP plugins when not fitting [1/2]~~ Avoid wrapping LightningModule in DDP plugins when not fitting Aug 25, 2021

ananthsub reviewed Aug 25, 2021

View reviewed changes

Comment thread tests/plugins/test_sharded_plugin.py Outdated

four4fish force-pushed the feat/6977 branch 2 times, most recently from 870790e to 1a2245a Compare August 25, 2021 21:28

awaelchli approved these changes Aug 25, 2021

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread tests/plugins/test_ddp_plugin.py Outdated

mergify Bot added the ready to be merged PRs ready to be merged label Aug 25, 2021

ananthsub mentioned this pull request Aug 25, 2021

Avoid wrapping LightningModule in *DataParallel overrides when not fitting #8632

Closed

12 tasks

carmocca reviewed Aug 26, 2021

View reviewed changes

Comment thread pytorch_lightning/plugins/training_type/ddp.py Outdated

ananthsub reviewed Aug 26, 2021

View reviewed changes

tchaton approved these changes Aug 27, 2021

View reviewed changes

Comment thread CHANGELOG.md Outdated

mergify Bot added the has conflicts label Aug 27, 2021

four4fish force-pushed the feat/6977 branch from 4a7c7a4 to 8788924 Compare August 27, 2021 20:57

mergify Bot added has conflicts and removed has conflicts labels Aug 27, 2021

ananthsub reviewed Aug 28, 2021

View reviewed changes

Comment thread pytorch_lightning/trainer/connectors/logger_connector/result.py

four4fish force-pushed the feat/6977 branch from 8788924 to 5525986 Compare August 28, 2021 22:18

mergify Bot added has conflicts and removed has conflicts labels Aug 28, 2021

Borda approved these changes Aug 31, 2021

View reviewed changes

mergify Bot removed the has conflicts label Aug 31, 2021

ananthsub reviewed Sep 1, 2021

View reviewed changes

Comment thread pytorch_lightning/plugins/training_type/ddp_spawn.py

ananthsub enabled auto-merge (squash) September 1, 2021 02:44

four4fish mentioned this pull request Sep 1, 2021

Avoid configuring SyncBatchNorm when not fitting #9243

Closed

auto-merge was automatically disabled September 1, 2021 02:55
Head branch was pushed to by a user without write access

four4fish force-pushed the feat/6977 branch from e79aafa to 17eee5e Compare September 1, 2021 02:55

ananthsub enabled auto-merge (squash) September 1, 2021 03:26

ananthsub reviewed Sep 1, 2021

View reviewed changes

Comment thread CHANGELOG.md Outdated

ananthsub reviewed Sep 1, 2021

View reviewed changes

Comment thread tests/plugins/test_sharded_plugin.py Outdated

four4fish mentioned this pull request Sep 2, 2021

Reconciliate processes masks errors in DDP plugin if _sync_dir isn't initialized #9263

Closed

ananthsub reviewed Sep 2, 2021

View reviewed changes

Comment thread tests/plugins/test_sharded_plugin.py Outdated

Avoid wrapping LightningModule in DDP plugins when not fitting

91f5f0d

auto-merge was automatically disabled September 2, 2021 01:06
Head branch was pushed to by a user without write access

four4fish force-pushed the feat/6977 branch from 737223e to 91f5f0d Compare September 2, 2021 01:06

Avoid wrapping LightningModule in DDP plugins when not fitting

ca6ed38

ananthsub enabled auto-merge (squash) September 2, 2021 01:13

ananthsub merged commit a451997 into Lightning-AI:master Sep 2, 2021

carmocca reviewed Sep 2, 2021

View reviewed changes

awaelchli mentioned this pull request Oct 29, 2021

Outputs from test_step() incorrectly gathered & converted to tensors (DataParallel) #10219

Closed

		- Fixed `DDP` "CUDA error: initialization error" due to a `copy` instead of `deepcopy` on `ResultCollection` ([#9239](https://github.com/PyTorchLightning/pytorch-lightning/pull/9239))


		- Fixed wrapping issue: avoid wrapping LightningModule with data-parallel modules when not fitting in `DDPPlugin`, `DDPSpawnPlugin`, `DDPShardedPlugin`, `DDPSpawnShardedPlugin` ([#9096]https://github.com/PyTorchLightning/pytorch-lightning/pull/9096)

Conversation

four4fish commented Aug 24, 2021 • edited by ananthsub Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Did you have fun?

Uh oh!

ananthsub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

awaelchli commented Aug 25, 2021

Uh oh!

four4fish commented Aug 25, 2021

Uh oh!

Uh oh!

Uh oh!

awaelchli commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

four4fish commented Aug 25, 2021

Uh oh!

Uh oh!

ananthsub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tchaton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ananthsub commented Sep 2, 2021

Uh oh!

Uh oh!

carmocca Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

four4fish commented Aug 24, 2021 •

edited by ananthsub

Loading

codecov Bot commented Aug 25, 2021 •

edited

Loading

awaelchli commented Aug 25, 2021 •

edited

Loading