-
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
Merged
p-wysocki
merged 12 commits into
openvinotoolkit:master
from
PiotrKrzem:feature/benchmark_command_in_output
Oct 26, 2022
Merged
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
00bcbc5
[SAMPLES/CPP] Show input command in output
PiotrKrzem 7346166
Update main.py
PiotrKrzem c09adea
Merge branch 'master' into feature/benchmark_command_in_output
PiotrKrzem b72a324
Merge branch 'master' into feature/benchmark_command_in_output
PiotrKrzem b7b9e49
Merge branch 'master' into feature/benchmark_command_in_output
PiotrKrzem 05f35cb
Merge branch 'master' into feature/benchmark_command_in_output
PiotrKrzem af2ca6b
[CPP] Add Windows-specific abspath method
PiotrKrzem def219d
[CPP] Formatting errors
PiotrKrzem 6772e7e
[C++] Fix naming conventions, fix incorrect buffer type to store outp…
PiotrKrzem 5b6fd41
[C++] Formatting errors - invisible spaces remove
PiotrKrzem 89ce3b1
[C++] Add comments, fix name of the command_string variable
PiotrKrzem c61ef10
[C++] Remove redefinition of symbol
PiotrKrzem File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.