Skip to content

[Runtime][PipelineExecutor] Add graph manually splitting logic into the unit test. - #11334

Merged
masahi merged 6 commits into
apache:mainfrom
huajsj:splitexample
May 19, 2022
Merged

[Runtime][PipelineExecutor] Add graph manually splitting logic into the unit test.#11334
masahi merged 6 commits into
apache:mainfrom
huajsj:splitexample

Conversation

@huajsj

@huajsjhuajsj commented May 17, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

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

Current unit test create 3 seperate module then re-connect them to run the pipeline executor. And this is not a real use case for pipeline executor.

Adding a manually graph splitting logic which split a full network into 3 subgraph then run the pipeline executor and verify the result to simulate the real use case .

@huajsjhuajsj changed the title [Runtime][PipelineExecutor] Add graph manually splitting example into the unit test.[Runtime][PipelineExecutor] Add graph manually splitting logic into the unit test.May 17, 2022
@huajsjhuajsj mentioned this pull request May 17, 2022
Closed
15 tasks
SebastianBoblest
SebastianBoblest reviewed May 17, 2022
View reviewed changes
Comment threadtests/python/relay/test_pipeline_executor.py Outdated
@huajsj

huajsj commented May 18, 2022

Copy link
Copy Markdown
ContributorAuthor

@SebastianBoblestETAS , please take a look.

SebastianBoblest
SebastianBoblest reviewed May 18, 2022
View reviewed changes

@SebastianBoblestSebastianBoblest left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just looked for typos and possible code changes on a formal level.
I cannot judge the functionality/correctness of the code.
I did not spot any obvious bugs though, so LGTM as far as I can judge it.

Comment threadtests/python/relay/test_pipeline_executor.py Outdated
Comment threadtests/python/relay/test_pipeline_executor.py Outdated
Comment threadtests/python/relay/test_pipeline_executor.py Outdated
Comment threadtests/python/relay/test_pipeline_executor.py Outdated
Comment threadtests/python/relay/test_pipeline_executor.py Outdated
Comment threadtests/python/relay/test_pipeline_executor.py Outdated
Comment threadtests/python/relay/test_pipeline_executor.py Outdated
Comment threadtests/python/relay/test_pipeline_executor.py Outdated
Comment threadtests/python/relay/test_pipeline_executor.py Outdated
Comment threadtests/python/relay/test_pipeline_executor.py Outdated
@huajsj

huajsj commented May 18, 2022

Copy link
Copy Markdown
ContributorAuthor

@SebastianBoblestETAS, all review comments addressed, please take a look.

SebastianBoblest
SebastianBoblest approved these changes May 19, 2022
View reviewed changes

@SebastianBoblestSebastianBoblest left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, two minor typos should be fixed.
Note that I can not really judge the functionality of this test, not my area of expertise.

Comment threadtests/python/relay/test_pipeline_executor.py
Comment threadtests/python/relay/test_pipeline_executor.py
huajsj added 6 commits May 19, 2022 13:48
@huajsj
[Runtime][PipelineExecutor] Add graph manually splitting example into
7bff440
the unit test.
Current unit test create 3 seperate module then re-connect them to
run the pipeline executor. And this is not a real use case for pipeline
executor.
Adding a manually graph splitting logic which split a full network into 3
subgraph then run the pipeline executor and verify the result to
simulate the real use case.
@huajsj
address review comments
2d055b1
@huajsj
trigger build.
0330073
@huajsj
address review comments
b248165
@huajsj
address review comments
d4e2a18
@huajsj
rebase and trigger build.
fce24f1
@huajsj

huajsj commented May 19, 2022

Copy link
Copy Markdown
ContributorAuthor

@masahi , please take a look.

@masahi
masahi merged commit e02bf82 into apache:mainMay 19, 2022
@driazatidriazati mentioned this pull request Jul 14, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@masahimasahimasahi approved these changes
+1 more reviewer
@SebastianBoblestSebastianBoblestSebastianBoblest approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@huajsj@masahi@SebastianBoblest