Skip to content

[Runtime][PipelineExecutor] Move graph_split into relay testing.#11405

Closed
huajsj wants to merge 3 commits intoapache:mainfrom
huajsj:testing
Closed

[Runtime][PipelineExecutor] Move graph_split into relay testing.#11405
huajsj wants to merge 3 commits intoapache:mainfrom
huajsj:testing

Conversation

@huajsj
Copy link
Contributor

@huajsj huajsj commented May 21, 2022

RFC: https://github.com/apache/tvm-rfcs/blob/main/rfcs/0014-pipeline-executor.md
Issue: #8596

Currently the helper function of pipeline executor unit test like 'graph_slit' stay in the unit test file, and such function not directly related the testing logic.

moving such helper function into tvm.relay.testing folder.

huajsj added 2 commits May 20, 2022 21:13
Currently the helper function of pipeline executor unit test like
'graph_slit' stay in the unit test file, and such function not directly
related the testing logic.

moving such helper function into tvm.relay.testing folder.
@masahi
Copy link
Member

masahi commented May 23, 2022

I think it's better to keep this function in a pipeline executor test file. Things under relay.testing are supposed to be generally useful. And you need to add documentation and improve code quality if you want others to use this code.

@huajsj
Copy link
Contributor Author

huajsj commented May 23, 2022

I think it's better to keep this function in a pipeline executor test file. Things under relay.testing are supposed to be generally useful. And you need to add documentation and improve code quality if you want others to use this code.

@masahi, thanks for the follow up, I do see some requirement for a solution to handle graph splitting for example in this discussion(https://discuss.tvm.apache.org/t/relay-generate-sub-graphs-from-existing-graph/11594), as a short term solution, this graph_split function can serve as an example and a python level support, I think that should be useful to separate this function into 'relay.testing' to let others use it, for sure I will add documentation and polish the code, please kindly let me know how you think.

@rebel-shshin
Copy link
Contributor

rebel-shshin commented May 24, 2022

Hi, @huajsj. Thanks for your contribution on TVM. I have a simple question for the graph_split func. In many real use case, the relay function can be existed in the module['main'] (i.e. OP fusion or BYOC cases). How can we handle these cases to split the graph?

@huajsj
Copy link
Contributor Author

huajsj commented May 24, 2022

Hi, @huajsj. Thanks for your contribution on TVM. I have a simple question for the graph_split func. In many real use case, the relay function can be existed in the module['main'] (i.e. OP fusion or BYOC cases). How can we handle these cases to split the graph?

As line 116 shown, graph_split only parse the top level function and treat the internal "relay.function" as a "relay.expr.let", that means the operator inside the function will not get involved by the graph split, and not impact the result. then either fuse or byoc logic will not get broken.
And if there is a requirement to split the internal function of like BYOC, then the order should be that do the graph split first then do the BYOC annotation.

@rebel-shshin
Copy link
Contributor

Thanks.

Hi, @huajsj. Thanks for your contribution on TVM. I have a simple question for the graph_split func. In many real use case, the relay function can be existed in the module['main'] (i.e. OP fusion or BYOC cases). How can we handle these cases to split the graph?

As line 116 shown, graph_split only parse the top level function and treat the internal "relay.function" as a "relay.expr.let", that means the operator inside the function will not get involved by the graph split, and not impact the result. then either fuse or byoc logic will not get broken. And if there is a requirement to split the internal function of like BYOC, then the order should be that do the graph split first then do the BYOC annotation.

Thanks :-) 👍

@huajsj
Copy link
Contributor Author

huajsj commented May 25, 2022

@masahi , the document adding and code polish already done, please take a look.

%1 = nn.multiply(%data, meta[relay.Constant][0])

the split_config should like following
split_config = [{"operator_name":"add", "operator_index": 1}]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this way of specifying only works for a graph that is a linear-chain. Being limited is ok for a one-off function in a test code, but for more general utilities I think we want a better implementation. So I still don't support moving this function under relay/testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I will close this PR and work for a more complete solution.

@huajsj huajsj closed this May 25, 2022
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.

3 participants