diff --git a/src-python/amazon/ionbenchmark/ion_benchmark_cli.py b/src-python/amazon/ionbenchmark/ion_benchmark_cli.py index 412173aec..233181111 100644 --- a/src-python/amazon/ionbenchmark/ion_benchmark_cli.py +++ b/src-python/amazon/ionbenchmark/ion_benchmark_cli.py @@ -72,10 +72,6 @@ def compare_command(): regression_threshold = float(args['--threshold']) comparison_keywords_arg = args['--compare'] - # TODO: Update this command to use the information in REPORT_FIELDS, such as the direction of improvement (doi). - # https://github.com/amazon-ion/ion-python/issues/281 - # Without that (i.e. right now), the compare command will actually fail when the ops/sec metric improves. :S - comparison_fields = [get_report_field_by_name(name) for name in comparison_keywords_arg.split(",")] with open(previous_path, 'br') as p, open(current_path, 'br') as c: @@ -100,7 +96,10 @@ def compare_command(): pct_diff = f"{relative_diff:.2%}" result[key] = pct_diff - if relative_diff > regression_threshold: + # Throughput improves as it increases; keep the existing comparison + # for fields without an explicit direction of improvement. + regression = -relative_diff if field.doi == 1 else relative_diff + if regression > regression_threshold: if not args['--quiet']: print(f"{case_name} '{key}' changed by {pct_diff}: {prev} => {cur}") has_regression = True diff --git a/tests/test_benchmark_cli.py b/tests/test_benchmark_cli.py index 9d948b53a..319923fb9 100644 --- a/tests/test_benchmark_cli.py +++ b/tests/test_benchmark_cli.py @@ -1,7 +1,10 @@ import os +import sys import time from os.path import abspath, join, dirname +import pytest + from amazon.ion import simpleion from amazon.ion.equivalence import ion_equals from amazon.ionbenchmark import Format, benchmark_spec @@ -17,7 +20,7 @@ def generate_test_path(p): def run_cli(c): import subprocess - cmd = ["python", abspath(join(dirname(os.path.abspath(__file__)), '../src-python/amazon/ionbenchmark/ion_benchmark_cli.py'))] + c + cmd = [sys.executable, abspath(join(dirname(os.path.abspath(__file__)), '../src-python/amazon/ionbenchmark/ion_benchmark_cli.py'))] + c proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, text=True) error_code = proc.wait() (out, err) = proc.communicate() @@ -208,6 +211,42 @@ def test_compare_with_large_regression(): assert error_code +@pytest.mark.parametrize('args', [ + ('ops/s_mean', 150, False), + ('ops/s_mean', 50, True), + ('ops/s_min', 150, False), + ('ops/s_min', 50, True), + ('ops/s_max', 150, False), + ('ops/s_max', 50, True), + ('ops/s_mean', 90, False), + ('ops/s_mean', 80, False), + ('time_mean', 150, True), + ('time_mean', 50, False), + ('file_size', 150, True), + ('file_size', 50, False), +]) +def test_compare_metric_direction(args, tmp_path): + field, current, has_regression = args + units = '(ns)' if field == 'time_mean' else '(B)' if field == 'file_size' else '' + key = field + units + previous_path = tmp_path / 'previous.ion' + current_path = tmp_path / 'current.ion' + report_path = tmp_path / 'comparison.ion' + for path, value in ((previous_path, 100), (current_path, current)): + with path.open('wb') as output: + simpleion.dump([{'name': 'example', key: value}], output, binary=False) + + error_code, _, _ = run_cli([ + 'compare', str(previous_path), str(current_path), '--fail', + '-c', field, '--output', str(report_path), + ]) + assert bool(error_code) == has_regression + with report_path.open('rb') as report_file: + report = simpleion.load(report_file) + # Reports retain the signed change, even when higher values are better. + assert report[0][key] == f'{(current - 100) / 100:.2%}' + + def test_format_conversion_ion_binary_to_ion_text(): rewrite_file_to_format(generate_test_path('integers.ion'), Format.Format.ION_BINARY.value) assert os.path.exists('temp_integers.10n') @@ -232,4 +271,3 @@ def test_multiple_top_level_values(args): (command, format_option, file) = args (error_code, _, _) = run_cli([f'{command}', file, '--format', f'{format_option}', '--io-type', 'file']) assert not error_code -