Add support for homogenous tuples#44774
Conversation
Codecov Report
@@ Coverage Diff @@
## master #44774 +/- ##
==========================================
- Coverage 68.10% 68.10% -0.01%
==========================================
Files 393 393
Lines 50945 50945
==========================================
- Hits 34698 34697 -1
- Misses 16247 16248 +1
Continue to review full report at Codecov.
|
facebook-github-bot
left a comment
There was a problem hiding this comment.
@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
da89c40 to
22f9df3
Compare
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit deb19e7 (more details on the Dr. CI page): Commit deb19e7 was recently pushed. Waiting for builds... This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
Alias homogenous tuples to lists in jit frontend parser Homogenous tuple is defined a `Tuple[Type, ...]` and semantically very similar to `List[Type]` This would allow one to use `torch.nn.common_types._size_any_t` to type annotate jit functions
1a54c65 to
844f628
Compare
844f628 to
deb19e7
Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| if (rhs_->kind() == AnyTupleType::Kind) { | ||
| return true; | ||
| } | ||
| if (rhs_->kind() == ListType::Kind) { |
There was a problem hiding this comment.
This rule seems off. It suggests that Tuple[int, ...] <: List[Tensor].
I don't think VariableLengthTuple should subtype any kind of List. It cannot be used safely in place of a List, since List allows additional operations which VariableLengthTuple does not.
| } else if(t.kind() == TypeKind::ListType) { | ||
| auto prim = t.cast<ListType>()->getElementType(); | ||
| out << *prim << "[]"; | ||
| } else if(t.kind() == TypeKind::VariableLengthTupleType) { |
There was a problem hiding this comment.
For a Tuple[int, ...] this will print as int[], which is indistinguishable from a list of ints. That may cause potential round-trip issues when serializing schemas (where a VariableLengthTuple accidentally gets casted to a List because the parser can't tell the difference). It will also confuse IRParser, which parses a IR graph string back into an actual Graph object.
We will probably need to define a way to represent a variable length tuple in the schema language as well, and use that here.
| } | ||
| } | ||
|
|
||
| #if 0 |
| # self.assertTrue(set(['aten::add.Tensor', 'aten::mul.Scalar']).issubset( | ||
| # set(torch.jit.export_opnames(scripted_M_mod)))) | ||
|
|
||
| def test_homogeneous_tuple(self): |
There was a problem hiding this comment.
We should have more tests here:
- Tests of the functionality of VariableLengthTuples, and that they actually work when you run the function and compare the results to a pure Python implementation.
- Tests of serialization/deserialization (use a VariableLengthTuples as an attribute to a module and make sure it still works when coming out the other side).
- Tests of various implicit conversion behaviors that we want to work, as well as subtyping behavior.
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
Alias homogenous tuples to lists in jit frontend parser
Homogenous tuple is defined a
Tuple[Type, ...]and semantically very similar toList[Type]This would allow one to use
torch.nn.common_types._size_any_tto type annotate jit functions