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 |
|
📦 Build Artifacts Available |
markurtz
left a comment
markurtz
left a comment
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. |
There was a problem hiding this comment.
Pull Request Overview
Adds a command and related infrastructure to load and display an existing benchmark report via the CLI.
- Introduces
reimport_benchmarks_reportand wires it into a newguidellm benchmark from-filesubcommand - Adds
print_full_reporttoGenerativeBenchmarksConsoleand updates the mainbenchmarkgroup to useDefaultGroupHandler - Includes unit tests/assets for re-display behavior and updates docs to reference the new
runsubcommand
Reviewed Changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/guidellm/benchmark/entrypoints.py | Added reimport_benchmarks_report and simplified console calls |
| src/guidellm/benchmark/output.py | Added print_full_report, updated unsupported-extension error |
| src/guidellm/main.py | Converted benchmark to a subcommand group with run & from-file |
| src/guidellm/utils/default_group.py | Added DefaultGroupHandler to support default subcommands |
| tests/unit/entrypoints/.../test_benchmark_from_file_entrypoint.py | New tests for display and re-export |
| docs/outputs.md | Updated examples to use benchmark run |
| docs/datasets.md | Updated examples to use benchmark run |
| README.md | Updated examples to use benchmark run |
| .pre-commit-config.yaml | Excluded test assets from formatting rules |
Comments suppressed due to low confidence (3)
src/guidellm/main.py:36
- To automatically execute
runwhen users invokeguidellm benchmarkwithout any args, adddefault_if_no_args=Trueto theDefaultGroupHandlerparameters.
)
src/guidellm/benchmark/entrypoints.py:154
- [nitpick] This docstring ends abruptly at "Can also specify". Please complete it to explain what additional options are available when re-importing a report.
existing benchmarks report. Can also specify
docs/outputs.md:3
- [nitpick] The new
benchmark from-filecommand isn’t mentioned here. Consider adding a section with usage examples forguidellm benchmark from-fileto show how to re-display existing reports.
GuideLLM provides flexible options for outputting benchmark results, catering to both console-based summaries and file-based detailed reports. This document outlines the supported output types, their configurations, and how to utilize them effectively.
|
📦 Build Artifacts Available |
|
📦 Build Artifacts Available |
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. --------- Co-authored-by: Samuel Monson <[email protected]> Co-authored-by: Mark Kurtz <[email protected]>
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. --------- Co-authored-by: Samuel Monson <[email protected]> Co-authored-by: Mark Kurtz <[email protected]> Signed-off-by: dalthecow <[email protected]>
3 participants
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.