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

[tvmc] Introduce 'run' subcommand (part 4/4) #6578

Merged
merged 7 commits into from
Oct 2, 2020
Merged

[tvmc] Introduce 'run' subcommand (part 4/4) #6578

merged 7 commits into from
Oct 2, 2020

Conversation

leandron
Copy link
Contributor

@leandron leandron commented Sep 28, 2020

Introduces the tvmc run subcommand on tvmc, the aims to enable execution of compiled modules using tvmc compile:

  • Include options to locally or remotelly using RPC
  • Include support to cpu and gpu devices
  • Introduces a new function to parse RPC command line strings (will be backported to tvmc tune in a future PR)

cc @comaniac @FrozenGene for reviews

python/tvm/driver/tvmc/common.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/common.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Outdated Show resolved Hide resolved
tests/python/driver/tvmc/test_runner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Show resolved Hide resolved
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

Almost there.

python/tvm/driver/tvmc/runner.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Show resolved Hide resolved
tests/python/driver/tvmc/conftest.py Show resolved Hide resolved
tests/python/driver/tvmc/test_runner.py Outdated Show resolved Hide resolved
leandron and others added 5 commits October 1, 2020 10:57
 * Add 'tvmc run' subcommand to execute compiled modules
 * Include options to locally or remotelly using RPC
 * Include support to cpu and gpu devices


Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>
@leandron
Copy link
Contributor Author

leandron commented Oct 1, 2020

something went wrong with CI. Retriggering it now

action="store_true",
help="generate profiling data from the runtime execution. "
"Using --profile requires the Graph Runtime Debug enabled on TVM. "
"Profiling may also have an impact on inference time, "
Copy link
Member

Choose a reason for hiding this comment

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

I am a little curious about this sentence. As far as I know, debug graph runtime is just to record op execution time, no other debug flags to add to build tvm(like -g). So how does it affect the inference time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Even running debug runtime would affect the inference time compared to the graph runtime.

Copy link
Contributor Author

@leandron leandron Oct 1, 2020

Choose a reason for hiding this comment

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

That is a good question. @comaniac can you comment? Maybe this needs some rephrasing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm just saying the overhead of collecting/dumping op execution time.

Copy link
Member

Choose a reason for hiding this comment

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

However, this overhead shouldn’t affect the inferece time in my opinion. Because we record this using chrono, when we dump, the time is recorded completely and shouldn’t affect our record.

If we want to describle the overhead, maybe we shouldn’t use the inference time and to say we will introduce the overload time of collecting / dumping which will make our pipiline longer than before.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this CLI is facing to end-users more than the developers, so I supposed they care the end-to-end time more than just the inference time. However, I agree with you that rephrasing the description to avoid inference time might be more accurate.

OHTH, given that this PR is in the list of 0.7 release, would you mind letting the PR in first and fix this issue in the follow-up PR along with other TODOs, since the CI takes a long time?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, of course.

python/tvm/driver/tvmc/runner.py Show resolved Hide resolved
assert len(sut[1]) == expected_number_of_results_per_line


