[fx] GraphModule copy top level attributes from root #45182
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Previously only the attributes that's used in the graph is copied in the constructor of GraphModule, this PR adds the support for copying all attributes by overriding `__dict__` Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
Summary: Previously only the attributes that's used in the graph is copied in the constructor of GraphModule, this PR adds the support for copying all attributes by overriding `__dict__` Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
jerryzh168
added a commit
that referenced
this pull request
Sep 23, 2020
Summary: Previously only the attributes that's used in the graph is copied in the constructor of GraphModule, this PR adds the support for copying all attributes by overriding `__dict__` Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: c9a0c7f570b44e99a83e46ccdc5e90139966ad70 Pull Request resolved: #45182
💊 CI failures summary and remediationsAs of commit cd94987 (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 16 times. |
jamesr66a
reviewed
Sep 23, 2020
Summary: Previously only the attributes that's used in the graph is copied in the constructor of GraphModule, this PR adds the support for copying all attributes by overriding `__dict__` Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23858632](https://our.internmc.facebook.com/intern/diff/D23858632) [ghstack-poisoned]
jerryzh168
added a commit
that referenced
this pull request
Sep 23, 2020
Summary: Previously only the attributes that's used in the graph is copied in the constructor of GraphModule, this PR adds the support for copying all attributes by overriding `__dict__` Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 3466ebdd4cafbca5ed305618307d6fb09b36fd89 Pull Request resolved: #45182
Codecov Report
@@ Coverage Diff @@
## gh/jerryzh168/439/base #45182 +/- ##
=======================================================
Coverage 68.10% 68.11%
=======================================================
Files 393 393
Lines 50945 50953 +8
=======================================================
+ Hits 34698 34707 +9
+ Misses 16247 16246 -1
Continue to review full report at Codecov.
|
jamesr66a
reviewed
Sep 24, 2020
Summary: Previously only the attributes that's used in the graph is copied in the constructor of GraphModule, this PR adds the support for copying all attributes by overriding `__dict__` Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23858632](https://our.internmc.facebook.com/intern/diff/D23858632) [ghstack-poisoned]
jamesr66a
reviewed
Sep 24, 2020
Summary: Previously only the attributes that's used in the graph is copied in the constructor of GraphModule, this PR adds the support for copying all attributes by overriding `__dict__` Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23858632](https://our.internmc.facebook.com/intern/diff/D23858632) [ghstack-poisoned]
Summary: Previously only the attributes that's used in the graph is copied in the constructor of GraphModule, this PR adds the support for copying all attributes by overriding `__dict__` Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23858632](https://our.internmc.facebook.com/intern/diff/D23858632) [ghstack-poisoned]
jerryzh168
added a commit
that referenced
this pull request
Sep 24, 2020
Summary: Previously only the attributes that's used in the graph is copied in the constructor of GraphModule, this PR adds the support for copying all attributes by overriding `__dict__` Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: df0b0b99545320553100d787c4b81f053565b291 Pull Request resolved: #45182
zdevito
reviewed
Sep 28, 2020
| return GraphModule(fake_mod, self.graph) | ||
| graph_module = GraphModule(fake_mod, self.graph) | ||
| # skip overwriting generated attributes | ||
| for attr in ['code', '_graph', '_modules']: |
There was a problem hiding this comment.
These fields are not complete. For instance _parameters and _buffers are not included and copying them might break the nn.Module similar to _modules. Any attempt to list parameters here would be very fragile because changes to nn.Module would affect what this list needs to be with no reliable way to inform an editor of nn.Module that this list also needs to be modified.
There was a problem hiding this comment.
actually I'm not sure why I included _modules we probably don't need that. This is just for skipping attributes generated when creating a GraphModule.
|
fixed in the original PR: #44074 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
Summary:
Previously only the attributes that's used in the graph is copied in the constructor
of GraphModule, this PR adds the support for copying all attributes by overriding
__dict__Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
Differential Revision: D23858632