[jit] Namespace isolation for classes #19903
Closed
Conversation
This file contains hidden or 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
Previously class types were owned by a global registry. That causes isolation issues when you want to load two models into C++ but they define classes of the same name. To solve this, we make classes compilation unit-local. Classes defined by loaded models are defined on the main module's compilation unit. For Python, we have a single global compilation unit (similar to what we had before). This isn't ideal but is hard to fix until we refactor compilation unit ownership semantics more broadly.
pytorchbot
added
oncall: jit
[jit] Namespace isolation for classes Previously class types were owned by a global registry. That causes isolation issues when you want to load two models into C++ but they define classes of the same name. To solve this, we make classes compilation unit-local. Classes defined by loaded models are defined on the main module's compilation unit. For Python, we have a single global compilation unit (similar to what we had before). This isn't ideal but is hard to fix until we refactor compilation unit ownership semantics more broadly. gh-metadata: pytorch pytorch 19903 gh/suo/33/head
[jit] Namespace isolation for classes Previously class types were owned by a global registry. That causes isolation issues when you want to load two models into C++ but they define classes of the same name. To solve this, we make classes compilation unit-local. Classes defined by loaded models are defined on the main module's compilation unit. For Python, we have a single global compilation unit (similar to what we had before). This isn't ideal but is hard to fix until we refactor compilation unit ownership semantics more broadly. gh-metadata: pytorch pytorch 19903 gh/suo/33/head
[jit] Namespace isolation for classes Previously class types were owned by a global registry. That causes isolation issues when you want to load two models into C++ but they define classes of the same name. To solve this, we make classes compilation unit-local. Classes defined by loaded models are defined on the main module's compilation unit. For Python, we have a single global compilation unit (similar to what we had before). This isn't ideal but is hard to fix until we refactor compilation unit ownership semantics more broadly. gh-metadata: pytorch pytorch 19903 gh/suo/33/head
[jit] Namespace isolation for classes Previously class types were owned by a global registry. That causes isolation issues when you want to load two models into C++ but they define classes of the same name. To solve this, we make classes compilation unit-local. Classes defined by loaded models are defined on the main module's compilation unit. For Python, we have a single global compilation unit (similar to what we had before). This isn't ideal but is hard to fix until we refactor compilation unit ownership semantics more broadly. gh-metadata: pytorch pytorch 19903 gh/suo/33/head
zdevito
approved these changes
May 8, 2019
Contributor
zdevito
left a comment
zdevito
left a comment
There was a problem hiding this comment.
Looks good. Right now if you cross use a Class with the same qualified name then export is going to end up with name conflicts, right? Thats fine for now.
| return nullptr; | ||
| } | ||
|
|
||
| /** |
Contributor
There was a problem hiding this comment.
I feel like this should be in a python-only compilation unit, for clarity.
Contributor
There was a problem hiding this comment.
Maybe python_sugared_value.cpp
Member
Author
There was a problem hiding this comment.
Unfortunately because of dependency stuff I can't move this cleanly; I'll follow up with the change in another PR
zdevito
pushed a commit
to zdevito/ATen
that referenced
this pull request
May 8, 2019
Summary: Pull Request resolved: pytorch/pytorch#19903 ghimport-source-id: deadf59f469ad620d0ee10b089dfc9bb92171710 Differential Revision: D15118978 Pulled By: suo fbshipit-source-id: f2b487fd65520d1b7f45cb74145634d334ef1614
Contributor
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:
Previously class types were owned by a global registry. That causes
isolation issues when you want to load two models into C++ but they
define classes of the same name.
To solve this, we make classes compilation unit-local. Classes defined
by loaded models are defined on the main module's compilation unit.
For Python, we have a single global compilation unit (similar to what we
had before). This isn't ideal but is hard to fix until we refactor
compilation unit ownership semantics more broadly.
Differential Revision: D15118978