Test TensorTypeSet instead of autograd_meta_ for variable-ness. #28543
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
By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
This was referenced Oct 23, 2019
ezyang
added a commit
that referenced
this pull request
Oct 23, 2019
By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: a551928 Pull Request resolved: #28543
…ness." By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
ezyang
added a commit
that referenced
this pull request
Oct 23, 2019
By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> ghstack-source-id: 97315a3 Pull Request resolved: #28543
…ness." By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
…ness." By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
…ness." By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
…ness." By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
This was referenced Oct 24, 2019
yf225
approved these changes
Oct 24, 2019
c10/core/TensorImpl.h
Outdated
| // of std::default_delete<AutogradMeta> which won't work as AutogradMeta | ||
| // is incomplete at this point. But the defaulting to nullptr is also | ||
| // pointless because unique_ptr has a default constructor which will do the | ||
| // right thing. |
There was a problem hiding this comment.
Is the NB: part of the comment meant for another PR? Because it says We CANNOT default this to nullptr but we still do in this PR :D
There was a problem hiding this comment.
Yeah it's wrong. I just fixed it
…ness." By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
albanD
approved these changes
Oct 25, 2019
…ness." By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
…ness." By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D18171157](https://our.internmc.facebook.com/intern/diff/D18171157) [ghstack-poisoned]
…ness." By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D18171157](https://our.internmc.facebook.com/intern/diff/D18171157) [ghstack-poisoned]
…ness." By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D18171157](https://our.internmc.facebook.com/intern/diff/D18171157) [ghstack-poisoned]
…ness." By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented in the right place!), these are equivalent. But when I introduce null autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is going to give me the right information no matter what. In the long run, this patch will be a wash, because everything will "be a variable" in the long term. But I am making this change now to make sure that the invariant actually holds. Signed-off-by: Edward Z. Yang <ezyang@fb.com> Differential Revision: [D18171157](https://our.internmc.facebook.com/intern/diff/D18171157) [ghstack-poisoned]
CircleCI build failures summaryAs of commit 49a1e11:
Here are the reasons each build failed. This comment was automatically generated by Dr. CI. Please report bugs/suggestions on the GitHub issue tracker. |
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:
By the current autograd_meta_ <=> type_set_ invariant (now explicitly documented
in the right place!), these are equivalent. But when I introduce null
autograd_meta_ optimization, they won't be equivalent anymore: TensorTypeSet is
going to give me the right information no matter what.
In the long run, this patch will be a wash, because everything will "be a variable"
in the long term. But I am making this change now to make sure that the invariant
actually holds.
Signed-off-by: Edward Z. Yang ezyang@fb.com
Differential Revision: D18171157