-
Notifications
You must be signed in to change notification settings - Fork 3.6k
1/n Simplify spawn plugins: Simplify handling of multiprocessing queue #10034
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
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
carmocca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
This is super cool!! love it! |
tchaton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Awesome work.
justusschock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just one minor comment :)
| torch.cuda.empty_cache() | ||
|
|
||
|
|
||
| class _FakeQueue(list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik subclassing mutable builtins (especially dict and list) can sometimes lead to unexpected results. Maybe use UserList as base class instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justusschock I don't think that's true nowadays: https://stackoverflow.com/questions/25464647/list-vs-userlist-and-dict-vs-userdict
list is the recommended approach for simple extension
and collections.* for complex extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting Raymond Hettinger (Python core dev):
"FWIW, the UserDict class in Py3.x was relocated to the collections
module. It isn't gone. We made an effort to kill it but found that
there were compelling use cases that were not a cleanly solved by any
other approach."
Codecov Report
@@ Coverage Diff @@
## master #10034 +/- ##
=======================================
Coverage 92% 92%
=======================================
Files 177 177
Lines 16554 16562 +8
=======================================
+ Hits 15192 15204 +12
+ Misses 1362 1358 -4 |
What does this PR do?
Part of #10059, implements the first 3 steps which are highly coupled. Please read steps 1-3 on the issue to better follow the thought process behind this PR. Steps 1-3 are combined in this PR because they are tightly entangled. Steps 4 and 5 will be independent follow-ups.
Notes to reviewer:
add_to_queue/get_from_queuein spawn plugins #10865 if you find the time.Does your PR introduce any breaking changes? If yes, please list them.
A very tiny edge case breaking change in two hooks,
LightningModule.add_to_queueandLightningModule.get_from_queuewhich now accept a_SimpleQueueas input instead of atorch.multiprocessing.SimpleQueue. This is however not expected to make an impact directly, since only the type hint changes while the user interface remains exactly the same.Before submitting
No new tests are required. We have enough (quite expensive) ddp spawn tests that will pass and confirm that these refactors and redesigns retain the existing functionality and performance.
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:
Did you have fun?
I made sure I had fun coding 🙃
Part of #1 (it's a lie, this is just here to avoid noisy GitHub bot)
cc @Borda @justusschock @awaelchli @akihironitta @tchaton @kaushikb11