-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there.
* 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>
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, " |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, of course.
assert len(sut[1]) == expected_number_of_results_per_line | ||
|
||
|
||
def test_run_tflite_module__with_profile__valid_input( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
Thanks @leandron @FrozenGene @zhiics @mbaret @tqchen |
* [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>
* [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>
* [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>
* [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>
* [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>
* [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>
* [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>
* [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>
Introduces the
tvmc run
subcommand ontvmc
, the aims to enable execution of compiled modules usingtvmc compile
:tvmc tune
in a future PR)cc @comaniac @FrozenGene for reviews