[AIMIGRAPHX-950] Revert to cout for driver output#4804
[AIMIGRAPHX-950] Revert to cout for driver output#4804
Conversation
eddieliao
added
the
Changelog: Not Applicable
There was a problem hiding this comment.
Pull request overview
Reverts driver “informational” output in src/driver/main.cpp from the MIGraphX logger back to plain stdout printing to preserve CI/script expectations, while keeping warnings on the logger (stderr).
Changes:
- Switched multiple
log::info()printouts (load/compile/verify/run/time/perf/roctx) tostd::cout. - Updated driver invocation, MIGraphX env dump, and completion message in
main()to print tostd::cout.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| file_type = get_file_type(file); | ||
| } | ||
| log::info() << "Reading: " << file; | ||
| std::cout << "Reading: " << file << std::endl; |
There was a problem hiding this comment.
std::cout is used here, but this file does not include <iostream> (it currently relies on transitive includes). Please add an explicit #include <iostream> to make the dependency correct and resilient to header/include changes.
| std::string driver_invocation = | ||
| std::string(argv[0]) + " " + migraphx::to_string_range(original_args, " "); | ||
| migraphx::log::info() << "Running [ " << get_version() << " ]: " << driver_invocation; | ||
| std::cout << "Running [ " << get_version() << " ]: " << driver_invocation << std::endl; |
There was a problem hiding this comment.
PR description says driver printouts in main are reverted to std::cout, but --ort-sha and --version paths still print via migraphx::log::info() (which may add formatting/prefixes and break scripts expecting plain stdout). Consider switching those to std::cout as well for consistency with the stated goal.
|
|
||
| std::string driver_invocation = | ||
| std::string(argv[0]) + " " + migraphx::to_string_range(original_args, " "); | ||
| migraphx::log::info() << "Running [ " << get_version() << " ]: " << driver_invocation; | ||
| std::cout << "Running [ " << get_version() << " ]: " << driver_invocation << std::endl; |
There was a problem hiding this comment.
The nearby comment indicates the first lines printed should follow the configured log level, but this message is now written to std::cout (independent of log severity). Please update/remove that comment (and/or the early log-level handling) so the code/comments remain accurate.
| file_type = get_file_type(file); | ||
| } | ||
| log::info() << "Reading: " << file; | ||
| std::cout << "Reading: " << file << std::endl; |
There was a problem hiding this comment.
I dont think its necessary to output status updates to cout. Those can go to the logger. I am not sure what other people think about that.
There was a problem hiding this comment.
@causten Thoughts? IMO the important ones are the perf numbers (already on cout) and potentially the command/env vars used, but if you'd like all the status updates to cout this PR does just that.
There was a problem hiding this comment.
I prefer logger, either is ok.
There was a problem hiding this comment.
Since this is a program that runs, not the actual migraphx library, lets get this back to the way all previous releases has printed it directly to stdout
There was a problem hiding this comment.
I prefer logger, either is ok.
I would prefer the logger as well because:
- It shows a timestamp on each status update so you can see what has happened when
- We can easily disable the extraneous output by disabling the logger or upping the log level to get more important information.
Intended output such as program output, times, perf reports, etc, should of course go to std out.
| std::string driver_invocation = | ||
| std::string(argv[0]) + " " + migraphx::to_string_range(original_args, " "); | ||
| migraphx::log::info() << "Running [ " << get_version() << " ]: " << driver_invocation; | ||
| std::cout << "Running [ " << get_version() << " ]: " << driver_invocation << std::endl; |
There was a problem hiding this comment.
The logger does need to at least be used here otherwise it wont print the time stamp.
There was a problem hiding this comment.
Would creating a separate cout sink work as a solution? The issue with the logger defaulting to the cerr sink is that the CI perf runs treat any output to cerr as a failure.
There was a problem hiding this comment.
Alternatively we edit the CI script to disable the logger when running and only collect perf results.
There was a problem hiding this comment.
We could set the log level to warning in the CI so it will only log important messages: MIGRAPHX_LOG_LEVEL=2.
| std::chrono::duration_cast<std::chrono::duration<double>>(end_time - start_time); | ||
| migraphx::log::info() << "[ " << get_version() << " ] Complete(" << duration.count() | ||
| << "s): " << driver_invocation; | ||
| std::cout << "[ " << get_version() << " ] Complete(" << duration.count() |
There was a problem hiding this comment.
The logger also needs to be used here so it will print the timestamp.
Regressions detected 🔴 |
|
Motivation
Reverts usage of the logger for printouts in driver main. This is to ensure current CI runs still work properly. Warnings are still kept on logger and will be output to std::cerr.
Technical Details
Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot Applicable