Improve error visibility when printing metrics #234
Conversation
| @@ -508,6 +508,7 @@ when `--output-tokens-mean` is provided. (default: `0`) | |||
| The seed used to generate random values. (default: `0`) | |||
|
|
|||
| ##### `--request-count <int>` | |||
| ##### `--num-requests <int>` | |||
There was a problem hiding this comment.
Added aliases. While PA uses request-count, all other GAP args that use counts start with --num (most notably, --num-prompts).
We can decide for GA what arg name to use (if only one), but having this consistency would make it less confusing to be adding args. I often would type out the wrong one, so I imagine some users might too.
There was a problem hiding this comment.
This was the previous name right?
An alias is fine, just having deja vu here.
Update function descriptions Remove unnecessary fix Remove print
dyastremsky
force-pushed
the
dyas-error-visibility
branch
from
bee6022 to
74da9a5
Compare
| from rich.console import Console | ||
| from rich.table import Table | ||
|
|
||
| from . import exporter_utils |
There was a problem hiding this comment.
Switching to relative imports for conciseness. This will also make any future refactors easier, so that we don't need to update the path if files are reorganized.
genai-perf/genai_perf/export_data/telemetry_data_exporter_util.py
Outdated
Show resolved
Hide resolved
| "│ Output token throughput (per sec) │ 456… │ N/A │ N/A │ N/A │ N/A │ N/A │\n" | ||
| "│ Request throughput (per sec) │ 123… │ N/A │ N/A │ N/A │ N/A │ N/A │\n" | ||
| "│ Request count (count) │ 3.00 │ N/A │ N/A │ N/A │ N/A │ N/A │\n" | ||
| "│ Time To First Token (ms) │ 8.00 │ 7.00 │ 9.00 │ 8.98 │ 8.80 │ 8.50 │\n" |
There was a problem hiding this comment.
The CSV and console exporters format the headers differently. When refactoring the formatting to a common utils class, I kept the title-style formatting.
dyastremsky
requested review from
debermudez,
nv-hwoo and
ganeshku1
and removed request for
debermudez
| @@ -508,6 +508,7 @@ when `--output-tokens-mean` is provided. (default: `0`) | |||
| The seed used to generate random values. (default: `0`) | |||
|
|
|||
| ##### `--request-count <int>` | |||
| ##### `--num-requests <int>` | |||
There was a problem hiding this comment.
This was the previous name right?
An alias is fine, just having deja vu here.
|
Nice work on this! |
@debermudez Not as far as I know. I wasn't involved in the initial request count work, so it's possible that was the case. This does seem like the more consistent name on the GenAI-Perf name, though I know we try to reuse the PA flag names in GenAI-Perf. |
When a metric error happens, it can be hard to debug whether something went wrong with the server, perf analyzer, or GenAI-Perf (e.g. edge condition, bug). These errors have been reported for different situations, and debugging them has been opaque.
This pull request catches common errors when accessing the statistics dictionary and logs them as errors. This allows the output to still be printed (with N/A) to not lose the data already gathered/processed but also alerts the user what the specific issue is.
Example screenshot with fake "p10" statistic added to trigger error.
