Skip to content
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

Update 2.x logging #308

Merged
merged 45 commits into from
Mar 11, 2024
Merged

Update 2.x logging #308

merged 45 commits into from
Mar 11, 2024

Conversation

koomie
Copy link
Collaborator

@koomie koomie commented Mar 8, 2024

This PR builds on top of the nice work of #236 to take advantage of improved logging and enables a quiet mode of operation via -q or --quiet. It also re-establishes the creation of a log.txt file is stored in the workloads directory when run in profiling mode. This file will include profiling output even when the console mode is configured to run in quiet mode.

Quiet mode example

$ ../src/omniperf profile -q --no-roof -n atest -- ./vcopy -n 1048576 -b 256

  ___                  _                  __ 
 / _ \ _ __ ___  _ __ (_)_ __   ___ _ __ / _|
| | | | '_ ` _ \| '_ \| | '_ \ / _ \ '__| |_ 
| |_| | | | | | | | | | | |_) |  __/ |  |  _|
 \___/|_| |_| |_|_| |_|_| .__/ \___|_|  |_|  
                        |_|                  

100%|█████████████████████████████████████████████████████████████████████████████████████████████| 25/25 [00:14<00:00,  1.69it/s]

Full profiling output from above would reside in workloads/atest/MI200/log.txt, e.g.

$ head  -50 workloads/atest/MI200/log.txt 
Omniperf version: 2.0.0-Tech-Preview
Profiler choice: rocprofv2
Path: /home1/karl/repos/omniperf2/sample/workloads/atest/MI200
Target: MI200
Command: ./vcopy -n 1048576 -b 256
Kernel Selection: None
Dispatch Selection: None
IP Blocks: All
KernelName verbose: 2

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Collecting Performance Counters
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[profile] Current input file: /home1/karl/repos/omniperf2/sample/workloads/atest/MI200/perfmon/pmc_perf_9.txt
   |-> [rocprofv2] ROCProfilerV2: Collecting the following counters:
   |-> [rocprofv2] - SQ_LDS_ADDR_CONFLICT
   |-> [rocprofv2] - SQ_LDS_UNALIGNED_STALL
   |-> [rocprofv2] - SQ_WAVES_EQ_64
   |-> [rocprofv2] - SQ_WAVES_LT_64
[profile] Current input file: /home1/karl/repos/omniperf2/sample/workloads/atest/MI200/perfmon/pmc_perf_12.txt
   |-> [rocprofv2] ROCProfilerV2: Collecting the following counters:
   |-> [rocprofv2] - SQ_INSTS_VALU_MFMA_MOPS_BF16
   |-> [rocprofv2] - SQ_INSTS_VALU_MFMA_MOPS_F32
   |-> [rocprofv2] - SQ_INSTS_VALU_MFMA_MOPS_F64
   |-> [rocprofv2] Enabling Counter Collection
   |-> [rocprofv2] vcopy testing on GCD 0
   |-> [rocprofv2] Finished allocating vectors on the CPU
   |-> [rocprofv2] Finished allocating vectors on the GPU
   |-> [rocprofv2] Finished copying vectors to the GPU
   |-> [rocprofv2] sw thinks it moved 1.000000 KB per wave
   |-> [rocprofv2] Total threads: 1048576, Grid Size: 4096 block Size:256, Wavefronts:16384:
[profile] Current input file: /home1/karl/repos/omniperf2/sample/workloads/atest/MI200/perfmon/pmc_perf_5.txt
   |-> [rocprofv2] ROCProfilerV2: Collecting the following counters:
   |-> [rocprofv2] - SQ_INSTS_VALU_FMA_F32
   |-> [rocprofv2] - SQ_INSTS_VALU_TRANS_F32
   |-> [rocprofv2] - SQ_INSTS_VALU_ADD_F64
   |-> [rocprofv2] - SQ_INSTS_VALU_MUL_F64
   |-> [rocprofv2] - SQ_INSTS_VALU_FMA_F64
   |-> [rocprofv2] - SQ_INSTS_VALU_TRANS_F64
[profile] Current input file: /home1/karl/repos/omniperf2/sample/workloads/atest/MI200/perfmon/SQ_IFETCH_LEVEL.txt
   |-> [rocprofv2] ROCProfilerV2: Collecting the following counters:
   |-> [rocprofv2] - GRBM_COUNT
   |-> [rocprofv2] - GRBM_GUI_ACTIVE
   |-> [rocprofv2] - SQ_WAVES
   |-> [rocprofv2] - SQ_IFETCH
   |-> [rocprofv2] - SQ_IFETCH_LEVEL
   |-> [rocprofv2] - SQ_ACCUM_PREV_HIRES
   |-> [rocprofv2] Enabling Counter Collection
   |-> [rocprofv2] vcopy testing on GCD 0

