Improve LegacyTypeDispatch to handle initialization correctly.#11304
Closed
ezyang wants to merge 7 commits intopytorch:export-D9657449from
Closed
Improve LegacyTypeDispatch to handle initialization correctly.#11304ezyang wants to merge 7 commits intopytorch:export-D9657449from
ezyang wants to merge 7 commits intopytorch:export-D9657449from
Conversation
27e2db5 to
cbe3a52
Compare
cbe3a52 to
41cd25d
Compare
pytorch#11283) Summary: …n TensorImpl. Pull Request resolved: pytorch#11283 Reviewed By: ezyang Differential Revision: D9660015 Pulled By: gchanan fbshipit-source-id: 263cba226d9ee981d55281c94e6fda5842a46b02
41cd25d to
74baa87
Compare
768e049 to
89bf56a
Compare
Summary: Also had to move OptionsGuard. Differential Revision: D9646190 fbshipit-source-id: f7483455fbda014a8ea97880ed38232514ac2c7d
Summary: ``` codemod -d . --extensions cc,cpp,cu,cuh,h getMaybeVariableType getType ``` Differential Revision: D9648830 fbshipit-source-id: 96e454298747f0abbd75c583c9ab2974646cb013
Summary: Still need to deduplicate this with caffe2/core/registry.h, but this will be a bit tricky because the current formulation of the macro is namespace sensitive (i.e., the macro for classes defined in at:: namespace won't work if you call from caffe2:: namespace). Differential Revision: D9654871 fbshipit-source-id: 6a5cb46dbb5b91fc8a1480494997de5b07a3bc3a
Summary: This one might strike you as a bit surprising, but it's necessary to expose this interface in ATen/core, because we need to be able to get a true Variable type from Variable tensors, and to do that we need to go through the hooks interface. Differential Revision: D9656548 fbshipit-source-id: 53820fcd76c60fafeb751d33432db570459cd8cc
Summary: We don't want to put all of Context into ATen/core, but one particular part cannot be avoided: the type registry, because implementations of TensorMethods will need to get a Type, and then do a virtual call on it. I needed to do a little bit of (temporary) footwork to get this in without also moving Type, because unique_ptr<Type> expects to be able to see the destructor of Type (but it's forward declared right now). So instead I put the destructor as an explicit functor. We can get rid of this once Type actually moves in ATen/core Differential Revision: D9657449 fbshipit-source-id: ae83d80bd8893034fe5acf3e660b788162946602
…ch#11304) Summary: Pull Request resolved: pytorch#11304 Pull Request resolved: pytorch#11331 In the previous commit, we added a bare-bones LegacyTypeDispatch in ATen/core. This is not sufficient for the use cases we need: we not only need to be able to get a Type, but we also need to be able to *initialize* the Types if its the first time we have retrieved a CPU/CUDA/Complex type. I hemmed and hawed about how to do this; the strategy this PR takes is to introduce a new "hooks" interface specifically for initializing CPU/CUDA/Complex (which still lives in Context). We then move all "user-friendly" functions to LegacyTypeDispatch. Here were some other options which I considered, but don't work: - Assume that Type is already initialized, because we only intend to call Type from Tensor methods, where we already have a Tensor. This does not work because Caffe2 created tensors will not have gone through the standard Type codepath, and will have skipped initialization. - Move CUDAHooks and ComplexHooks to ATen/core. Besides being sucky, this isn't even a complete fix, because I still need to initialize CPU hooks (so you *still* need another hooks interface). Differential Revision: D9666612 fbshipit-source-id: d27c0f824aaf885236fae70594d9687079babad3
89bf56a to
5c98186
Compare
This was referenced Sep 6, 2018
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
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.
Improve LegacyTypeDispatch to handle initialization correctly.
In the previous commit, we added a bare-bones LegacyTypeDispatch in ATen/core.
This is not sufficient for the use cases we need: we not only need to be able to
get a Type, but we also need to be able to initialize the Types if its the first time
we have retrieved a CPU/CUDA/Complex type. I hemmed and hawed about how
to do this; the strategy this PR takes is to introduce a new "hooks" interface
specifically for initializing CPU/CUDA/Complex (which still lives in Context). We then
move all "user-friendly" functions to LegacyTypeDispatch.
Here were some other options which I considered, but don't work:
from Tensor methods, where we already have a Tensor. This does not work
because Caffe2 created tensors will not have gone through the standard
Type codepath, and will have skipped initialization.
this isn't even a complete fix, because I still need to initialize CPU hooks
(so you still need another hooks interface).
Differential Revision: D9666612
Stack: