From d74c06f4b7dd3e286e843e82d3d17038ccb2e09d Mon Sep 17 00:00:00 2001 From: Oleksandr Pavlyk <21087696+oleksandr-pavlyk@users.noreply.github.com> Date: Tue, 14 Jul 2026 16:49:28 -0500 Subject: [PATCH] Harden nvbench-compare CLI failure handling Apply the same small stability fixes to the modern and legacy compare scripts: report malformed nested JSON through the structured parse-error path, reject directory comparisons that produce no matching JSON files, use the supported pipe table format for old tabulate versions, and retitle the summary plot as a GPU timing change. Add regression coverage for the modern comparator error paths and output formatting. --- python/scripts/nvbench_compare.py | 14 ++- python/scripts/nvbench_compare_legacy.py | 14 ++- python/test/test_nvbench_compare.py | 106 +++++++++++++++++++++++ 3 files changed, 126 insertions(+), 8 deletions(-) diff --git a/python/scripts/nvbench_compare.py b/python/scripts/nvbench_compare.py index 98dc6ad..396b906 100644 --- a/python/scripts/nvbench_compare.py +++ b/python/scripts/nvbench_compare.py @@ -3316,7 +3316,7 @@ def compare_benches( ) ) else: - print(tabulate.tabulate(rows, headers=headers, tablefmt="markdown")) + print(tabulate.tabulate(rows, headers=headers, tablefmt="pipe")) print("") @@ -3392,7 +3392,7 @@ def compare_benches( plt.close(fig) if plot: - title = "%SOL Bandwidth change" + title = "GPU timing change" if len(comparison_device_names) == 1: title = f"{title} - {next(iter(comparison_device_names))}" if filter_plan.global_axis_filters: @@ -3574,6 +3574,12 @@ def main() -> int: to_compare.append((r, c)) else: to_compare = [(files_or_dirs[0], files_or_dirs[1])] + if not to_compare: + print( + f"No non-empty matching JSON files found in {files_or_dirs[0]!r} " + f"and {files_or_dirs[1]!r}" + ) + return 1 stats = ComparisonStats() @@ -3590,7 +3596,7 @@ def main() -> int: except ValueError as exc: print(str(exc)) return 1 - except (KeyError, TypeError, IndexError) as exc: + except (AttributeError, KeyError, TypeError, IndexError) as exc: print(format_json_structure_error(ref, comp, exc)) return 1 @@ -3659,7 +3665,7 @@ def main() -> int: except ValueError as exc: print(str(exc)) return 1 - except (KeyError, TypeError, IndexError) as exc: + except (AttributeError, KeyError, TypeError, IndexError) as exc: print(format_json_structure_error(ref, comp, exc)) return 1 diff --git a/python/scripts/nvbench_compare_legacy.py b/python/scripts/nvbench_compare_legacy.py index b902185..cec4c49 100644 --- a/python/scripts/nvbench_compare_legacy.py +++ b/python/scripts/nvbench_compare_legacy.py @@ -1349,7 +1349,7 @@ def compare_benches( ) ) else: - print(tabulate.tabulate(rows, headers=headers, tablefmt="markdown")) + print(tabulate.tabulate(rows, headers=headers, tablefmt="pipe")) print("") @@ -1425,7 +1425,7 @@ def compare_benches( plt.close(fig) if plot: - title = "%SOL Bandwidth change" + title = "GPU timing change" if len(comparison_device_names) == 1: title = f"{title} - {next(iter(comparison_device_names))}" if filter_plan.global_axis_filters: @@ -1557,6 +1557,12 @@ def main() -> int: to_compare.append((r, c)) else: to_compare = [(files_or_dirs[0], files_or_dirs[1])] + if not to_compare: + print( + f"No non-empty matching JSON files found in {files_or_dirs[0]!r} " + f"and {files_or_dirs[1]!r}" + ) + return 1 stats = ComparisonStats() @@ -1573,7 +1579,7 @@ def main() -> int: except ValueError as exc: print(str(exc)) return 1 - except (KeyError, TypeError, IndexError) as exc: + except (AttributeError, KeyError, TypeError, IndexError) as exc: print(format_json_structure_error(ref, comp, exc)) return 1 @@ -1635,7 +1641,7 @@ def main() -> int: except ValueError as exc: print(str(exc)) return 1 - except (KeyError, TypeError, IndexError) as exc: + except (AttributeError, KeyError, TypeError, IndexError) as exc: print(format_json_structure_error(ref, comp, exc)) return 1 diff --git a/python/test/test_nvbench_compare.py b/python/test/test_nvbench_compare.py index 8378f37..8e09090 100644 --- a/python/test/test_nvbench_compare.py +++ b/python/test/test_nvbench_compare.py @@ -3170,6 +3170,54 @@ def test_main_reports_invalid_benchmark_entry_structure( assert "missing key 'name'" in output +def test_main_reports_invalid_nested_json_structure( + monkeypatch, capsys, nvbench_compare +): + root = { + "devices": [{"id": 0, "name": "Test GPU"}], + "benchmarks": [ + { + "name": "bench", + "devices": [0], + "axes": [], + "states": [ + { + "name": "state", + "device": 0, + "axis_values": ["bad"], + "summaries": [], + } + ], + } + ], + } + + monkeypatch.setattr(nvbench_compare.reader, "read_file", lambda _: root) + monkeypatch.setattr(sys, "argv", ["nvbench_compare", "ref.json", "cmp.json"]) + + assert nvbench_compare.main() == 1 + output = capsys.readouterr().out + assert "invalid NVBench JSON structure" in output + assert "has no attribute 'get'" in output + + +def test_main_rejects_directory_inputs_without_matching_json_files( + tmp_path, monkeypatch, capsys, nvbench_compare +): + ref_dir = tmp_path / "ref" + cmp_dir = tmp_path / "cmp" + ref_dir.mkdir() + cmp_dir.mkdir() + (ref_dir / "ref_only.json").write_text("{}", encoding="utf-8") + (cmp_dir / "cmp_only.json").write_text("{}", encoding="utf-8") + + monkeypatch.setattr(sys, "argv", ["nvbench_compare", str(ref_dir), str(cmp_dir)]) + + assert nvbench_compare.main() == 1 + output = capsys.readouterr().out + assert "No non-empty matching JSON files found" in output + + def test_main_prints_bulk_debug_python_to_stdout(monkeypatch, capsys, nvbench_compare): devices = [{"id": 0, "name": "Test GPU"}] root = { @@ -3405,6 +3453,64 @@ def test_compare_benches_legacy_display_uses_scalar_diff(monkeypatch, nvbench_co assert row[-2] == "1.00%" +def test_compare_benches_old_tabulate_fallback_uses_pipe_format( + monkeypatch, nvbench_compare +): + run_data = make_comparison_run_data(nvbench_compare) + tabulate_calls = [] + + def fake_tabulate(rows, headers, *args, **kwargs): + tabulate_calls.append({"rows": rows, "headers": headers, "kwargs": kwargs}) + return "" + + monkeypatch.setattr( + nvbench_compare, + "load_tabulate_for_table_output", + lambda: (types.SimpleNamespace(tabulate=fake_tabulate), (0, 8, 2)), + ) + + nvbench_compare.compare_benches( + run_data, + [make_benchmark([make_state(nvbench_compare, "state", mean="1.0")])], + [make_benchmark([make_state(nvbench_compare, "state", mean="1.01")])], + threshold=0.0, + plot_along=None, + plot=False, + dark=False, + filter_plan=make_filter_plan(nvbench_compare), + no_color=True, + ) + + assert tabulate_calls[0]["kwargs"]["tablefmt"] == "pipe" + + +def test_compare_benches_summary_plot_title_describes_timing( + monkeypatch, nvbench_compare +): + run_data = make_comparison_run_data(nvbench_compare) + plot_calls = [] + + monkeypatch.setattr( + nvbench_compare, + "plot_comparison_entries", + lambda *args, **kwargs: plot_calls.append({"args": args, "kwargs": kwargs}), + ) + + nvbench_compare.compare_benches( + run_data, + [make_benchmark([make_state(nvbench_compare, "state", mean="1.0")])], + [make_benchmark([make_state(nvbench_compare, "state", mean="1.01")])], + threshold=0.0, + plot_along=None, + plot=True, + dark=False, + filter_plan=make_filter_plan(nvbench_compare), + no_color=True, + ) + + assert plot_calls[0]["kwargs"]["title"] == "GPU timing change - Test GPU" + + def test_compare_benches_explain_display_uses_explicit_intervals( monkeypatch, nvbench_compare ):