Run __setstate__ when cloning modules #45858
Conversation
When cloning a module that has __setstate__, __getstate__ methods. We need to load these methods to initialize these modules. [ghstack-poisoned]
When cloning a module that has __setstate__, __getstate__ methods. We need to load these methods to initialize these modules. ghstack-source-id: 6311fc9279524e7e0b39fb5939e0294cde26031f Pull Request resolved: #45858
facebook-github-bot
added
the
oncall: jit
|
@bzinodev can you also make the same change in the quantization code? We have a clone implementation here https://github.com/pytorch/pytorch/blob/master/torch/csrc/jit/passes/quantization/insert_observers.cpp#L96 |
There was a problem hiding this comment.
Looks good to me, but I would like another set of eyes from something with more knowledge of setstate / getstate - cc @suo, @jamesr66a
💊 CI failures summary and remediationsAs of commit f74a8ff (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 5 times. |
When cloning a module that has __setstate__, __getstate__ methods. We need to load these methods to initialize these modules. Differential Revision: [D24116524](https://our.internmc.facebook.com/intern/diff/D24116524) [ghstack-poisoned]
When cloning a module that has __setstate__, __getstate__ methods. We need to load these methods to initialize these modules. Differential Revision: [D24116524](https://our.internmc.facebook.com/intern/diff/D24116524) [ghstack-poisoned]
When cloning a module that has __setstate__, __getstate__ methods. We need to load these methods to initialize these modules. ghstack-source-id: efe8c81a9d1e46b0b8ebe03c2cfba31eab1680eb Pull Request resolved: #45858
Codecov Report
@@ Coverage Diff @@
## gh/bzinodev/19/base #45858 +/- ##
====================================================
Coverage 68.25% 68.26%
====================================================
Files 410 410
Lines 53608 53608
====================================================
+ Hits 36592 36593 +1
+ Misses 17016 17015 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This looks fine. If getstate is defined it should be called on a deepcopy. It appears that clone is a deepcopy.
Some followups that could be good (doesn't have to be in this PR, just recording):
- deepcopy should do the same thing if it doesn't already.
- Can we please rename
cloneto something actually meaningful? I had to read the implementation to see if it was a copy vs. deepcopy, since literally just says "clone" in the method documentation. Maybe, deepcopy_with_type cc @jerry39213gh
Stack from ghstack:
When cloning a module that has setstate, getstate methods.
We need to load these methods to initialize these modules.
Differential Revision: D24116524