[Relay][Frontend] Span filling common API #13402

chunit-quic · 2022-11-16T06:58:45Z

Expose and add span attribute of Expr-derived types from C++ to Python
Add common API of span filling
Add test cases of span filling
Add function to control whether to fill span via environment variable

tvm-bot · 2022-11-16T06:58:48Z

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}
Built docs for commit 4f0e02e can be found here.

_{Generated by tvm-bot}

chunit-quic · 2022-11-16T07:01:52Z

Hi community,

This is the very first PR of the RFC, TVM Explorer Infrastructure. We spend a while to extract the code blocks, and verify them after rebasing. Feel free to give us any comment. Thank you! :D

For your reference, here is the related links.
PreRFC in forum
RFC in git
Tracking issue

@haowhsu-quic, @zack-ch

echuraev

I have read the RFC about TVM Exprorer. It looks awesome! Thank you for your contribution. Added few minor comments.

echuraev · 2022-11-16T07:34:33Z

python/tvm/relay/frontend/common.py

+
+
+def _should_fill_span():
+    should_fill_span = os.environ.get("TVM_SPANFILLING", "1")


Will we document somewhere how to use span filling and describe variable TVM_SPANFILLING?
At least, probably we should add information about this variable to the set_span method?

Thank you for point out. :D
I have pushed a patch with documentation below set_span(). This document includes environment variable setting, how to use and example.

python/tvm/relay/frontend/common.py

chunit-quic · 2022-11-17T05:52:49Z

@FrozenGene @areusch @mbs-octoml
Tag reviewers in the forum thread and previously reverted PR.
It would be a big help to have your advices. Thank you! :D

echuraev

LGTM. Thanks

areusch

thanks @chunit-quic and sorry for the delay--i've been out on PTO for a while. i've left some comments here, overall this is a great addition!

areusch · 2022-11-22T14:58:03Z

python/tvm/testing/utils.py

+
+class _control_span_filling:
+    def __init__(self, on=True):
+        self._old_state = os.environ["TVM_SPANFILLING"] if "TVM_SPANFILLING" in os.environ else None


just curious why you are consulting os.envrion here?

Because we can command like the following way to get rid of span without modifying the code. It would be a bit more convenient.

TVM_SPANFILLING=0 python ${your_program.py}

right--sorry perhaps I should have commented on _should_fill_span in python/tvm/relay/frontend/common.py. what I mean is--could we consider a global variable or a PassContext option which can be initialized from the env should a driver script want, rather than forcing this control to be in the environment?

No problem, I change the way from using the environment variable to the passContext config. :)

src/relay/ir/expr.cc

tests/python/frontend/test_common.py

areusch · 2022-11-22T15:07:17Z

tests/python/relay/utils/tag_span.py

+from tvm.relay.expr_functor import ExprVisitor
+
+
+def _set_span(expr, src):


we have an analogue WithFields in the C++, I think this would be better placed there. for now, see FunctionWithFields.

Thank you for let us know that we can leverage this function. :D
Yet about this _set_span, it is just a simple helper function and only used in the tests. Would you hope to add the functions like CallWithFields or TupleWithFields in C++ and expose them to python side in this patch? Or perhaps this _set_span is enough for the test cases?

i think if you're going to do something like this, it would be great to just add e.g. CallWithFields and TupleWithFields properly to the FFI now. no need to migrate any other code, but keeping this code in test here seems like it's asking for two implementations to surface. lmk if this is too much of a burden.

Sure thing! I have exposed them from C++ and change the functions.

tests/python/relay/utils/tag_span.py

areusch · 2022-11-22T15:11:17Z

python/tvm/relay/frontend/common.py

@@ -997,3 +1002,167 @@ def try_resolve_var_to_const(x, graph_params):
        return _op.const(value, dtype)

    return x
+
+
+class _SpanFiller(ExprMutator):


we had been implementing a bunch of passes in C++, since the Python visitor infra involves a lot of calls that cross the FFI. I wonder if you'd be up for moving this into C++?

I see. Currently we don't have plan to migrate them to a C++ pass. There are two reasons.

First of all, considering the profiling result in the preRFC, the frontend conversion time is not increased too much.

Second, the fill() and __init__() function heavily interact with different frontends. We would like to keep the flexibility to modify them via python script quickly. For example, in the min_max_common conversion of PyTorch, it might end up being a None, which is not common in other frontends.

However, if the performance of frontend conversion is significantly decreased by SpanFiller in some cases, we can arrange a patch to move the mutator part (visitor funcions) to C++ pass. :)

ok, given for now we want to invoke this after the import, we can keep it as Python for now

- Expose and add span attribute of Expr-derived types from C++ to Python - Add common API of span filling - Add test cases of span filling - Add function to control whether to fill span via environment variable - Modify the way of pretty-print to print span

- Change based on comment - Discard the change of pretty-print - Add document to set_span

- Change the test cases to pytest style - Group the set_span test cases to a class

chunit-quic · 2022-11-24T01:53:36Z

thanks @chunit-quic and sorry for the delay--i've been out on PTO for a while. i've left some comments here, overall this is a great addition!

Hi @areusch,
Thank you for reviewing!
We have modified several parts based on your previous comments. :D
Take your time (It's thanks giving recently.) We will wait for your reply.

areusch

sorry for the delay @chunit-quic , one more question here

src/relay/ir/expr.cc

areusch

sorry for the long delay here! i just have a couple of actionable comments, but otherwise I'm good with this PR landing.

areusch · 2022-12-09T23:21:00Z

tests/python/relay/utils/tag_span.py

+from tvm.relay.expr_functor import ExprVisitor
+
+
+def _set_span(expr, src):


i think if you're going to do something like this, it would be great to just add e.g. CallWithFields and TupleWithFields properly to the FFI now. no need to migrate any other code, but keeping this code in test here seems like it's asking for two implementations to surface. lmk if this is too much of a burden.

areusch · 2022-12-09T23:23:15Z

python/tvm/relay/frontend/common.py

@@ -997,3 +1002,167 @@ def try_resolve_var_to_const(x, graph_params):
        return _op.const(value, dtype)

    return x
+
+
+class _SpanFiller(ExprMutator):


ok, given for now we want to invoke this after the import, we can keep it as Python for now

areusch · 2022-12-09T23:23:25Z

python/tvm/relay/frontend/common.py

+    def visit_function(self, fn):
+        new_params = [self.visit(x) for x in fn.params]
+        new_body = self.visit(fn.body)
+        return _function.Function(


want to use FunctionWithFields here too? (and similar below)

Yes, I have changed them in the latest patch.

areusch · 2022-12-09T23:25:16Z

python/tvm/testing/utils.py

+
+class _control_span_filling:
+    def __init__(self, on=True):
+        self._old_state = os.environ["TVM_SPANFILLING"] if "TVM_SPANFILLING" in os.environ else None


right--sorry perhaps I should have commented on _should_fill_span in python/tvm/relay/frontend/common.py. what I mean is--could we consider a global variable or a PassContext option which can be initialized from the env should a driver script want, rather than forcing this control to be in the environment?

- Expose Relay Expr WithFields APIs to python side - Change the APIs in _SpanFiller from creating new instance to WithFields. - Change the control of frontend span filler from env var to the passcontext config

areusch

thanks @chunit-quic!

- Expose and add span attribute of Expr-derived types from C++ to Python - Add common API of span filling - Add test cases of span filling - Add function to control whether to fill span via environment variable - Modify the way of pretty-print to print span Co-authored-by: Joey Tsai <chunit@qti.qualcomm.com>

echuraev reviewed Nov 16, 2022

View reviewed changes

echuraev approved these changes Nov 17, 2022

View reviewed changes

areusch reviewed Nov 22, 2022

View reviewed changes

Joey Tsai added 3 commits November 24, 2022 09:44

[SpanFillingCommonAPI]

556ec6b

- Change based on comment - Discard the change of pretty-print - Add document to set_span

[SpanFillingCommonAPI]

3a873d9

- Change the test cases to pytest style - Group the set_span test cases to a class

chunit-quic force-pushed the SpanFillingCommonAPI branch from 6ac7339 to 3a873d9 Compare November 24, 2022 01:45

lhutton1 mentioned this pull request Dec 1, 2022

[TVMC][microNPU] tvmc option for printing which operators are offloaded to Ethos-U #13212

Merged

areusch reviewed Dec 2, 2022

View reviewed changes

src/relay/ir/expr.cc Show resolved Hide resolved

areusch reviewed Dec 9, 2022

View reviewed changes

[SpanFillingCommonAPI]

5a2cfd8

- Expose Relay Expr WithFields APIs to python side - Change the APIs in _SpanFiller from creating new instance to WithFields. - Change the control of frontend span filler from env var to the passcontext config

chunit-quic force-pushed the SpanFillingCommonAPI branch from 4f0e02e to 5a2cfd8 Compare December 13, 2022 09:03

areusch approved these changes Dec 27, 2022

View reviewed changes

areusch merged commit 520f2c5 into apache:main Dec 27, 2022

ysh329 mentioned this pull request Apr 17, 2023

[Release] v0.12.0 Release Candidate Notes #14645

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Relay][Frontend] Span filling common API #13402

[Relay][Frontend] Span filling common API #13402

chunit-quic commented Nov 16, 2022 •

edited

Loading

tvm-bot commented Nov 16, 2022 •

edited

Loading

chunit-quic commented Nov 16, 2022 •

edited

Loading

echuraev left a comment

echuraev Nov 16, 2022

chunit-quic Nov 17, 2022

chunit-quic commented Nov 17, 2022

echuraev left a comment

areusch left a comment

areusch Nov 22, 2022

chunit-quic Nov 23, 2022

areusch Dec 9, 2022

chunit-quic Dec 13, 2022

areusch Nov 22, 2022

chunit-quic Nov 23, 2022

areusch Dec 9, 2022

chunit-quic Dec 13, 2022

areusch Nov 22, 2022

chunit-quic Nov 23, 2022

areusch Dec 9, 2022

chunit-quic commented Nov 24, 2022

areusch left a comment

areusch left a comment

areusch Dec 9, 2022

areusch Dec 9, 2022

areusch Dec 9, 2022

chunit-quic Dec 13, 2022

areusch Dec 9, 2022

areusch left a comment



		def _should_fill_span():
		should_fill_span = os.environ.get("TVM_SPANFILLING", "1")

		from tvm.relay.expr_functor import ExprVisitor


		def _set_span(expr, src):

[Relay][Frontend] Span filling common API #13402

[Relay][Frontend] Span filling common API #13402

Conversation

chunit-quic commented Nov 16, 2022 • edited Loading

tvm-bot commented Nov 16, 2022 • edited Loading

chunit-quic commented Nov 16, 2022 • edited Loading

echuraev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunit-quic commented Nov 17, 2022

echuraev left a comment

Choose a reason for hiding this comment

areusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunit-quic commented Nov 24, 2022

areusch left a comment

Choose a reason for hiding this comment

areusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

areusch left a comment

Choose a reason for hiding this comment

chunit-quic commented Nov 16, 2022 •

edited

Loading

tvm-bot commented Nov 16, 2022 •

edited

Loading

chunit-quic commented Nov 16, 2022 •

edited

Loading