Add ivalue::type(), part 1 by zdevito · Pull Request #25439 · pytorch/pytorch

zdevito · 2019-08-30T00:13:38Z

Stack from ghstack:

[JIT] add type refinements for isinstance checks #26271 [JIT] add type refinements for isinstance checks
[jit] refactor extra sugared values #26270 [jit] refactor extra sugared values
[jit] add dynamic isinstance #26269 [jit] add dynamic isinstance
Allow types as node attributes #26268 Allow types as node attributes
[JIT] add CondValue to unify refinements and code emission #26145 [JIT] add CondValue to unify refinements and code emission
[jit] Refactor emitIsInstance #26061 [jit] Refactor emitIsInstance
Use static type information to restore type tags #25447 Use static type information to restore type tags
Add ivalue::type(), part 1 #25439 Add ivalue::type(), part 1

This introduces a type() method on IValue that returns the tagged type
of the IValue. The intention is that this value is always present/accurate,
making it possible for clients to recover the Type from an IValue.
Currently our APIs here are incomplete: they can sometimes recover a type but not always.

This PR adds the function, and cleans up remaining cases where Lists/Dicts are not
tagged. However, this information does not survive serialization unchanged.

A second PR will use the type information in the ClassType being serialized
to fixup the serialized ivalues to have the correct types again.
After this patch it will be save to remove our incomplete APIs for recovering types.

Differential Revision: D17125595

This introduces a type() method on IValue that returns the tagged type of the IValue. The intention is that this value is always present/accurate, making it possible for clients to recover the Type from an IValue. Currently our APIs here are incomplete: they can sometimes recover a type but not always. This PR adds the function, and cleans up remaining cases where Lists/Dicts are not tagged. However, this information does not survive serialization unchanged. A second PR will use the type information in the ClassType being serialized to fixup the serialized ivalues to have the correct types again. After this patch it will be save to remove our incomplete APIs for recovering types.

Add ivalue::type(), part 1 This introduces a type() method on IValue that returns the tagged type of the IValue. The intention is that this value is always present/accurate, making it possible for clients to recover the Type from an IValue. Currently our APIs here are incomplete: they can sometimes recover a type but not always. This PR adds the function, and cleans up remaining cases where Lists/Dicts are not tagged. However, this information does not survive serialization unchanged. A second PR will use the type information in the ClassType being serialized to fixup the serialized ivalues to have the correct types again. After this patch it will be save to remove our incomplete APIs for recovering types. gh-metadata: pytorch pytorch 25439 gh/zdevito/101/head

Add ivalue::type(), part 1 This introduces a type() method on IValue that returns the tagged type of the IValue. The intention is that this value is always present/accurate, making it possible for clients to recover the Type from an IValue. Currently our APIs here are incomplete: they can sometimes recover a type but not always. This PR adds the function, and cleans up remaining cases where Lists/Dicts are not tagged. However, this information does not survive serialization unchanged. A second PR will use the type information in the ClassType being serialized to fixup the serialized ivalues to have the correct types again. After this patch it will be save to remove our incomplete APIs for recovering types. gh-metadata: pytorch pytorch 25439 gh/zdevito/101/head temp

eellison · 2019-09-11T22:39:09Z

How do you see this interacting with users who have to construct IValues in c++ ? It should make it easier right ?

zdevito · 2019-09-12T02:43:37Z

It should make it easier to convert IValues accurately to/from Python since we always will know their types. For C++ it will require specifying the type of list/dict on creation, which is something our API already supports.

Add ivalue::type(), part 1 This introduces a type() method on IValue that returns the tagged type of the IValue. The intention is that this value is always present/accurate, making it possible for clients to recover the Type from an IValue. Currently our APIs here are incomplete: they can sometimes recover a type but not always. This PR adds the function, and cleans up remaining cases where Lists/Dicts are not tagged. However, this information does not survive serialization unchanged. A second PR will use the type information in the ClassType being serialized to fixup the serialized ivalues to have the correct types again. After this patch it will be save to remove our incomplete APIs for recovering types. gh-metadata: pytorch pytorch 25439 gh/zdevito/101/head temp

suo

lgtm, some minor comments/questions inline

suo · 2019-09-17T16:18:08Z

aten/src/ATen/core/ivalue_inl.h

      std::ostream& out,
      const Future& v);

+  TypePtr type() {


Should be const

as currently written I think value() is not const because it holds the mutex. I think it isafe to just have it return type_ which also avoids the changing type tag behavior.

suo · 2019-09-17T16:19:07Z

aten/src/ATen/core/ivalue_inl.h

      const Future& v);

+  TypePtr type() {
+    if (!completed()) {


This seems a little strange—should the type of a Future change depending on its completion state?

suo · 2019-09-17T16:22:45Z

aten/src/ATen/core/ivalue_inl.h

-  static c10::intrusive_ptr<Tuple> create(std::vector<IValue> elements_, std::shared_ptr<TupleType> type_) {
-    TORCH_INTERNAL_ASSERT(nullptr != type_.get(), "Type cannot be nullptr");
+  // named tuples have additional type information, so we
+  // direclty create them tagged


spelling: directly

suo · 2019-09-17T16:24:37Z

aten/src/ATen/core/ivalue.cpp

+    case Tag::Object:
+      return toObjectRef().type();
+    case Tag::Uninitialized:
+      return AnyType::get();


Aside: what is Uninitialized anyway? I'm asking because IValues that are Any type should be scrutinized

It is introduce when we have early returns, break, and continue. It represents an uninitialized IValue that we know because of invariants introduced in the pass cannot be used. This fact isn't representable in the graph directly so this placeholder is needed. The alternative, representing them with optional values, requires a lot of unwrapping logic around use sites.

suo · 2019-09-17T16:26:26Z

aten/src/ATen/core/jit_type.h

        strides_(tensor.sizes().size()),
        requires_grad_(tensor.requires_grad()) {
-          if (!tensor.is_mkldnn()) {
+          if (!tensor.is_mkldnn() && !tensor.is_sparse()) {


what's the reason for this change?

we can't ask for strides on tensors that do not have them. We now have tests that ask for the type of tuples, some of which contain sparse tensors, which uncovered this bug.

suo · 2019-09-17T16:28:20Z

aten/src/ATen/core/jit_type.h


  static DictTypePtr create(TypePtr key, TypePtr value) {
    switch (key->kind()) {
+      case TypeKind::AnyType:


Any is not really a valid key type for dictionaries, right? Since it may end up being something not in the list below.

I suppose there's not an easy way to represent empty dicts without another KeyType cluttering things, so I think it's fine to leave. How are we ensuring that it's not possible to somehow construct a dictionary with an invalid key type?

This check is the at-creation-type check that the dict can't possibly work. There is still a at insertion/lookup-time check that the key is hashable. It is valid to have a Any -> X dictionary. It is just the case that dynamically trying to insert a non-hashable key into it will fail.

Add ivalue::type(), part 1 This introduces a type() method on IValue that returns the tagged type of the IValue. The intention is that this value is always present/accurate, making it possible for clients to recover the Type from an IValue. Currently our APIs here are incomplete: they can sometimes recover a type but not always. This PR adds the function, and cleans up remaining cases where Lists/Dicts are not tagged. However, this information does not survive serialization unchanged. A second PR will use the type information in the ClassType being serialized to fixup the serialized ivalues to have the correct types again. After this patch it will be save to remove our incomplete APIs for recovering types. gh-metadata: pytorch pytorch 25439 gh/zdevito/101/head temp

Summary: Seems CI was broken by PR #25439 - fix based on interface change. Test Plan: - build locally ghstack-source-id: 7d28006 Pull Request resolved: #26448

Summary: Seems CI was broken by PR #25439 - fix based on interface change. Test Plan: - build locally Differential Revision: [D17468987](https://our.internmc.facebook.com/intern/diff/D17468987) [ghstack-poisoned]

Summary: Seems CI was broken by PR #25439 - fix based on interface change. Test Plan: - build locally ghstack-source-id: 9ce9ffb Pull Request resolved: #26448

Summary: Pull Request resolved: #26448 Seems CI was broken by PR #25439 - fix based on interface change. Test Plan: - build locally Differential Revision: D17468987 Pulled By: ljk53 fbshipit-source-id: 3c1cb582c8d05357a94295b670b2ce61a7a5a4cd

Summary: PR #25439 introduced type() to IValue, which enforced ivalue::Future to be constructed with type. This broke ParallelNative.h and a few other places where Future was constructed without type and simply used as a barrier: https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/ParallelNative.h#L63 We could set NoneType by default if we want to enforce non-null type everywhere, but looks like Tuple can take null TupleType so just follow it. Test Plan: - builds [ghstack-poisoned]

Summary: PR #25439 introduced type() to IValue, which enforced ivalue::Future to be constructed with type. This broke ParallelNative.h and a few other places where Future was constructed without type and simply used as a barrier: https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/ParallelNative.h#L63 We could set NoneType by default if we want to enforce non-null type everywhere, but looks like Tuple can take null TupleType so just follow it. Test Plan: - builds Differential Revision: [D17556002](https://our.internmc.facebook.com/intern/diff/D17556002) [ghstack-poisoned]

Summary: PR #25439 introduced type() to IValue, which enforced ivalue::Future to be constructed with type. This broke ParallelNative.h and a few other places where Future was constructed without type and simply used as a barrier: https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/ParallelNative.h#L63 Use NoneType::get() to initialize these Futures. Test Plan: - builds Differential Revision: [D17556002](https://our.internmc.facebook.com/intern/diff/D17556002) [ghstack-poisoned]

…bsolutely have to, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" Looks like it was forgotten in #25439 [ghstack-poisoned]

…o, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" Looks like it was forgotten in #25439 [ghstack-poisoned]

…bsolutely have to, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" Looks like it was forgotten in #25439 [ghstack-poisoned]

…o, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" Looks like it was forgotten in #25439 [ghstack-poisoned]

…bsolutely have to, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" Looks like it was forgotten in #25439 [ghstack-poisoned]

…o, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" Looks like it was forgotten in #25439 [ghstack-poisoned]

…bsolutely have to, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" Looks like it was forgotten in #25439 [ghstack-poisoned]

…o, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" Looks like it was forgotten in #25439 [ghstack-poisoned]

…:impl::GenericDict(c10::impl::deprecatedUntypedDict()) (#65164) Summary: Pull Request resolved: #65164 Looks like it was forgotten in #25439 Test Plan: Imported from OSS Reviewed By: malfet Differential Revision: D31072625 Pulled By: pbelevich fbshipit-source-id: a5ffcfb0836f962ab6952a187ba7717c4d4a6e33

zdevito requested a review from apaszke as a code owner August 30, 2019 00:13

pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: cpp Related to C++ API module: internals Related to internal abstractions in c10 and ATen module: pybind Related to our Python bindings / interactions with other Python libraries labels Aug 30, 2019

This was referenced Aug 30, 2019

[jit] Make NoneType <: Optional[T] #25361

Closed

temp for tagging tuples #25438

Closed

zdevito added 3 commits August 29, 2019 17:14

zdevito mentioned this pull request Aug 30, 2019

Use static type information to restore type tags #25447

Closed

zdevito added 4 commits August 30, 2019 10:21

zdevito requested a review from suo September 11, 2019 20:00

zdevito mentioned this pull request Sep 11, 2019

[jit] Refactor emitIsInstance #26061

Closed

suo approved these changes Sep 17, 2019

View reviewed changes

zdevito added 3 commits September 17, 2019 11:52

ljk53 mentioned this pull request Sep 19, 2019

[pytorch] fix JNI wrapper for IValue interface change #26448

Closed

ljk53 added a commit that referenced this pull request Sep 19, 2019

[pytorch] fix JNI wrapper for IValue interface change

b2459b5

Summary: Seems CI was broken by PR #25439 - fix based on interface change. Test Plan: - build locally ghstack-source-id: 7d28006 Pull Request resolved: #26448

ljk53 added a commit that referenced this pull request Sep 19, 2019

[pytorch] fix JNI wrapper for IValue interface change

5eb43da

Summary: Seems CI was broken by PR #25439 - fix based on interface change. Test Plan: - build locally ghstack-source-id: 9ce9ffb Pull Request resolved: #26448

ljk53 mentioned this pull request Sep 24, 2019

[pytorch] fix ParallelNative.h/cpp build #26746

Closed

facebook-github-bot deleted the gh/zdevito/101/head branch October 28, 2019 22:23

mruberry added the Merged label Oct 28, 2020

pbelevich mentioned this pull request Sep 16, 2021

Delete obsolete message in Dict: "or if you absolutely have to, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" #65164

Closed

pbelevich added a commit that referenced this pull request Sep 17, 2021

Update on "[JIT] Delete obsolete message: or if you absolutely have t…

88dfe95

…o, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" Looks like it was forgotten in #25439 [ghstack-poisoned]

pbelevich added a commit that referenced this pull request Sep 17, 2021

Update on "[JIT] Delete obsolete message: or if you absolutely have t…

88dd4f0

…o, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" Looks like it was forgotten in #25439 [ghstack-poisoned]

pbelevich added a commit that referenced this pull request Sep 20, 2021

Update on "[JIT] Delete obsolete message: or if you absolutely have t…

292bae3

…o, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" Looks like it was forgotten in #25439 [ghstack-poisoned]

pbelevich added a commit that referenced this pull request Sep 20, 2021

Update on "[JIT] Delete obsolete message: or if you absolutely have t…

62d2ac5

…o, use c10::impl::GenericDict(c10::impl::deprecatedUntypedDict())" Looks like it was forgotten in #25439 [ghstack-poisoned]

Conversation

zdevito commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eellison commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zdevito commented Sep 12, 2019

Uh oh!

suo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zdevito commented Aug 30, 2019 •

edited

Loading

eellison commented Sep 11, 2019 •

edited

Loading