Improve robustness of perfRunner by adding error checking in several places #1983
Conversation
mlir/utils/performance/perfRunner.py
Outdated
| def validate_tuning_db_entry(arch, config, perfConfig): | ||
| # Validate arch | ||
| if " " in arch: | ||
| raise ValueError(f"invalid db entry: '{arch} {config} {perfConfig}' with arch='{arch}'") |
There was a problem hiding this comment.
I think it would be better to Found invalid db entry. Arch is incorrect or arch is empty
There was a problem hiding this comment.
Using a regex now as suggested by Daniel, so finer grain errors are not easy to have
mlir/utils/performance/perfRunner.py
Outdated
| return result_average | ||
|
|
||
| def valid_perf_config(perfConfig): | ||
| if not (perfConfig.startswith('v1') or perfConfig.startswith('v2') or perfConfig.startswith('v3')): |
There was a problem hiding this comment.
Print better message like perf_config doesn't start with v1/v2/v3 ?
There was a problem hiding this comment.
add a TODO to use this function: parse_perf_config() (of this PR: https://github.com/ROCm/rocMLIR/pull/1918/files) once it's merged. Or copy-paste it to a common utility file and use it here.
There was a problem hiding this comment.
I'm using parse_perf_config (from Justin's PR) now to check perfConfig as suggested by Daniel, I will make sure that Justin adds this checks to have an informative error message
mlir/utils/performance/perfRunner.py
Outdated
| return result_average | ||
|
|
||
| def valid_perf_config(perfConfig): | ||
| if not (perfConfig.startswith('v1') or perfConfig.startswith('v2') or perfConfig.startswith('v3')): |
There was a problem hiding this comment.
add a TODO to use this function: parse_perf_config() (of this PR: https://github.com/ROCm/rocMLIR/pull/1918/files) once it's merged. Or copy-paste it to a common utility file and use it here.
pabloantoniom
changed the title
pabloantoniom
changed the title
| with open(fileName, mode='r', newline='') as csv_file: | ||
| reader = csv.DictReader(csv_file) | ||
| return sum(int(float(row['AverageNs'])) for row in reader if 'AverageNs' in row) | ||
| except KeyError: |
There was a problem hiding this comment.
will we have a KeyError exeption? We check "if 'AverageNs' in row"
| # Validates that the db entry (composed by the arch, config, and perfConfig) is well-formed and consistent | ||
| def validate_tuning_db_entry(arch, num_cu, config, perf_config): | ||
| # 1. Check perf_config with parse_perf_config helper function. | ||
| if num_cu == "unknown": |
There was a problem hiding this comment.
why not use None as a sentinel for unknown?
| # 1. Check perf_config with parse_perf_config helper function. | ||
| if num_cu == "unknown": | ||
| # parse_perf_config requires num_cu, so just pass 1 to make it happy. | ||
| parsed_params = parse_perf_config(perf_config, "1", arch) |
There was a problem hiding this comment.
why are we passing 1? I think we should always know the num_cus
| raise ValueError(f"invalid db entry: '{arch} {config} {perf_config}' with arch='{arch}': perf_config is invalid") | ||
|
|
||
| # 2. Validate arch | ||
| arch_pattern = r"^gfx([0-9]+)(:[^:\s]+)*$" |
There was a problem hiding this comment.
already exists in perfRunner: GFX_CHIP_RE
| raise ValueError(f"invalid db entry: '{arch} {config} {perf_config}' with arch='{arch}': arch is invalid") | ||
|
|
||
| # 3. Validate config | ||
| re_ty = r"(f32|f16|i16|i8)" |
There was a problem hiding this comment.
use existing variables for types in perfRunner: DATA_TYPES_GEMM, DATA_TYPES_GEMM_GEMM, ...
| re_tf = r"(true|false)" | ||
| gemm_config_pattern = rf"^-t {re_ty} -out_datatype {re_ty} -transA {re_tf} -transB {re_tf} -g [0-9]+ -m [0-9]+ -n [0-9]+ -k [0-9]+$" | ||
|
|
||
| re_conv = r"(conv|convint8|convfp16)" |
There was a problem hiding this comment.
use DATA_TYPES constant
| # 3. Validate config | ||
| re_ty = r"(f32|f16|i16|i8)" | ||
| re_tf = r"(true|false)" | ||
| gemm_config_pattern = rf"^-t {re_ty} -out_datatype {re_ty} -transA {re_tf} -transB {re_tf} -g [0-9]+ -m [0-9]+ -n [0-9]+ -k [0-9]+$" |
There was a problem hiding this comment.
this is not needed. We already check (or should check) in fromCommandLine() of GemmConfiguration, AttentionConfiguration, etc...
| if parsed_params is None: | ||
| raise ValueError(f"invalid db entry: '{arch} {config} {perf_config}' with arch='{arch}': perf_config is invalid") | ||
|
|
||
| # 2. Validate arch |
There was a problem hiding this comment.
I think we should have the checks in fromCommandLine() instead of here.
| attn_ty = r"(f32|f16)" | ||
| attn_config_pattern = rf"^-t {attn_ty} -transQ {re_tf} -transK {re_tf} -transV {re_tf} -transO {re_tf} -g [0-9]+ -seq_len_q [0-9]+ -seq_len_k [0-9]+ -head_dim_qk [0-9]+ -head_dim_v [0-9]+$" | ||
|
|
||
| if not re.match(gemm_config_pattern, config) and not re.match(conv_config_pattern, config) and not re.match(attn_config_pattern, config): |
There was a problem hiding this comment.
missing gemm+gemm and conv+gemm
| if len(entries) == 3: | ||
| arch, config, perfConfig = entries | ||
| ret[arch, config] = perfConfig | ||
| numCu = "unknown" |
There was a problem hiding this comment.
rather than using unknown, let's get the default value for each arch.
3 participants
Motivation
This PR improves the robustness of the script by doing the following:
getNanoSeconds, mainly check if file exists before attempting to read it (which might be related to rocprof not generating the file in the expected path) and check that the format is correct.Technical Details
Check arch, config and perfConfig and make sure they are all valid. Check errors in
getNanoSecondsTest Plan
Testing with invalid entries.
Test Result
The error of each invalid entry is reported properly.
Submission Checklist