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.

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

Merged
merged 16 commits into from
Jul 21, 2020

Conversation

awaelchli
Copy link
Member

@awaelchli awaelchli commented Jul 21, 2020

What does this PR do?

Fixes #2442

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? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • 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?

The new tests fail on master and demonstrate the bug.

@awaelchli awaelchli added the bug Something isn't working label Jul 21, 2020
@pep8speaks
Copy link

pep8speaks commented Jul 21, 2020

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

@mergify mergify bot requested a review from a team July 21, 2020 14:22
@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #2657 into master will increase coverage by 0%.
The diff coverage is 100%.

@@           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)
Copy link
Member

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?

Copy link
Member Author

@awaelchli awaelchli Jul 21, 2020

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.

@mergify mergify bot requested a review from a team July 21, 2020 15:12
@williamFalcon
Copy link
Contributor

@awaelchli can you also test this with dp and ddp_spawn backends?
make sure to use two different functions and not a parametrized test or it won’t work

@awaelchli awaelchli marked this pull request as ready for review July 21, 2020 17:33
Copy link
Member

@justusschock justusschock left a 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

@mergify mergify bot requested a review from a team July 21, 2020 17:55
@awaelchli
Copy link
Member Author

awaelchli commented Jul 21, 2020

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

@williamFalcon williamFalcon merged commit a5538af into master Jul 21, 2020
@Borda Borda deleted the bugfix/recursive-device branch July 21, 2020 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

validation_epoch_end needs to return CUDA tensors
5 participants