[JIT] Support for NamedTuple #21428

jamesr66a · 2019-06-05T20:47:18Z

Resolves https://github.com/pytorch/lockdown/issues/18

This implements NamedTuple by taking advantage of the existing names field in TupleType.

TODO: This currently doesn't retain the NamedTuple-ness through serialization. Discussed with @suo offline, we can probably make a way to define an anonymous NamedTuple in script (e.g. NamedTuple('Foo', [('a', int), ('b', float), ('c', List[float])]) and serialize that
TODO: implement support for calling the constructor with kwargs

jamesr66a · 2019-06-05T20:47:53Z

test/test_jit_py3.py

flake8 was freaking out here in my pre-commit hook for some reason

facebook-github-bot

@jamesr66a has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@jamesr66a has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@jamesr66a has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

suo

This duplication is sad, but I can't really see a better way short of merging the tuple and class concepts together. Can you add a lockdown P1 to explore whether that's possible? I think it should work if we implement a scalar replacement of aggregates pass.

Otherwise, left some comments inline

torch/csrc/jit/script/init.cpp

aten/src/ATen/core/jit_type.h

torch/jit/__init__.py

torch/csrc/jit/script/init.cpp

suo · 2019-06-10T20:22:44Z

aten/src/ATen/core/ivalue_inl.h

btw, in your serialization PR you'll need to fix up the pickler as well—currently the type member will just get ignore and it'll get pickled as a tuple, which isn't what we want.

torch/jit/__init__.py

facebook-github-bot

@jamesr66a has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

suo

Looks good! Much simpler than before. Only one substantive comment

torch/csrc/jit/script/init.cpp

torch/jit/__init__.py

suo

lgtm. I only glanced the tupleptr reverts, I assume they are just straight reverts. In the future, please stack your PRs so they are easier to review 💯

zdevito

The structure looks good, but I don't there is any type checking being done on tuple construction.
I'd also like to see what happens on unification e.g. a named and unnamed tuple on either side of an if-statement.

torch/csrc/jit/script/sugared_value.cpp

torch/csrc/jit/script/sugared_value.h

torch/csrc/jit/script/sugared_value.cpp

test/test_jit_py3.py

facebook-github-bot

@jamesr66a is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zdevito

Schema checking code is buggy. It is an easy fix but this isn't correct yet.

torch/csrc/jit/script/sugared_value.cpp

zdevito

Schema checking code is buggy. It is an easy fix but this isn't correct yet.

torch/csrc/jit/script/sugared_value.cpp

Summary: Resolves meta-pytorch/lockdown#18 This implements NamedTuple by taking advantage of the existing `names` field in `TupleType`. TODO: This currently doesn't retain the NamedTuple-ness through serialization. Discussed with suo offline, we can probably make a way to define an anonymous NamedTuple in script (e.g. `NamedTuple('Foo', [('a', int), ('b', float), ('c', List[float])])` and serialize that TODO: implement support for calling the constructor with kwargs Pull Request resolved: pytorch#21428 Differential Revision: D15741564 fbshipit-source-id: b981d3c0f058afec5d6a7500d989bb10ac898278

addressed

facebook-github-bot

@jamesr66a is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Resolves meta-pytorch/lockdown#18 This implements NamedTuple by taking advantage of the existing `names` field in `TupleType`. TODO: This currently doesn't retain the NamedTuple-ness through serialization. Discussed with suo offline, we can probably make a way to define an anonymous NamedTuple in script (e.g. `NamedTuple('Foo', [('a', int), ('b', float), ('c', List[float])])` and serialize that TODO: implement support for calling the constructor with kwargs Pull Request resolved: pytorch/pytorch#21428 Differential Revision: D15741564 Pulled By: jamesr66a fbshipit-source-id: c077cbcea1880675ca6deb340a9ec78f824a136c

Summary: Resolves https://github.com/pytorch/lockdown/issues/18 This implements NamedTuple by taking advantage of the existing `names` field in `TupleType`. TODO: This currently doesn't retain the NamedTuple-ness through serialization. Discussed with suo offline, we can probably make a way to define an anonymous NamedTuple in script (e.g. `NamedTuple('Foo', [('a', int), ('b', float), ('c', List[float])])` and serialize that TODO: implement support for calling the constructor with kwargs Pull Request resolved: #21428 Differential Revision: D15741564 fbshipit-source-id: b981d3c0f058afec5d6a7500d989bb10ac898278

facebook-github-bot · 2019-06-15T02:42:48Z

@jamesr66a merged this pull request in 4bcc72f.

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

jamesr66a commented Jun 5, 2019

View reviewed changes

test/test_jit_py3.py Outdated

Copy link

Collaborator Author

jamesr66a Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flake8 was freaking out here in my pre-commit hook for some reason

