-
Notifications
You must be signed in to change notification settings - Fork 683
Update default executor runner with new optional options #14017
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
Conversation
In the executor runner all inputs get hard coded to ones. Add optional input option, in which case tensor inputs will be written from supplied binary input files. Also update the Arm VKML runner unit test runner as a user with real inputs. GenAI used, Blackduck Scan OK Change-Id: Ie0363e04f0bbcb2342781f3c560ac30837d88a31
By default not all output is printed. Adds option for printing all output. Also adds option to print output to file. Also update the Arm VKML unit test runner as a user that prints output to file. Enables acos_unit test to run on Vulkan runtime that depends on this. Change-Id: If61c1fe89c9da004fa9db4524e1413893549abce
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14017
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit 943493b with merge base 3c533aa ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
The file was added by mistake.
Hi @digantdesai this is touching stuff outside Arm backend and we need help reviewing it. |
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.
LGTM, seems like BC changes for the runner and the extn/utils, make sure the CI is green.
run_on_vulkan_runtime=True, | ||
) | ||
pipeline.run() | ||
try: |
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.
Nit: can we do it like this -
class TOSAPipelineMaker(BasePipelineMaker, Generic[T]): |
Else I imagine we have to write this for every single test..
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.
Yes indeed we shall not do a try/except in every unit test. Just wanted to enable a test that tested this. We will either do a central try/except or add xfails for remaining tests in coming patches.
model_path, | ||
"model.pte", | ||
"Model serialized in flatbuffer format."); | ||
DEFINE_string(inputs, "", "Comma-separated list of input files"); |
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.
JFYI you can also use bundled-io in the PTE.
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.
Yes, perhaps add that in a separate patch
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.
Outputs can also be saved with ETDump but that is maybe a bigger/separate patch this file should probably be re-synced with examples/devtools/example_runner/example_runner.cpp someday.
Or @digantdesai do you want to avoid adding "inputs" as an argument and only handle it via BundleIO?
E.g. is this something you prefere us to fix now in this PR and later after 1.0/GA?
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.
Just spoted your "LGTM" so that answers my question :)
Test fails are unrelated. |
We are pausing the merge a short bit, we want to make sure the testing also work if VGF tools/VulkanML is installed, and it currently isn't yet in the GitHub testing. But will be in a separate PR. UPDATE: should be merged at the same time / together with #14116 |
)" This reverts commit 16aac24.
…4193) Reverts #14017 https://www.internalfb.com/diff/D82053273 Here's the error from internal failure ``` executorch/extension/runner_util/inputs.cpp:118:13: error: format specifies type 'long' but the argument has type 'std::tuple_element<1, std::pair<char *, unsigned int>>::type' (aka 'unsigned int') [-Werror,-Wformat] [CONTEXT] 117 | "input size (%ld) and tensor size (%ld) mismatch!", [CONTEXT] | ~~~ xplat/executorch/extension/runner_util/inputs.cpp:119:13: error: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Werror,-Wformat] [CONTEXT] 117 | "input size (%ld) and tensor size (%ld) mismatch!", [CONTEXT] | ~~~ xplat/executorch/extension/runner_util/inputs.cpp:118:13: error: format specifies type 'long' but the argument has type 'std::tuple_element<1, std::pair<char *, unsigned int>>::type' (aka 'unsigned int') [-Werror,-Wformat] [CONTEXT] 117 | "input size (%ld) and tensor size (%ld) mismatch!", [CONTEXT] | ~~~ xplat/executorch/extension/runner_util/inputs.cpp:119:13: error: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Werror,-Wformat] [CONTEXT] 117 | "input size (%ld) and tensor size (%ld) mismatch!", [CONTEXT] | executorch/extension/runner_util/inputs.cpp:118:13: error: format specifies type 'long' but the argument has type 'std::tuple_element<1, std::pair<char *, unsigned int>>::type' (aka 'unsigned int') [-Werror,-Wformat] [CONTEXT] 117 | "input size (%ld) and tensor size (%ld) mismatch!", [CONTEXT] | ~~~ [CONTEXT] | %u [CONTEXT] 118 | buffer_size, [CONTEXT] | ^~~~~~~~~~~ [CONTEXT] xplat/executorch/runtime/platform/log.h:181:13: note: expanded from macro 'ET_LOG' [CONTEXT] 180 | _format, \ [CONTEXT] | ~~~~~~~ [CONTEXT] 181 | ##__VA_ARGS__); \ [CONTEXT] | ^~~~~~~~~~~ xplat/executorch/extension/runner_util/inputs.cpp:119:13: error: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Werror,-Wformat] [CONTEXT] 117 | "input size (%ld) and tensor size (%ld) mismatch!", [CONTEXT] | ~~~ [CONTEXT] | %zu [CONTEXT] 118 | buffer_size, [CONTEXT] 119 | tensor_meta->nbytes()); [CONTEXT] | ^~~~~~~~~~~~~~~~~~~~~ [CONTEXT] xplat/executorch/runtime/platform/log.h:181:13: note: expanded from macro 'ET_LOG' [CONTEXT] 180 | _format, \ [CONTEXT] | ~~~~~~~ [CONTEXT] 181 | ##__VA_ARGS__); \ [CONTEXT] | ^~~~~~~~~~~ [CONTEXT] 2 errors generated. ```
…ytorch#14193) This reverts commit dc190f9. Original PR, pytorch#14017 was reverted by pytorch#14193. This reverts the revert. The only difference from the original PR is fixing a printf format mismatch in inputs.cpp: %ld is changed to %zu. This should compile on both 32- and 64-bit ABIs. Change-Id: If487e6c6bde313844a94db99a7431af33dfcdd0a
…torch#14193) Reverts pytorch#14017 https://www.internalfb.com/diff/D82053273 Here's the error from internal failure ``` executorch/extension/runner_util/inputs.cpp:118:13: error: format specifies type 'long' but the argument has type 'std::tuple_element<1, std::pair<char *, unsigned int>>::type' (aka 'unsigned int') [-Werror,-Wformat] [CONTEXT] 117 | "input size (%ld) and tensor size (%ld) mismatch!", [CONTEXT] | ~~~ xplat/executorch/extension/runner_util/inputs.cpp:119:13: error: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Werror,-Wformat] [CONTEXT] 117 | "input size (%ld) and tensor size (%ld) mismatch!", [CONTEXT] | ~~~ xplat/executorch/extension/runner_util/inputs.cpp:118:13: error: format specifies type 'long' but the argument has type 'std::tuple_element<1, std::pair<char *, unsigned int>>::type' (aka 'unsigned int') [-Werror,-Wformat] [CONTEXT] 117 | "input size (%ld) and tensor size (%ld) mismatch!", [CONTEXT] | ~~~ xplat/executorch/extension/runner_util/inputs.cpp:119:13: error: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Werror,-Wformat] [CONTEXT] 117 | "input size (%ld) and tensor size (%ld) mismatch!", [CONTEXT] | executorch/extension/runner_util/inputs.cpp:118:13: error: format specifies type 'long' but the argument has type 'std::tuple_element<1, std::pair<char *, unsigned int>>::type' (aka 'unsigned int') [-Werror,-Wformat] [CONTEXT] 117 | "input size (%ld) and tensor size (%ld) mismatch!", [CONTEXT] | ~~~ [CONTEXT] | %u [CONTEXT] 118 | buffer_size, [CONTEXT] | ^~~~~~~~~~~ [CONTEXT] xplat/executorch/runtime/platform/log.h:181:13: note: expanded from macro 'ET_LOG' [CONTEXT] 180 | _format, \ [CONTEXT] | ~~~~~~~ [CONTEXT] 181 | ##__VA_ARGS__); \ [CONTEXT] | ^~~~~~~~~~~ xplat/executorch/extension/runner_util/inputs.cpp:119:13: error: format specifies type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Werror,-Wformat] [CONTEXT] 117 | "input size (%ld) and tensor size (%ld) mismatch!", [CONTEXT] | ~~~ [CONTEXT] | %zu [CONTEXT] 118 | buffer_size, [CONTEXT] 119 | tensor_meta->nbytes()); [CONTEXT] | ^~~~~~~~~~~~~~~~~~~~~ [CONTEXT] xplat/executorch/runtime/platform/log.h:181:13: note: expanded from macro 'ET_LOG' [CONTEXT] 180 | _format, \ [CONTEXT] | ~~~~~~~ [CONTEXT] 181 | ##__VA_ARGS__); \ [CONTEXT] | ^~~~~~~~~~~ [CONTEXT] 2 errors generated. ```
For Arm backend update the Arm VKML unit test runner as a user with real inputs and file output. Enables add/acos_unit tests to run on Vulkan runtime that depend on this.
cc @digantdesai @freddan80 @per @zingo @oscarandersson8218