Skip to content
This repository was archived by the owner on Oct 9, 2023. It is now read-only.

Replace DP/DDP/DDPSpawn plugins to strategies, keep the old for compatibility #1451

Merged
merged 4 commits into from
Sep 6, 2022

Conversation

krshrimali
Copy link
Contributor

@krshrimali krshrimali commented Sep 5, 2022

What does this PR do?

Error:

File "/opt/hostedtoolcache/Python/3.8.13/x64/lib/python3.8/site-packages/flash/image/classification/adapters.py", line 23, in <module>
    from pytorch_lightning.plugins import DataParallelPlugin, DDPPlugin, DDPSpawnPlugin
ImportError: cannot import name 'DataParallelPlugin' from 'pytorch_lightning.plugins' 

PL 1.6.0 had removed these plugins, in the favor of strategies. This PR makes sure to not break the compatibility for older PL versions, and making sure to support latest PL API.

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?
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests? [not needed for typos/docs]
  • 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

  • Is this pull request ready for review? (if not, please submit in draft mode)

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 🙃

Copy link
Collaborator

@ethanwharris ethanwharris left a comment

Choose a reason for hiding this comment

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

LGTM 😃

@Borda Borda enabled auto-merge (squash) September 5, 2022 19:24
@codecov
Copy link

codecov bot commented Sep 5, 2022

Codecov Report

Merging #1451 (6575f40) into master (da42a63) will decrease coverage by 0.55%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #1451      +/-   ##
==========================================
- Coverage   92.84%   92.29%   -0.56%     
==========================================
  Files         287      287              
  Lines       12975    13061      +86     
==========================================
+ Hits        12046    12054       +8     
- Misses        929     1007      +78     
Flag Coverage Δ
pytest 13.29% <0.00%> (?)
tpu 13.29% <0.00%> (?)
unittests 92.82% <83.33%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
flash/image/classification/adapters.py 82.53% <83.33%> (-1.42%) ⬇️
flash/image/segmentation/cli.py 83.33% <0.00%> (-16.67%) ⬇️
flash/pointcloud/detection/cli.py 83.33% <0.00%> (-16.67%) ⬇️
flash/pointcloud/segmentation/cli.py 83.33% <0.00%> (-16.67%) ⬇️
flash/text/question_answering/cli.py 83.33% <0.00%> (-16.67%) ⬇️
flash/audio/speech_recognition/cli.py 83.33% <0.00%> (-16.67%) ⬇️
flash/text/seq2seq/summarization/cli.py 83.33% <0.00%> (-16.67%) ⬇️
flash/image/detection/cli.py 84.61% <0.00%> (-15.39%) ⬇️
flash/audio/classification/cli.py 84.61% <0.00%> (-15.39%) ⬇️
flash/video/classification/cli.py 84.61% <0.00%> (-15.39%) ⬇️
... and 28 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@krshrimali
Copy link
Contributor Author

The failing test is flaky (and not related to this PR), from an offline discussion with @ethanwharris:

We would just need to use a different random seed where it performs better.

For now, merging this PR. 🔥

@krshrimali krshrimali disabled auto-merge September 6, 2022 06:25
@krshrimali krshrimali merged commit e05c4ce into master Sep 6, 2022
@krshrimali krshrimali deleted the fix/plugins_to_strategies branch September 6, 2022 06:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants