Option to re-display a benchmark file #185
Conversation
jaredoconnell
force-pushed
the
redisplay-results
branch
from
350e1e2 to
1e496b0
Compare
The gitignore is setup to ignore these files by default
jaredoconnell
force-pushed
the
redisplay-results
branch
from
fc2301d to
27e8391
Compare
|
📦 Build Artifacts Available |
src/guidellm/__main__.py
Outdated
| type=click.Path(), | ||
| default=Path.cwd() / "benchmarks.json", | ||
| ) | ||
| def display(path): |
There was a problem hiding this comment.
display seems fine to me, but @markurtz may have a better keyword in mind.
|
📦 Build Artifacts Available |
There was a problem hiding this comment.
Pull Request Overview
Adds a “display” subcommand to reload and reprint an existing benchmark report file in the CLI.
- Introduces
print_full_reporton the console to batch‐print metadata, info, and stats. - Adds
display_benchmarks_reportentrypoint and wires it into__main__.pyas thedisplaycommand. - Includes unit tests and updates exports and pre-commit settings.
Reviewed Changes
Copilot reviewed 7 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/guidellm/benchmark/output.py | Added print_full_report to consolidate full report output |
| src/guidellm/benchmark/entrypoints.py | Defined display_benchmarks_report to load and show a file |
| src/guidellm/benchmark/init.py | Exported new display_benchmarks_report |
| src/guidellm/main.py | Added display CLI command |
| tests/unit/entrypoints/test_display_entrypoint.py | New tests for JSON/YAML report display |
| .pre-commit-config.yaml | Excluded assets directory from formatting hooks |
Comments suppressed due to low confidence (4)
src/guidellm/main.py:289
- [nitpick] The command name
displayis very generic. Consider renaming it to something likebenchmark-displayorbenchmarks:displayto avoid collisions and improve discoverability.
@cli.command(help="Redisplay a saved benchmark report.")
.pre-commit-config.yaml:6
- The regex
^tests/?.*/assets/.+may not correctly match nestedtests/unit/assetspaths. Consider using^tests/.+/assets/.+to reliably exclude asset files.
exclude: ^tests/?.*/assets/.+
src/guidellm/main.py:293
- The CLI uses
Pathhere but there is nofrom pathlib import Pathimport in this file. Please addfrom pathlib import Pathat the top.
default=Path.cwd() / "benchmarks.json",
src/guidellm/benchmark/entrypoints.py:138
- This function annotates
file: PathbutPathis not imported. Please addfrom pathlib import Path.
def display_benchmarks_report(file: Path):
There was a problem hiding this comment.
Thanks, @jaredoconnell; overall, the code looks good. I want to expand the functionality to encompass not only displaying the results but also reexporting them to a new file if the user desires.
Given that, I'd recommend something along the lines of the following for the CLI:
guidellm benchmark report PATH --output OUTPUT where output is optional. If an output file is supplied, we will resave the report to that file path using the extension as the file type. It could also potentially be named export, convert, or anything along those lines.
For the benchmark CLI pathways, it would then look like the following:
guidellm benchmark run ...
guidellm benchmark report ...
And if that ACTION after the benchmark is not supplied, we will default to filling it in as run. This way, we namespace all of the commands under benchmark and add flexibility towards the future.
|
📦 Build Artifacts Available |
|
This is ready for re-review. Since the last reviews, there is an option to re-export the benchmarks, and the command format was changed. The default command feature isn't supported by Click so I am using a class to handle the new behavior. I included some external code for the default command as opposed to doing that myself because I found that there were a lot of edge cases that broke the functionality. Should I document this command as Step 6 in the quick start? |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
|
The CI errors appears to be from using an older commit's code. That's very odd. |
closes #175
This adds a command to re-display a prior benchmarks file in the CLI.
Before marking this as ready for review, we need to decide what command format we want to use. During the call with Mark we discussed this being an option within the benchmark command.
Also, let me know if the stripped down results file is a good one to use. I manually removed data from a large results file.