Skip to content
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

[Relay][Frontend] Span Filling TensorFlow 1 #13728

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

chunit-quic
Copy link
Contributor

  • Set node name as the source name of span during the conversion of Tensorflow1 model
  • Add structural_equal comparison with and without set_span to the existing test cases.
  • Add span test cases for frequent conversions.

- Set node name as the source name of span during the conversion of
  Tensorflow1 model
- Add structural_equal comparison with and without set_span to the
  existing test cases.
- Add span test cases for frequent conversions.
@tvm-bot
Copy link
Collaborator

tvm-bot commented Jan 9, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

  • No users to tag found in teams: relay, frontend See #10317 for details

Generated by tvm-bot

@chunit-quic
Copy link
Contributor Author

Hi community,
This is a part of the RFC, TVM Explorer Infrastructure. After this PR, the source layer information of Tensorflow1 will be tagged to the converted relay. Feel free to give us any comment. Thank you! :D

For your reference, here is the related links.
PreRFC in forum
apache/tvm-rfcs#92
#13116
@haowhsu-quic, @zack-ch

@masahi masahi merged commit b2da945 into apache:main Jan 9, 2023
fzi-peccia pushed a commit to fzi-peccia/tvm that referenced this pull request Mar 27, 2023
- Set node name as the source name of span during the conversion of
  Tensorflow1 model
- Add structural_equal comparison with and without set_span to the
  existing test cases.
- Add span test cases for frequent conversions.

Co-authored-by: Joey Tsai <chunit@qti.qualcomm.com>
@xiaolong18
Copy link
Contributor

@chunit-quic Thank you for your work,I want to ask you a question, I tested the tests/python/frontend/tensorflow/test_forward.py::test_forward_dynmaic_rnn_lstmblockcell and report error , I found that unnecessary let nodes will be generated after turning on set_span, It is difficult for me to debug and locate the reason why set_span will change the structure of relay ir .
Since the tensorflow version in ci docker is relatively new, so in ci test this test case is skipped, If you have time to take a look and run this test case whith older tensorflow version , I will be very grateful.thanks

@chunit-quic
Copy link
Contributor Author

Hi @xiaolong18

Thank you for your information. Currently we are doing some urgent projects, and perhaps we can only get back to here at the end of October.

But we might give you some hints about this problem. we saw some duplicated ops are generated in the pytorch frontend before.
It is caused by a frontend operator which is converted to multiple exprs, and one of these expressions (let's say p expr) are comsumed by multiple users(u1, u2). When span filler recursively traverse back, it construct a set of span-included exprs from u1 expr until no expr or encountering a span-filled expr. After traversed u1, a span-filled p is constructed (sf-p). However, when spanfiller traverses u2, it still found its parent is p, rather than sf-p. Then a set of duplicated exprs are generated unfortunately.

We handle this kind of problems via invoke set_span directy inside this kind of op conversions, like the NMS operator in pyotorch. Perhaps it could be a reference to you.

Hope it could give you clues to do some more investigation. Please feel free to ping us, yet it might take some time for us to get back to you. :D

Thank,
Joey

@xiaolong18
Copy link
Contributor

@chunit-quic I tried to build a test case and discovered this problem. as you say , I feel that the main reason for this problem is that when multiple consumers consume the same provider, the consumers are directly unaware of each other, so they may build the provider repeatedly.
So for the processing of any node, the span should be filled quickly after it is parsed into relay ir. In this way, since the span has been filled, no consumer can rewrite this provider. is that right?,Or can we use a global member in _Spanfilter to store the expr that already has span, and then take it out directly from it?

I imitated ci's test case and built an example with a similar structure:
x = set_span(relay.Var("x"), "x_var")
param_1 = realy.Var("x_1")
param_2 = realy.Var("x_2")
param_a = relay.Var("x_a")
param_b = realy.Var("x_b")
f_add = relay.add(param_1, param_2)
f_sub = relay.subtract(param_1, param_2)
f_re = relay.Tuple([f_add, f_sub])
f = relay.Function([param_1, param_2], f_re)
let = relay.Let(x,f , x(param_a, param_b))
let_0 = realy.TupleGetItem(let, 0)
let_1 = relay.TupleGetItem(let,1)

let_0_relu = relay.nn.relu(let, 0)
final_old =relay.Tuple([let_0_relu, let_1])
print(final_old)
assert relay.analysis.well_formed(final_old)

let_0_relu_new =set_span(relay.nn.relu(let_0), "aa")
final_new = set_span(relay.Tuple([let_0_relu_new, let_1]), "bb")
print(final_new)
assert relay.analysis.well_formed(final_new)

Thanks!

@chunit-quic
Copy link
Contributor Author

Hi @xiaolong18
Sorry for late reply. We finally can get back to here now.

the span should be filled quickly after it is parsed into relay ir. In this way, since the span has been filled, no consumer can rewrite this provider. is that right?

Yes, that’s exactly how we handle this kind of problem. We will recommend fixing it this way.

the span should be filled quickly after it is parsed into relay ir. In this way, since the span has been filled, no consumer can rewrite this provider. is that right?

Somehow it should be fine. Yet it might not be a good choice. Generally, using global variable is not desirable. Besides, the map cannot be declared in _SpanFiller, because lifecycle of _SpanFiller is within the scope of function set_span. That’s why we use the first solution to tackle this problem.

Thanks,
Joey

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants