Skip to content

Allow comparison of completely arbitrary json files #371

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

Merged
merged 1 commit into from
Mar 11, 2025
Merged

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Mar 11, 2025

No description provided.

@mdboom mdboom requested a review from Copilot March 11, 2025 16:32
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request enables comparisons between completely arbitrary JSON files as well as traditional git commit hashes by introducing a new processing branch for JSON files and related helper functions.

  • Refactors comparison functions to handle an optional machine parameter (str | None)
  • Introduces helper functions (name_and_hash, get_first_result_for_machine) and a new _main_with_files entry point
  • Adjusts runner lookup methods to default to an unknown runner when necessary

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
bench_runner/scripts/compare.py Updated functions to handle optional machine values and refactored name formatting for clarity
bench_runner/runners.py Introduced a default unknown_runner and adjusted hostname lookup to avoid key errors
tests/test_compare.py Modified expected outputs to remove redundant commit hash formatting
bench_runner/result.py Added force_valid in Comparison, updated is_windows logic, and relocated from_arbitrary_filename
Comments suppressed due to low confidence (4)

bench_runner/scripts/compare.py:73

  • [nitpick] Consider adding an inline comment to explain the purpose of the newly added 'force_valid' parameter to clarify its intended behavior.
comparison = mod_result.BenchmarkComparison(ref, head, "base", True)

bench_runner/scripts/compare.py:87

  • [nitpick] Adding a docstring to 'name_and_hash' could help clarify why the function returns just the name when it equals the hash.
def name_and_hash(name: str, hash: str) -> str:

bench_runner/runners.py:88

  • [nitpick] Ensure that the 'unknown_runner' default is initialized with appropriate values (and documented) so that downstream code using its attributes behaves as expected.
return get_runners_by_hostname().get(hostname, unknown_runner).nickname

bench_runner/result.py:647

  • In 'is_windows', ensure that self.metadata is always available when self.nickname is 'unknown' to avoid potential attribute errors; consider adding a guard for missing metadata.
if self.nickname != "unknown":

@mdboom mdboom merged commit 194b390 into main Mar 11, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant