-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[BENCHMARK_APP] Show input command in output #13402
[BENCHMARK_APP] Show input command in output #13402
Conversation
samples/cpp/benchmark_app/main.cpp
Outdated
@@ -161,12 +190,16 @@ int main(int argc, char* argv[]) { | |||
|
|||
// ----------------- 1. Parsing and validating input arguments | |||
// ------------------------------------------------- | |||
auto args_string = get_console_command(argc, argv); |
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.
What is a reason for doing it inside tool?
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.
It is easier to verify which benchmark_app is being used, C++ or Python, and from which directory. It can happen for example when the PYTHONPATH/PATH env variable is modified, or after the output alignment PR is merged, when both benchmarks' output is indistinguishable.
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.
It is easier to verify which benchmark_app is being used, C++ or Python, and from which directory. It can happen for example when the PYTHONPATH/PATH env variable is modified, or after the output alignment PR is merged, when both benchmarks' output is indistinguishable.
Validation related stuff should not be included into the tool. Why this PR was merged? cc @p-durandin
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.
As pointed out by @helena-intel in the ticket, the idea of this change is to make the usage of the tool and sharing output logs easier for the customers and our coworkers alike, I think this is a valid addition to the tool
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.
@PiotrKrzem I still don't understand why it is problem to share logs with initial command line. If user have ran tool, he knows what he typed in command line.
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.
Moreover for some automation scenarios we increase final log size.
…#13402)" (openvinotoolkit#14027) This reverts commit f893a58.
Details:
Tickets: