[Frontend, Tensorflow2] Added support for TensorList ops #8454
Conversation
Co-authored-by: David Huang <davhuan@amazon.com> Co-authored-by: Rohan Mukherjee <mukrohan@amazon.com> Co-authored-by: Xingyu Zhou <zhoxingy@amazon.com> Co-authored-by: Srinidhi Goud <srinidhi.goud29@gmail.com> Co-authored-by: Xiao <weix@amazon.com>
zxy844288792
force-pushed
the
tensorlist_pr
branch
from
1c5ff10 to
4c73073
Compare
zxy844288792
requested review from
areusch,
icemelon,
jroesch,
junrushao,
merrymercy,
tqchen,
yzhliu and
zhiics
as code owners
zxy844288792
force-pushed
the
tensorlist_pr
branch
from
5d65283 to
ead2151
Compare
zxy844288792
requested review from
Huyuwei,
jwfromm,
kazum,
mbrookhart and
siju-samuel
as code owners
|
Thanks for the pr, I will take a look at it later today. |
| # A map to record tensor list write ops and input tl/tensor indices | ||
| # Value is (index of tensor list, index of written node) | ||
| _tensor_list_write_ops = { | ||
| "TensorListSetItem": (0, 2), |
There was a problem hiding this comment.
is there any reason why you chose 0 and 2 here?
There was a problem hiding this comment.
This is the input tensorlist and tensor indices defined in tensorflow API
| elif cnode.name != stack_node.name: | ||
| if is_tensor_list_constuctor(cnode): | ||
| shape_attr = parse_attr(input_shape_node.attr) | ||
| if "value" not in shape_attr: |
There was a problem hiding this comment.
just for curiosity, for the case 'value' not in shape_attr, does that mean it is a input node without shape?
There was a problem hiding this comment.
yes, then we will treat it as dynamic shape. It will still work as a unit test but in real model, it might cause problem if following by some ops like conv2D
| "TensorListGetItem": _tensorlist_get_item(), | ||
| "TensorListReserve": _tensorlist_reserve(), | ||
| "TensorListSetItem": _tensorlist_set_item(), | ||
| "TensorListStack": _tensorlist_stack(), |
There was a problem hiding this comment.
seems there are several other TensorList related ops like TensorListScatter are not added here, what are the reasons you chose the list here to support?
There was a problem hiding this comment.
The reason why I chose these operators is due to the models I am working on only contain these ops, It will be ongoing efforts for adding new operator support
| ) | ||
| return model | ||
|
|
||
| run_sequential_model(tensorlist_read_model, input_shape=(3, 32)) |
There was a problem hiding this comment.
maybe adding a test case with a public model that contains tensorlist ops
There was a problem hiding this comment.
I have some public models but they will trigger a corner case which I will fix in the next Pr. The TensorList op alone works fine.
zxy844288792
force-pushed
the
tensorlist_pr
branch
from
ead2151 to
c53b599
Compare
|
Thanks @zxy844288792 @yongwww @trevor-m |
This PR adds support for TensorList ops in TF2 frontend parser. Ops like TensorListFromTensor, TensorListGetItem etc are common in graphdef of TF2 OD models like Faster-RCNN etc.
This PR is in line with the original plan of supporting tensorflow2 parser in TVM frontend as in the discussion #4102 Exact commit plan here.
Co-authored-by: David Huang davhuan@amazon.com
Co-authored-by: Rohan Mukherjee mukrohan@amazon.com
Co-authored-by: Xingyu Zhou zhoxingy@amazon.com
Co-authored-by: Srinidhi Goud srinidhi.goud29@gmail.com
Co-authored-by: Xiao weix@amazon.com
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.