-
Notifications
You must be signed in to change notification settings - Fork 22.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JIT] Support for NamedTuple #21428
[JIT] Support for NamedTuple #21428
Conversation
|
||
@torch.jit.script | ||
def foo(x) -> float: | ||
fv = FeatureVector(3.0, [3.0], 3.0) # noqa |
There was a problem hiding this comment.
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
826e4f2
to
75ac46a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamesr66a has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
75ac46a
to
9333d13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamesr66a has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
9333d13
to
d8f56a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamesr66a has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
aten/src/ATen/core/ivalue_inl.h
Outdated
@@ -192,9 +193,21 @@ struct GenericDict; | |||
|
|||
struct CAFFE2_API Tuple : public List<IValue> { | |||
using List<IValue>::List; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
bc59b15
to
174e5bc
Compare
174e5bc
to
e7b0836
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamesr66a has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Much simpler than before. Only one substantive comment
4502062
to
160fa5f
Compare
160fa5f
to
3b449e6
Compare
3b449e6
to
23cd1d8
Compare
23cd1d8
to
28dd375
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 💯
28dd375
to
ad133a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
f59855b
to
1455e32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamesr66a is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Schema checking code is buggy. It is an easy fix but this isn't correct yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Schema checking code is buggy. It is an easy fix but this isn't correct yet.
Summary: Resolves 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
1455e32
to
b29950d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamesr66a is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Resolves 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
@jamesr66a merged this pull request in 4bcc72f. |
Resolves https://github.com/pytorch/lockdown/issues/18
This implements NamedTuple by taking advantage of the existing
names
field inTupleType
.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 thatTODO: implement support for calling the constructor with kwargs