jamesr66a requested a review from suo June 6, 2019 16:55

jamesr66a changed the title ~~[WIP][JIT] Support for NamedTuple~~ [JIT] Support for NamedTuple Jun 6, 2019

jamesr66a force-pushed the named_tuple_support2 branch 2 times, most recently from 826e4f2 to 75ac46a Compare June 10, 2019 17:25

facebook-github-bot reviewed Jun 10, 2019

View reviewed changes

jamesr66a force-pushed the named_tuple_support2 branch from 75ac46a to 9333d13 Compare June 10, 2019 17:27

facebook-github-bot reviewed Jun 10, 2019

View reviewed changes

jamesr66a force-pushed the named_tuple_support2 branch from 9333d13 to d8f56a9 Compare June 10, 2019 17:59

facebook-github-bot reviewed Jun 10, 2019

View reviewed changes

suo requested changes Jun 10, 2019

View reviewed changes

jamesr66a requested a review from suo June 10, 2019 21:39

suo reviewed Jun 11, 2019

View reviewed changes

torch/jit/__init__.py Outdated Show resolved Hide resolved

jamesr66a force-pushed the named_tuple_support2 branch from bc59b15 to 174e5bc Compare June 12, 2019 04:44

jamesr66a requested review from suo and zdevito June 12, 2019 04:45

jamesr66a force-pushed the named_tuple_support2 branch from 174e5bc to e7b0836 Compare June 12, 2019 04:46

facebook-github-bot reviewed Jun 12, 2019

View reviewed changes

suo reviewed Jun 12, 2019

View reviewed changes

torch/csrc/jit/script/init.cpp Outdated Show resolved Hide resolved

torch/jit/__init__.py Outdated Show resolved Hide resolved

jamesr66a force-pushed the named_tuple_support2 branch from 4502062 to 160fa5f Compare June 13, 2019 00:31

jamesr66a requested review from ebetica, goldsborough and yf225 as code owners June 13, 2019 00:31

pytorchbot added the module: cpp Related to C++ API label Jun 13, 2019

jamesr66a force-pushed the named_tuple_support2 branch from 160fa5f to 3b449e6 Compare June 13, 2019 00:32

jamesr66a requested a review from suo June 13, 2019 00:37

jamesr66a force-pushed the named_tuple_support2 branch from 3b449e6 to 23cd1d8 Compare June 13, 2019 00:49

jamesr66a force-pushed the named_tuple_support2 branch from 23cd1d8 to 28dd375 Compare June 13, 2019 01:21

suo approved these changes Jun 13, 2019

View reviewed changes

jamesr66a force-pushed the named_tuple_support2 branch from 28dd375 to ad133a8 Compare June 13, 2019 04:01

zdevito reviewed Jun 13, 2019

View reviewed changes

torch/csrc/jit/script/sugared_value.cpp Outdated Show resolved Hide resolved

torch/csrc/jit/script/sugared_value.h Outdated Show resolved Hide resolved

torch/csrc/jit/script/sugared_value.cpp Outdated Show resolved Hide resolved

test/test_jit_py3.py Outdated Show resolved Hide resolved

jamesr66a requested a review from zdevito June 13, 2019 18:39

jamesr66a force-pushed the named_tuple_support2 branch from f59855b to 1455e32 Compare June 13, 2019 21:23

facebook-github-bot reviewed Jun 14, 2019

View reviewed changes

zdevito previously requested changes Jun 14, 2019

View reviewed changes

torch/csrc/jit/script/sugared_value.cpp Outdated Show resolved Hide resolved

zdevito reviewed Jun 14, 2019

View reviewed changes

torch/csrc/jit/script/sugared_value.cpp Outdated Show resolved Hide resolved

jamesr66a force-pushed the named_tuple_support2 branch from 1455e32 to b29950d Compare June 14, 2019 18:49

pytorchbot added the caffe2 label Jun 14, 2019

jamesr66a requested a review from zdevito June 14, 2019 19:36

facebook-github-bot reviewed Jun 14, 2019

View reviewed changes

facebook-github-bot closed this in 4bcc72f Jun 14, 2019

jamesr66a mentioned this pull request Jun 15, 2019

Support for NamedTuple (#21428) #21812

Closed

jamesr66a mentioned this pull request Jun 15, 2019

[JIT] Some small fixes for NamedTuple #21813

Closed

facebook-github-bot added the merged label Jun 15, 2019

mruberry added the Merged label Oct 28, 2020

[JIT] Support for NamedTuple #21428

[JIT] Support for NamedTuple #21428

Uh oh!

Conversation

jamesr66a commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesr66a Jun 5, 2019

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

suo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suo Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

suo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

suo left a comment

Choose a reason for hiding this comment

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jamesr66a commented Jun 5, 2019 •

edited

Loading