fix jit List[Optional[Tensor]] type singleton bug#77846
Closed
bdhirsh wants to merge 2 commits intogh/bdhirsh/239/basefrom
Closed
fix jit List[Optional[Tensor]] type singleton bug#77846bdhirsh wants to merge 2 commits intogh/bdhirsh/239/basefrom
bdhirsh wants to merge 2 commits intogh/bdhirsh/239/basefrom
Conversation
[ghstack-poisoned]
Contributor
🔗 Helpful links
❌ 2 New Failures, 1 PendingAs of commit 8716053 (more details on the Dr. CI page): Expand to see more
🕵️ 2 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages
|
This should fix the remaining dynamo <> functionalization integration issue. I had a fix earlier for JIT containers not correctly having singleton instances [here](#76085), which fixed a bug in this code for functionalization: ``` def f(a, b): return a[b] functionalize(foo)(torch.arange(3), torch.ones(2, dtype=torch.long)) ``` But apparently the following code is still broken: ``` def f(a, b): return torch.ops.aten.index(a, b) functionalize(foo)(torch.arange(3), torch.ones(2, dtype=torch.long)) ``` Why? we have separate schema parsing logic for the ops in `torch.ops.aten`, and that logic circumvented my fix, creating its own singleton instance for Optional[Tensor]. We can't test this in core for the same reason as the last fix, so companion functorch tests here: pytorch/functorch#820 [ghstack-poisoned]
bdhirsh
commented
May 19, 2022
| fake_value = c10::TypeFactory::create<c10::OptionalType>(fake_value); | ||
| real_value = c10::TypeFactory::create<c10::OptionalType>(real_value); | ||
| fake_value = c10::OptionalType::get(fake_value); | ||
| real_value = c10::OptionalType::get(real_value); |
Collaborator
Author
There was a problem hiding this comment.
There is probably a more general fix to this problem, but I just wanted to do something simple that would fix dynamo
For example, code that calls TypeFactory::create<ListType> would probably end up creating multiple "singleton" instances of a given ListType, but that doesn't matter in practice because the logic for checking if two list types are the same in IValue.h basically just checks if both types are list-like, and then pointer-compares their inner types.
ezyang
approved these changes
May 19, 2022
Collaborator
Author
|
@pytorchbot merge this please |
Contributor
|
Hey @bdhirsh. |
facebook-github-bot
pushed a commit
that referenced
this pull request
May 20, 2022
Summary: Pull Request resolved: #77846 Approved by: https://github.com/ezyang Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/de646c06d457277f24d790eb41545e72f2d76f66 Reviewed By: seemethere Differential Revision: D36537747 Pulled By: bdhirsh fbshipit-source-id: e5f8a544f2275301f6229ea57fbfb8266d959819
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
4 participants
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.
This should fix the remaining dynamo <> functionalization integration issue.
I had a fix earlier for JIT containers not correctly having singleton instances here, which fixed a bug in this code for functionalization:
But apparently the following code is still broken:
Why? we have separate schema parsing logic for the ops in
torch.ops.aten, and that logic circumvented my fix, creating its own singleton instance for Optional[Tensor].We can't test this in core for the same reason as the last fix, so companion functorch tests here: pytorch/functorch#820
Stack from ghstack: