-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix dtype/device property not getting updated in submodules #2657
Conversation
Hello @awaelchli! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-07-21 17:32:43 UTC |
Codecov Report
@@ Coverage Diff @@
## master #2657 +/- ##
=======================================
Coverage 91% 91%
=======================================
Files 70 72 +2
Lines 5778 6131 +353
=======================================
+ Hits 5270 5599 +329
- Misses 508 532 +24 |
if dtype is not None: | ||
module._dtype = dtype | ||
|
||
self.apply(apply_fn) |
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.
what is the reason of using apply
?
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.
part of the answer is here in my comment.
apply in contrast to "to" works recursively on all modules and allows us to update our custom properties.
I'm writing a test right now to make sure it fixes what failed before.
@awaelchli can you also test this with dp and ddp_spawn backends? |
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.
Actually I'm kind of clueless why it works now and did not work before, since torch.nn.Module also called apply (e.g. https://github.com/pytorch/pytorch/blob/master/torch/nn/modules/module.py#L462).
But LGTM
@justusschock Yes, I understand. It took me a while to figure it out. The thing is, the super calls the _apply, and if you look inside you will see that it will only dispatch the .cuda, .to, etc. to buffers and parameters! But we need our special properties to be set on the module, so we need to call apply (not _apply). |
What does this PR do?
Fixes #2442
Before submitting
The new tests fail on master and demonstrate the bug.