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 double precision + ddp_spawn #6924

Merged
merged 34 commits into from
Jun 1, 2021
Merged

Fix double precision + ddp_spawn #6924

merged 34 commits into from
Jun 1, 2021

Conversation

ethanwharris
Copy link
Member

@ethanwharris ethanwharris commented Apr 9, 2021 •
edited by carmocca
Loading

What does this PR do?

Fixes #6893 - should also prevent issue from discussion #6851

Basically just changes patching to wrapping, had to add a new base wrapper class and support for unwrapping, but it means that double precision + distributed wrappers works fine and no longer needs a teardown.

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 🙃

@ethanwharris ethanwharris changed the title Bugfix/double precision ddp spawn Fix double precision + ddp_spawn Apr 9, 2021
@codecov
Copy link

codecov bot commented Apr 9, 2021 •
edited
Loading

Codecov Report

Merging #6924 (16e0aed) into master (0b6fd1d) will increase coverage by 4%.
The diff coverage is 98%.

@@           Coverage Diff           @@
##           master   #6924    +/-   ##
=======================================
+ Coverage      88%     92%    +4%     
=======================================
  Files         199     199            
  Lines       12987   13008    +21     
=======================================
+ Hits        11412   12006   +594     
+ Misses       1575    1002   -573

@ethanwharris ethanwharris marked this pull request as ready for review April 9, 2021 15:36
@mergify mergify bot removed the has conflicts label Apr 11, 2021
@ethanwharris ethanwharris added bug Something isn't working priority: 1 Medium priority task labels Apr 11, 2021
@mergify mergify bot removed the has conflicts label May 4, 2021
Borda
Borda previously requested changes May 4, 2021
View reviewed changes
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.

can you reconsider this PR with respect to concurrent #7289?

@ethanwharris
Copy link
Member Author

ethanwharris commented May 4, 2021 •
edited
Loading

@Borda could explain how #7289 is concurrent, that's to do with wrapping DataModule right? The fix here is to do with wrapping LightningModule for plugins

@Borda
Copy link
Member

Borda commented May 4, 2021

@Borda could explain how #7289 is concurrent, that's to do with wrapping DataModule right? The fix here is to do with wrapping LightningModule for plugins

yep, but shall we also rather use the __new__ construct?

@Borda Borda dismissed their stale review May 4, 2021 07:57

discussion

@edenlightning edenlightning removed this from the v1.3 milestone May 4, 2021
@mergify mergify bot added the has conflicts label May 6, 2021
@stale
Copy link

stale bot commented May 30, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/generated/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

@stale stale bot added the won't fix This will not be worked on label May 30, 2021
@stale stale bot removed the won't fix This will not be worked on label Jun 1, 2021
@mergify mergify bot removed the has conflicts label Jun 1, 2021
@ethanwharris
Copy link
Member Author

@Borda I don't think we can use __new__ here is we just want to sometimes wrap the module. __new__ would work if we wanted to always wrap it (e.g. to decorate some of the functions).

@ethanwharris
Copy link
Member Author

@Borda @awaelchli @justusschock I've updated the PR, mind re-reviewing?

tchaton
tchaton approved these changes Jun 1, 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 !

@carmocca carmocca added this to the v1.3.x milestone Jun 1, 2021
awaelchli
awaelchli approved these changes Jun 1, 2021
View reviewed changes
Copy link
Contributor

@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.

Note the test for double precision now takes 70+ seconds because of spawn getting called six times 2 x (fit, test, predict)
This is not ideal especially for a test that does not assert anything.

@ethanwharris
Copy link
Member Author

@awaelchli I've simplified the test so now should only spawn once 😃

@awaelchli awaelchli added the ready PRs ready to be merged label Jun 1, 2021
@ethanwharris ethanwharris enabled auto-merge (squash) June 1, 2021 14:32
@ethanwharris ethanwharris merged commit 03bb389 into master Jun 1, 2021
@ethanwharris ethanwharris deleted the bugfix/double_precision_ddp_spawn branch June 1, 2021 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justusschock justusschock justusschock approved these changes

@tchaton tchaton tchaton approved these changes

@awaelchli awaelchli awaelchli approved these changes

@Borda Borda Borda left review comments

@carmocca carmocca Awaiting requested review from carmocca

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
bug Something isn't working priority: 1 Medium priority task ready PRs ready to be merged
Projects
None yet
Milestone
pl:1.3.x
Development

Successfully merging this pull request may close these issues.

DDP_SPAWN + Double Precision gives pickle error
7 participants
@ethanwharris @awaelchli @Borda @tchaton @justusschock @carmocca @edenlightning