@koomie koomie added the enhancement New feature or request label Mar 8, 2024
@koomie koomie added this to the v2.0.0 milestone Mar 8, 2024
@koomie koomie requested a review from coleramos425 March 8, 2024 21:22
coleramos425 and others added 27 commits March 8, 2024 16:39
Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: colramos-amd <colramos@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
loglevel is set to ERROR

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
loglevel delimiter in output

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
indented output with profiler selection when enabled

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
* add a profileMode keyword option, used to indent profiling output

* update logic if forked process is not successful, output is
  displayed directly with INFO logging or lower; also dispaly output
  in ERROR mode if the process fails

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
lines prior to code exit

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
quiet arg to setup_logging function

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
(1) setup_logging_handler -> called prior to arg parsing
(2) setup_logging_priority -> called after arg parsing

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
this function enables file-based logger output for use with profile
mode

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
…o debug logs

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
…o debug

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
wrapper and push into base class via a companion utility function

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Copy link
Collaborator

@coleramos425 coleramos425 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I had a few minor suggestions below

Comment on lines 603 to 606
print("Please ensure that the 'en_US.UTF-8' locale is available on your system.")
print("")
print("ERROR: ", error)
sys.exit(1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we're not using console_error() here? If we want to standardize it we can use:

Suggested change
print("Please ensure that the 'en_US.UTF-8' locale is available on your system.")
print("")
print("ERROR: ", error)
sys.exit(1)
console_log("Please ensure that the 'en_US.UTF-8' locale is available on your system.")
console_log("")
console_error(error)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because this check happens right up front before the logging handlers and formatters are configured. Consequently, you will get the default formatter which is a bit confusing and looks odd.

To be fair, I'm not sure we need this check right up front but I was trying to keep it similar to what you had while moving out of top-level omniperf to keep that file clean. If we wanted to push this check a bit further down the call stack, then we could likely go back to using console_error()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added add814f which implements this approach to set_locale_encoding after logging is fully initialized.

src/omniperf_base.py Outdated Show resolved Hide resolved
src/omniperf_profile/profiler_rocscope.py Outdated Show resolved Hide resolved
src/omniperf_profile/profiler_rocscope.py Outdated Show resolved Hide resolved
src/omniperf_profile/profiler_base.py Outdated Show resolved Hide resolved
src/omniperf_profile/profiler_base.py Outdated Show resolved Hide resolved
src/utils/db_connector.py Outdated Show resolved Hide resolved
src/utils/db_connector.py Outdated Show resolved Hide resolved
src/utils/utils.py Outdated Show resolved Hide resolved
src/utils/utils.py Outdated Show resolved Hide resolved
koomie and others added 11 commits March 11, 2024 11:14
Co-authored-by: Cole Ramos <colramos@amd.com>
Signed-off-by: Karl W. Schulz <koomie@users.noreply.github.com>
Co-authored-by: Cole Ramos <colramos@amd.com>
Signed-off-by: Karl W. Schulz <koomie@users.noreply.github.com>
Co-authored-by: Cole Ramos <colramos@amd.com>
Signed-off-by: Karl W. Schulz <koomie@users.noreply.github.com>
Co-authored-by: Cole Ramos <colramos@amd.com>
Signed-off-by: Karl W. Schulz <koomie@users.noreply.github.com>
Co-authored-by: Cole Ramos <colramos@amd.com>
Signed-off-by: Karl W. Schulz <koomie@users.noreply.github.com>
Co-authored-by: Cole Ramos <colramos@amd.com>
Signed-off-by: Karl W. Schulz <koomie@users.noreply.github.com>
Co-authored-by: Cole Ramos <colramos@amd.com>
Signed-off-by: Karl W. Schulz <koomie@users.noreply.github.com>
Co-authored-by: Cole Ramos <colramos@amd.com>
Signed-off-by: Karl W. Schulz <koomie@users.noreply.github.com>
Co-authored-by: Cole Ramos <colramos@amd.com>
Signed-off-by: Karl W. Schulz <koomie@users.noreply.github.com>
function to use console_error() directly (#308 (comment))

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
@koomie
Copy link
Collaborator Author

koomie commented Mar 11, 2024

Thanks for this! I had a few minor suggestions below

Thanks for all the suggestions. I think I landed all of them and tried to address use of console_log during the locale encoding check as well.

If you're satisfied, I think we are ready to land this PR.

@koomie
Copy link
Collaborator Author

koomie commented Mar 11, 2024

Thanks for this! I had a few minor suggestions below

Thanks for all the suggestions. I think I landed all of them and tried to address use of console_log during the locale encoding check as well.

If you're satisfied, I think we are ready to land this PR.

One other minor comment: this will affect a number of the CLI examples shown in the docs and we will need to update those as well at some point.

@coleramos425
Copy link
Collaborator

If you're satisfied, I think we are ready to land this PR.

One other minor comment: this will affect a number of the CLI examples shown in the docs and we will need to update those as well at some point.

Great! I'll hop on those doc changes as soon as it's landed

@koomie koomie merged commit e85037f into 2.x Mar 11, 2024
7 checks passed
@koomie koomie mentioned this pull request Mar 11, 2024
@coleramos425 coleramos425 deleted the better-logs-koomie branch March 13, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants