[ivalue] Add tuple constructor + to<std::tuple<Args...>>#26668
[ivalue] Add tuple constructor + to<std::tuple<Args...>>#26668bwasti wants to merge 1 commit intopytorch:masterfrom
Conversation
aten/src/ATen/core/ivalue_inl.h
Outdated
There was a problem hiding this comment.
It's probably better if we use SFINAE based on is_constructible<IValue, Args>..., if it works.
aten/src/ATen/core/ivalue.h
Outdated
There was a problem hiding this comment.
Can we please make this constructor private?
There was a problem hiding this comment.
Also, we seem to have a polyfill for std::apply in C++17.h, can we just use that directly here?
ezyang
left a comment
There was a problem hiding this comment.
I think this could be simpler.
94a68e0 to
28696a1
Compare
|
simplified it a bit and address sfinae comment note: I didn't use ghstack for this so it has some (two lines) of the changes in #26739 |
28696a1 to
265fe40
Compare
aten/src/ATen/core/ivalue_inl.h
Outdated
There was a problem hiding this comment.
This is pleasing to my eye!
aten/src/ATen/core/ivalue_inl.h
Outdated
There was a problem hiding this comment.
Nit: avoid dumping helper functions like this in global namespace; put it in namespace detail { ... } or similar
aten/src/ATen/core/ivalue_inl.h
Outdated
There was a problem hiding this comment.
There should be a runtime test that the number of elements in the tuple matches the number of arguments in the parameter pack. I don't see that test in this function, or the function above.
ezyang
left a comment
There was a problem hiding this comment.
Looks good, but we need that runtime test that the number of tuple elements matches the IValue...
aten/src/ATen/core/ivalue.cpp
Outdated
There was a problem hiding this comment.
Should not be part of this PR.
There was a problem hiding this comment.
yea, sorry -- I mentioned that in an above comment (I didn't use ghstack for this). will go away
265fe40 to
3495ff5
Compare
|
updated -- the windows build failure looks legit so I'm looking into that edit: it's just this bug #25389 |
aten/src/ATen/core/ivalue_inl.h
Outdated
There was a problem hiding this comment.
Hmm... an assert? The existing guidance in this file is a bit murky: it is true most of the type tests are done using AT_ASSERT, but that old mean actually means that we haven't appropriately applied the guidance from #20287
I think ivalue tests should be done with TORCH_CHECK. The reasoning is that IValue is part of the public C++ API, and a user may very well attempt to extract an IValue at the wrong type. IT would be wrong for them to report an error message to PyTorch in that case.
3495ff5 to
0874c01
Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
@bwasti has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
0874c01 to
9c88f77
Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
@bwasti is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: This PR adds the ability to use tuples directly with the IValue constructor rather than the vector<IValue> approach Pull Request resolved: pytorch/pytorch#26668 Differential Revision: D17668653 Pulled By: bwasti fbshipit-source-id: aff7f62fe3b502df78b28b2355cff88d88ad288c
Summary: This PR adds the ability to use tuples directly with the IValue constructor rather than the vector<IValue> approach Pull Request resolved: pytorch#26668 Differential Revision: D17668653 Pulled By: bwasti fbshipit-source-id: aff7f62fe3b502df78b28b2355cff88d88ad288c
Summary: This PR adds the ability to use tuples directly with the IValue constructor rather than the vector<IValue> approach Pull Request resolved: pytorch#26668 Differential Revision: D17668653 Pulled By: bwasti fbshipit-source-id: aff7f62fe3b502df78b28b2355cff88d88ad288c
This PR adds the ability to use tuples directly with the IValue constructor rather than the vector approach