[TF][Relay] TensorFlow Frontend support with shared params #5042
Conversation
|
yes, this is an issue I too see recently while working on TF 2.0 upgrade. Few suggestions on this. |
| @@ -77,10 +77,21 @@ def _dim_check(attrs): | |||
| return False | |||
| return _dim_check, "Only 2d or 3d kernel supported." | |||
|
|
|||
| # Build count_used to record params have been converted to attrs | |||
| used_params = {} | |||
There was a problem hiding this comment.
Agree with @srkreddy1238 if relay build can remove unused params, we don't need this here.
There was a problem hiding this comment.
Thank you for the suggestions! Then I think the thing to do is simple and we can just remove the pop operations.
There was a problem hiding this comment.
Hi, I have already modified the code according to your suggestions! Thank you for the help!
Thank you for the suggestions! I can try it then! |
| mod, params = relay.frontend.from_tensorflow(constant_graph, | ||
| outputs=['output']) | ||
| with relay.build_config(opt_level=3): | ||
| graph, lib, params = relay.build(mod, |
There was a problem hiding this comment.
You may use the same test case to verify the relay behaviour dropping unused params if frontend just ignores pop. In this case final params after relay build shouldn't contain param '''axis'''
There was a problem hiding this comment.
Yeah! I think the axis will be removed then. Thank you for the information!
There was a problem hiding this comment.
Hi, I have already modified the code according to both suggestions from you two! Thank you so much!
There was a problem hiding this comment.
Hi I have already done the changes as your suggestions! Could you please update the status of this pr so that it could be merged? Thank you so much!
lfengad
force-pushed
the
master
branch
3 times, most recently
from
8e0f034 to
824f930
Compare
|
ping @srkreddy1238 Please review it again. If approve, we should follow the instruction to set the status: https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly |
|
ping @srkreddy1238 |
FrozenGene
dismissed
srkreddy1238’s stale review
Has waited 7 days and ping 2 times. The reviewer's review has been solved
|
Thanks @lfengad's pr and @srkreddy1238 @kevinthesun 's review. Considering @srkreddy1238 's review comment is fixed and wait @srkreddy1238 7 days / ping two times, additionally two reviewers have approved, so let us merge it firstly. If @srkreddy1238 has other comments, we could update or revert this pr easily. |
|
Sorry @lfengad. Was a bit disconnected few days over pandemic situations. |
We observe that in lots of tensorflow models used in our production environment, some operators share the same node as their attributes. For example, there might be two "tf.reduce_mean" operators each with its own "axis" attribute. Their "axis" attributes can take the same "tf.constant" node. In this case, different operators share the same node as attributes.
However, in the current tensorflow frontend of relay, such sharing of the same param among different operators is not supported. For example, in the current implementation of "_get_param" function in "python/tvm/relay/frontend/tensorflow.py", the corresponding param will be deleted permanently using "pop" from the "_params" dictionary after being checked in "_get_param" function. In this case, if another operator needs to check the same param as its attribute, that param will not be found. Hence, the current implementation of the tensorflow frontend cannot support param sharing among different operators.
In order to support the case of param sharing among operators, we modify the tensorflow frontend. We leverage on the python decorator method. Everytime "_get_param" is called, instead of deleting the corresponding visited param as in the current implementation, we record it into a "used_params" dictionary and keep it in the original "_params" dictionary, so that another operator can still visit it in the following process. Then after the whole relay graph completes building, we delete the params have been visited in "used_params" from "_params" to generate the final "_params" output.
Compared to the old implementation ,our new implementation doesn't delete the visited params immediately in "_get_param", but records them and deletes them at the final step. In this way, the case where different operators share the same param can be supported.
Any suggestions are welcome! @tqchen @FrozenGene