Remove false positive error message in the executor_runner #7170
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7170
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit b838755 with merge base c5fea7e ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
ykhrustalev
force-pushed
the
ykhrustalev/false-error
branch
from
12a1b0a to
0225f2f
Compare
There was a problem hiding this comment.
It would definite be good to avoid printing log statements in the success path like this: ideally, ET will never print logs during successful execution.
@manuelcandales what do you think about this interface?
| @@ -484,6 +486,30 @@ inline bool tensor_is_type( | |||
| return true; | |||
| } | |||
|
|
|||
| inline bool tensor_is_type( | |||
| executorch::aten::Tensor t, | |||
| const std::vector<executorch::aten::ScalarType>& dtypes) { | |||
There was a problem hiding this comment.
The runtime must not use streams, vectors or any other type that allocates memory dynamically: see https://pytorch.org/executorch/main/portable-cpp-programming.html for some commentary on this.
In this case you may be able to use a Span<ScalarType> since the container size is known at construction time.
There was a problem hiding this comment.
noted, I will use the Span container
| return true; | ||
| } | ||
|
|
||
| std::stringstream dtype_ss; |
There was a problem hiding this comment.
It's not safe to assemble a dynamic string here. If you wanted to print all of the types, printing individual log lines would be the safest and easiest. If you wanted to assemble a string, you could create a fixed-size buffer on the stack and strncat strings onto it, but that can be error-prone.
There was a problem hiding this comment.
yes, I was thinking about using the c-ctyle buffer for that, I will update
There was a problem hiding this comment.
I decided to go a simple way that provides an explicit override without any loops.
The use of strncat will lead to a warning https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83404 which I think is not desired.
@dbort , hope you find that version suitable.
There was a problem hiding this comment.
@dbort surficing the thread on top.
manuelcandales
added
the
release notes: ops & kernels
|
@manuelcandales can you take a look please how shall we proceed? |
) ### Summary There are false positive error messages from the type check that attempts to verify several allowed types at once, but uses one function call per type leading to some of them not passing the check and emitting an error message. ``` > ./executor_runner --model-path ~/model.pte ... I 00:00:00.519929 executorch:executor_runner.cpp:91] Using method forward I 00:00:00.519931 executorch:executor_runner.cpp:138] Setting up planned buffer 0, size 10753549312. I 00:00:01.212589 executorch:executor_runner.cpp:161] Method loaded. I 00:00:01.213544 executorch:executor_runner.cpp:171] Inputs prepared. E 00:00:02.320414 executorch:tensor_util.h:481] Expected to find Float type, but tensor has type Half E 00:00:02.320521 executorch:tensor_util.h:487] Check failed (t.scalar_type() == dtype): Expected to find Float type, but tensor has type Half ... I 00:00:24.911098 executorch:executor_runner.cpp:180] Model executed successfully. ... ``` Note the repeated lines > Check failed (t.scalar_type() == dtype): Expected to find Float type, but tensor has type Half ### Test plan I can only provide the log message from my local run for that ``` ... I 00:00:00.745945 executorch:executor_runner.cpp:138] Setting up planned buffer 0, size 13026185216. I 00:00:02.139255 executorch:executor_runner.cpp:161] Method loaded. I 00:00:02.139295 executorch:executor_runner.cpp:171] Inputs prepared. I 00:00:28.297681 executorch:executor_runner.cpp:180] Model executed successfully. ... ```
Summary
There are false positive error messages from the type check that attempts to verify several allowed types at once, but uses one function call per type leading to some of them not passing the check and emitting an error message.
Note the repeated lines
Test plan
I can only provide the log message from my local run for that