Re: [PATCH v4] benchtests: Fix validate_benchout.py exceptions

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 9/13/21 7:14 PM, Naohiro Tamura wrote:
> This patch fixed validate_benchout.py two exceptions, AttributeError
> if benchout_strings.schema.json is specified and
> json.decoder.JSONDecodeError if benchout is not JSON.
> 
> AttributeError unconditionally occurs with a correct JSON benchout
> file such as below because the code
> "bench['functions'][func][k].keys()" is either  "bench-variant",
> "ifunc", or "results" that doesn't have keys()."
> 
> $ ~/glibc/benchtests/scripts/validate_benchout.py bench-memcpy.out \
>    ~/glibc/benchtests/scripts/benchout_strings.schema.json
> Traceback (most recent call last):
>    File "/home/naohirot/work/github/glibc/benchtests/scripts/validate_benchout.py", line 86, in <module>
>      sys.exit(main(sys.argv[1:]))
>    File "/home/naohirot/work/github/glibc/benchtests/scripts/validate_benchout.py", line 69, in main
>      bench.parse_bench(args[0], args[1])
>    File "/home/naohirot/work/github/glibc/benchtests/scripts/import_bench.py", line 139, in parse_bench
>      do_for_all_timings(bench, lambda b, f, v:
>    File "/home/naohirot/work/github/glibc/benchtests/scripts/import_bench.py", line 107, in do_for_all_timings
>      if 'timings' not in bench['functions'][func][k].keys():
> AttributeError: 'str' object has no attribute 'keys'
> 
> $ cat bench-memcpy.out
>    1 {
>    2  "timing_type": "hp_timing",
>    3  "functions": {
>    4   "memcpy": {
>    5    "bench-variant": "default",
>    6    "ifuncs": ["generic_memcpy", "__memcpy_thunderx", "__memcpy_thunderx2", "__memcpy_falkor", "__memcpy_simd", "__memcpy_a64fx", "__memcpy_generic"],
>    7    "results": [
>    8     {
>    9      "length": 1,
>   10      "align1": 0,
>   11      "align2": 0,
>   12      "dst > src": 0,
>   13      "timings": [10.9326, 11.0449, 11.5515, 13.5693, 11.5198, 6.77368, 11.5259]
>   14     },
>   ...
> ---
>   benchtests/scripts/import_bench.py      | 17 +++++++++++------
>   benchtests/scripts/validate_benchout.py |  6 +++++-
>   2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/benchtests/scripts/import_bench.py b/benchtests/scripts/import_bench.py
> index a799b4e1b7dc..f5e67570d4c5 100644
> --- a/benchtests/scripts/import_bench.py
> +++ b/benchtests/scripts/import_bench.py
> @@ -101,13 +101,18 @@ def do_for_all_timings(bench, callback):
>       Args:
>           bench: The benchmark object
>           callback: The callback function
> +    Raises:
> +        validator.exceptions.ValidationError: if 'timings' key not found
>       """
>       for func in bench['functions'].keys():
>           for k in bench['functions'][func].keys():
> -            if 'timings' not in bench['functions'][func][k].keys():
> -                continue
> -
> -            callback(bench, func, k)
> +            if k == 'results':
> +                for r in range(len(bench['functions'][func][k])):
> +                    if 'timings' not in bench['functions'][func][k][r].keys():
> +                        raise validator.exceptions.ValidationError(
> +                            "'timings' key not found")
> +                    else:
> +                        callback(bench, func, k, r)
>   
>   
>   def compress_timings(points):
> @@ -136,6 +141,6 @@ def parse_bench(filename, schema_filename):
>           with open(filename, 'r') as benchfile:
>               bench = json.load(benchfile)
>               validator.validate(bench, schema)
> -            do_for_all_timings(bench, lambda b, f, v:
> -                    b['functions'][f][v]['timings'].sort())
> +            do_for_all_timings(bench, lambda b, f, v, r:
> +                    b['functions'][f][v][r]['timings'].sort())
>               return bench

This will break the original use case, i.e. bench.out, since it doesn't 
have the 'results' nesting timings.  The sorting doesn't seem necessary 
for validation, it's only necessary for compare_bench.py.  You could 
move the do_for_all_timings call into compare_bench.py, which is 
specific to bench.out so that you don't have to modify do_for_all_timings.

Siddhesh