[package] mangle imported module names #50049
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
Rationale and implementation immortalized in a big comment in _mangling.py [ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit af3fa71 (more details on the Dr. CI page):
ci.pytorch.org: 1 failedThis 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 to the (internal) Dr. CI Users group. |
Rationale and implementation immortalized in a big comment in _mangling.py [ghstack-poisoned]
suo
added a commit
that referenced
this pull request
Jan 4, 2021
Rationale and implementation immortalized in a big comment in _mangling.py ghstack-source-id: e7131e1ebe4c87f7a036db6541ed2f8d40de55fa Pull Request resolved: #50049
zdevito
reviewed
Jan 5, 2021
There was a problem hiding this comment.
I'd suggest taking the approach of trying to unmangle and mangle the names at the earliest possible point in the process of handling them. Several cases (like trying to resolve a package for __torch_package_0) are probably happening because the mangled prefix hasn't be removed before loading something. The only place mangled names should exist is in the name field of a module and the module field of classes. When those get loaded for use in our package infrastructure they should the be removed right away. Error reporting could further look into what the mangled name was to described what went wrong.
Rationale and implementation immortalized in a big comment in _mangling.py Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
suo
added a commit
that referenced
this pull request
Jan 5, 2021
Rationale and implementation immortalized in a big comment in _mangling.py ghstack-source-id: 18f6cd3ed80da0f5f74295e4cd5930779309e863 Pull Request resolved: #50049
Rationale and implementation immortalized in a big comment in _mangling.py Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
Rationale and implementation immortalized in a big comment in _mangling.py Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
suo
added a commit
that referenced
this pull request
Jan 8, 2021
Rationale and implementation immortalized in a big comment in _mangling.py ghstack-source-id: e91e29bf1c1d5b0d37e03168b80b6a2a9ff8f7a8 Pull Request resolved: #50049
Rationale and implementation immortalized in a big comment in _mangling.py Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
suo
added a commit
that referenced
this pull request
Jan 10, 2021
Rationale and implementation immortalized in a big comment in _mangling.py ghstack-source-id: 4d44af17adb3fadd34a50b5fcf34d13e267c5ed8 Pull Request resolved: #50049
Rationale and implementation immortalized in a big comment in _mangling.py Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
suo
added a commit
that referenced
this pull request
Jan 11, 2021
Rationale and implementation immortalized in a big comment in `torch/package/mangling.md`. This change also allows imported modules to be TorchScripted ghstack-source-id: 051e34b6d2d1d14f54ff98683c58f532adc5764a Pull Request resolved: #50049
Rationale and implementation immortalized in a big comment in `torch/package/mangling.md`. This change also allows imported modules to be TorchScripted Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
Rationale and implementation immortalized in a big comment in `torch/package/mangling.md`. This change also allows imported modules to be TorchScripted Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
suo
added a commit
that referenced
this pull request
Jan 11, 2021
Rationale and implementation immortalized in a big comment in `torch/package/mangling.md`. This change also allows imported modules to be TorchScripted ghstack-source-id: 291d7304ed06574836c8c7ed042a2b55a808ef0c Pull Request resolved: #50049
zdevito
reviewed
Jan 11, 2021
torch/package/mangling.md
Outdated
| - `__module__` | ||
| - `__file__` | ||
| 2. No mangled names should be serialized by `PackageExporter`. | ||
| 3. Every entry point to `PackageImporter` and `PackageExporter` must |
There was a problem hiding this comment.
I feel like this rule should be that you must pass demanded names to the package importer and exporter. The surface area of the importer/exporter API is to large to try to maintain this invariant.
torch/package/exporter.py
Outdated
| @@ -198,6 +228,7 @@ def _get_source_of_module(self, module: types.ModuleType) -> str: | |||
| return ''.join(result) | |||
|
|
|||
| def require_module_if_not_provided(self, module_name: str, dependencies=True): | |||
| module_name = self._demangle(module_name) | |||
There was a problem hiding this comment.
I feel like this demangling is coming too late. The only mangled properties are the __name__ fields of modules. I feel like the only safe place to put a demangle call is directly in the same function that looks up __name__ from the module. Otherwise, you get this big separation between when a mangled name is extract and when it is used. The only place we should want mangled names is in the __name__ field so that Python infra doesn't get confused about module lookup.
Rationale and implementation immortalized in a big comment in `torch/package/mangling.md`. This change also allows imported modules to be TorchScripted Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
Rationale and implementation immortalized in a big comment in `torch/package/mangling.md`. This change also allows imported modules to be TorchScripted Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
Rationale and implementation immortalized in a big comment in `torch/package/mangling.md`. This change also allows imported modules to be TorchScripted Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
Rationale and implementation immortalized in a big comment in `torch/package/mangling.md`. This change also allows imported modules to be TorchScripted Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
suo
added a commit
that referenced
this pull request
Jan 12, 2021
Rationale and implementation immortalized in a big comment in `torch/package/mangling.md`. This change also allows imported modules to be TorchScripted ghstack-source-id: ed64eaff1ba947d27e7cb79fad7818699c2ac46e Pull Request resolved: #50049
zdevito
approved these changes
Jan 13, 2021
Codecov Report
@@ Coverage Diff @@
## gh/suo/380/base #50049 +/- ##
===================================================
- Coverage 80.73% 80.72% -0.01%
===================================================
Files 1910 1910
Lines 207182 207101 -81
===================================================
- Hits 167267 167187 -80
+ Misses 39915 39914 -1 |
Rationale and implementation immortalized in a big comment in `torch/package/mangling.md`. This change also allows imported modules to be TorchScripted Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
Rationale and implementation immortalized in a big comment in `torch/package/mangling.md`. This change also allows imported modules to be TorchScripted Differential Revision: [D25758625](https://our.internmc.facebook.com/intern/diff/D25758625) [ghstack-poisoned]
suo
added a commit
that referenced
this pull request
Jan 13, 2021
Rationale and implementation immortalized in a big comment in `torch/package/mangling.md`. This change also allows imported modules to be TorchScripted ghstack-source-id: 34c5650d71eda7155a33142f34558efc6b9ba901 Pull Request resolved: #50049
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:
Rationale and implementation immortalized in a big comment in
torch/package/mangling.md.This change also allows imported modules to be TorchScripted
Differential Revision: D25758625