def test_run_tflite_module__with_profile__valid_input(
Copy link
Member

Choose a reason for hiding this comment

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

why there are many function names using double underscore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used on other test cases, mostly to mark separation of sentences. I can revert it back in case it looks too wrong, but for me they are very useful when reading it in a long list of tests with similar names and slightly different conditions.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest we keeping the style like all other test. Current function names are like generated by automatic tools. How about this suggestion?

Copy link
Member

@FrozenGene FrozenGene left a comment

Choose a reason for hiding this comment

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

Overal LGTM. But we should complete other TODOs in the following prs as this tool is very critial and maybe the most important end userface developers use TVM in the future.

@comaniac comaniac merged commit 5db80f0 into apache:master Oct 2, 2020
@comaniac
Copy link
Contributor

comaniac commented Oct 2, 2020

@leandron leandron deleted the tvmc_run_feature branch October 7, 2020 12:56
TusharKanekiDey pushed a commit to TusharKanekiDey/tvm that referenced this pull request Oct 13, 2020
* [tvmc] Introduce 'run' subcommand (part 4/4)

 * Add 'tvmc run' subcommand to execute compiled modules
 * Include options to locally or remotelly using RPC
 * Include support to cpu and gpu devices


Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>

* adjust based on code review comments

* make test fixture to safely skip environments without tflite

* make --help option more clear

* improve error message to show expected inputs

* code-review adjusts

* update doc-string to default zeros->random

Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>
TusharKanekiDey pushed a commit to TusharKanekiDey/tvm that referenced this pull request Oct 14, 2020
* [tvmc] Introduce 'run' subcommand (part 4/4)

 * Add 'tvmc run' subcommand to execute compiled modules
 * Include options to locally or remotelly using RPC
 * Include support to cpu and gpu devices


Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>

* adjust based on code review comments

* make test fixture to safely skip environments without tflite

* make --help option more clear

* improve error message to show expected inputs

* code-review adjusts

* update doc-string to default zeros->random

Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>
TusharKanekiDey pushed a commit to TusharKanekiDey/tvm that referenced this pull request Oct 15, 2020
* [tvmc] Introduce 'run' subcommand (part 4/4)

 * Add 'tvmc run' subcommand to execute compiled modules
 * Include options to locally or remotelly using RPC
 * Include support to cpu and gpu devices


Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>

* adjust based on code review comments

* make test fixture to safely skip environments without tflite

* make --help option more clear

* improve error message to show expected inputs

* code-review adjusts

* update doc-string to default zeros->random

Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>
TusharKanekiDey pushed a commit to TusharKanekiDey/tvm that referenced this pull request Oct 15, 2020
* [tvmc] Introduce 'run' subcommand (part 4/4)

 * Add 'tvmc run' subcommand to execute compiled modules
 * Include options to locally or remotelly using RPC
 * Include support to cpu and gpu devices


Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>

* adjust based on code review comments

* make test fixture to safely skip environments without tflite

* make --help option more clear

* improve error message to show expected inputs

* code-review adjusts

* update doc-string to default zeros->random

Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>
TusharKanekiDey pushed a commit to TusharKanekiDey/tvm that referenced this pull request Oct 16, 2020
* [tvmc] Introduce 'run' subcommand (part 4/4)

 * Add 'tvmc run' subcommand to execute compiled modules
 * Include options to locally or remotelly using RPC
 * Include support to cpu and gpu devices


Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>

* adjust based on code review comments

* make test fixture to safely skip environments without tflite

* make --help option more clear

* improve error message to show expected inputs

* code-review adjusts

* update doc-string to default zeros->random

Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>
TusharKanekiDey pushed a commit to TusharKanekiDey/tvm that referenced this pull request Oct 16, 2020
* [tvmc] Introduce 'run' subcommand (part 4/4)

 * Add 'tvmc run' subcommand to execute compiled modules
 * Include options to locally or remotelly using RPC
 * Include support to cpu and gpu devices


Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>

* adjust based on code review comments

* make test fixture to safely skip environments without tflite

* make --help option more clear

* improve error message to show expected inputs

* code-review adjusts

* update doc-string to default zeros->random

Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Oct 19, 2020
* [tvmc] Introduce 'run' subcommand (part 4/4)

 * Add 'tvmc run' subcommand to execute compiled modules
 * Include options to locally or remotelly using RPC
 * Include support to cpu and gpu devices


Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>

* adjust based on code review comments

* make test fixture to safely skip environments without tflite

* make --help option more clear

* improve error message to show expected inputs

* code-review adjusts

* update doc-string to default zeros->random

Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Oct 19, 2020
* [tvmc] Introduce 'run' subcommand (part 4/4)

 * Add 'tvmc run' subcommand to execute compiled modules
 * Include options to locally or remotelly using RPC
 * Include support to cpu and gpu devices


Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>

* adjust based on code review comments

* make test fixture to safely skip environments without tflite

* make --help option more clear

* improve error message to show expected inputs

* code-review adjusts

* update doc-string to default zeros->random

Co-authored-by: Marcus Shawcroft <marcus.shawcroft@arm.com>
Co-authored-by: Matthew Barrett <matthew.barrett@arm.com>
@leandron leandron mentioned this pull request Feb 4, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants