dictKeys and dictItems ops on typed dicts return typed lists #23270
Conversation
Differential Revision: [D16448942](https://our.internmc.facebook.com/intern/diff/D16448942/)
pytorchbot
added
oncall: jit
Differential Revision: [D16448942](https://our.internmc.facebook.com/intern/diff/D16448942/) ghstack-source-id: 87040601 Pull Request resolved: #23270
|
This was fixed separately in #23267 , you might need to rebase past that |
Differential Revision: [D16448942](https://our.internmc.facebook.com/intern/diff/D16448942/)
|
I rebased past it |
Differential Revision: [D16448942](https://our.internmc.facebook.com/intern/diff/D16448942/)
Pull Request resolved: #23270 ghstack-source-id: 87092122 Differential Revision: [D16448942](https://our.internmc.facebook.com/intern/diff/D16448942/)
eellison
left a comment
eellison
left a comment
There was a problem hiding this comment.
What is the goal of the PR?
| c10::List<Elem> makeListForDictKeysOrValues( | ||
| const std::pair<c10::optional<TypePtr>, c10::optional<TypePtr>>& types, | ||
| const std::vector<std::pair<IValue, IValue>>& order) { | ||
| TORCH_INTERNAL_ASSERT( |
There was a problem hiding this comment.
What is the point of the changes to this function ? it already worked
There was a problem hiding this comment.
Additional error checking that the lists we're passing around have the correct type
There was a problem hiding this comment.
the typesystem should already ensure that it's the correct type. if we're adding runtime enforcements here shouldn't we be adding a runtime enforcement to every op that involves a list?
There was a problem hiding this comment.
It's not a huge deal either way but I still would prefer not to set the precedent that we don't trust the input types of the statically typed langauge.
@zdevito thoughts ?
There was a problem hiding this comment.
to reiterate from our offline discussion: What this code should do is take the type from the source dict and write it to the target list, same as what makeGenericListForDictKeysOrValues is doing.
Unfortunately, only GenericList has a constructor that takes a runtime type and can do this. List<T> doesn't need that constructor because it infers the runtime type from its T type parameter. So instead of getting the runtime type from the dict and passing it to the list, the only thing we can do is check that this inferred type is the same we would have passed in if we could have.
Note that this is a TORCH_INTERNAL_ASSERT and not a TORCH_CHECK because this is not a type check that reports errors to users. Static typing should make sure that this never fails. But it can fail if there's a PyTorch bug and if that happens, I'd rather we catch that here than just ignore it.
There was a problem hiding this comment.
This is just an assert, so it is ok. It improves the error message that would otherwise happen in the to call.
| const std::pair<c10::optional<TypePtr>, c10::optional<TypePtr>>& types, | ||
| const std::vector<std::pair<IValue, IValue>>& order) { | ||
| auto values = c10::impl::GenericList(c10::impl::deprecatedUntypedList()); | ||
| auto type = std::get<Index>(types); |
There was a problem hiding this comment.
Here, it's more than just error checking. Without this, the runtime key/value type of the Dict wouldn't propagate into the resulting list.
There was a problem hiding this comment.
We create the type of the list based on the static type. E.g. an optional list of ints with a value of [1] should be a generic list. If it this created an intlist that would be a bug.
how exactly is the runtime type set ? and as with ^^^ the type system should already enforce the correct runtime value
|
@eellison Each Dict and List stores a type pointer for their elements at runtime and this PR makes sure that this type information is correctly propagated when calling these ops. |
| c10::List<Elem> makeListForDictKeysOrValues( | ||
| const std::pair<c10::optional<TypePtr>, c10::optional<TypePtr>>& types, | ||
| const std::vector<std::pair<IValue, IValue>>& order) { | ||
| TORCH_INTERNAL_ASSERT( |
There was a problem hiding this comment.
This is just an assert, so it is ok. It improves the error message that would otherwise happen in the to call.
Differential Revision: [D16448942](https://our.internmc.facebook.com/intern/diff/D16448942/)
Pull Request resolved: #23270 ghstack-source-id: 87389530 Differential Revision: [D16448942](https://our.internmc.facebook.com/intern/diff/D16448942/)
Summary: Pull Request resolved: pytorch/pytorch#23270 ghstack-source-id: 87389530 Differential Revision: D16448942 fbshipit-source-id: e6b578f0e97776112259d7ea38e143e4716ec273
|
This pull request has been merged in 30bc19d. |
Stack from ghstack:
Differential Revision: D16448942