Add interactivity plot metric (tok/s/user vs tok/s/GPU)#838
Conversation
Plot goodput-style curves using 1000/ITL for interactivity and system throughput per GPU, with --num-gpus since GPU count is not in result files. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughChangesThe evaluation utilities now support loading and transforming the Interactivity performance plotting
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The quality checks have failed. Please run |
mergify
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/evaluate/perf_utils.py (1)
209-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for the new interactivity pipeline.
This introduces several pure functions with meaningful branching (aggregate-vs-fallback TPS, CSV-vs-JSON-vs-artifact fallback chain, filtering non-positive ITL,
num_gpusvalidation). No test file was included in this review batch for this logic; given the multiple fallback paths and theValueErrorraised in_load_interactivity_csv, targeted unit tests (e.g. missingoutput_tps_meancolumn, missing artifacts dir, zero/negativeitl_ms) would be low effort and meaningfully de-risk this new code path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/evaluate/perf_utils.py` around lines 209 - 346, Add targeted unit tests for the interactivity pipeline, covering aggregate TPS with mean fallback, CSV loading and artifact/JSON fallback when output_tps_mean is absent, missing artifacts and invalid CSV raising ValueError, filtering non-positive ITL values, and transform_interactivity rejecting non-positive num_gpus. Anchor the tests to _system_tps_from_bench, _load_interactivity_csv, and transform_interactivity while preserving existing valid-path behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/evaluate/perf_utils.py`:
- Around line 209-223: Update _system_tps_from_bench to read GuideLLM 0.7.1’s
actual successful output token-total field instead of
output_token_count.successful.total_sum. Preserve the existing duration-based
aggregate calculation and fallback to output_tokens_per_second.successful.mean
when the token total is unavailable.
---
Nitpick comments:
In `@scripts/evaluate/perf_utils.py`:
- Around line 209-346: Add targeted unit tests for the interactivity pipeline,
covering aggregate TPS with mean fallback, CSV loading and artifact/JSON
fallback when output_tps_mean is absent, missing artifacts and invalid CSV
raising ValueError, filtering non-positive ITL values, and
transform_interactivity rejecting non-positive num_gpus. Anchor the tests to
_system_tps_from_bench, _load_interactivity_csv, and transform_interactivity
while preserving existing valid-path behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 383ab1e1-6f64-4767-8e47-183d35f196e1
📒 Files selected for processing (3)
docs/user_guide/tutorials/evaluating_performance.mdscripts/evaluate/perf_utils.pyscripts/evaluate/plot.py
| def _system_tps_from_bench(bench: dict) -> float | None: | ||
| """Aggregate output tokens/s for a benchmark (not per-request median).""" | ||
| metrics = bench.get("metrics", {}) | ||
| try: | ||
| out = metrics["output_token_count"]["successful"] | ||
| duration = float(bench["duration"]) | ||
| total = float(out["total_sum"]) | ||
| if duration > 0: | ||
| return total / duration | ||
| except (KeyError, TypeError, ValueError, ZeroDivisionError): | ||
| pass | ||
| try: | ||
| return float(metrics["output_tokens_per_second"]["successful"]["mean"]) | ||
| except (KeyError, TypeError, ValueError): | ||
| return None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does guidellm 0.7.1 GenerativeBenchmark JSON output include an "output_token_count" metric with a "total_sum" field under "successful"?
💡 Result:
No, the GuideLLM 0.7.1 GenerativeBenchmark JSON output does not include an output_token_count metric with a total_sum field under a successful key [1][2][3]. While GuideLLM records extensive metrics and statistics—including output_token_count as a measurement of generated tokens [4][3]—the JSON report structure organizes its data primarily by benchmark execution, metadata, and request-level statistics [1][5][6]. Specifically, the JSON schema typically includes per-benchmark metrics for successful requests, but it does not use a nested total_sum field within a successful section as described [1][3]. Statistics such as total token counts are generally derived from the collected data during analysis or provided in summary form, rather than being explicitly structured with a total_sum field in the output JSON [5][6][3].
Citations:
- 1: https://github.com/vllm-project/guidellm/blob/main/src/guidellm/benchmark/schemas/generative/report.py
- 2: https://github.com/vllm-project/guidellm/blob/main/docs/guides/outputs.md
- 3: https://developers.redhat.com/articles/2025/09/15/benchmarking-guidellm-air-gapped-openshift-clusters
- 4: https://github.com/vllm-project/guidellm/blob/main/docs/guides/metrics.md
- 5: https://pypi.org/project/guidellm/0.7.0/
- 6: https://github.com/vllm-project/guidellm/blob/main/README.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== perf_utils.py excerpt ==\n'
sed -n '200,235p' scripts/evaluate/perf_utils.py
printf '\n== references to output_token_count / output_tokens_per_second ==\n'
rg -n 'output_token_count|output_tokens_per_second|total_sum|successful' scripts/evaluate -S
printf '\n== file list around guidellm/schema mentions ==\n'
rg -n 'guidellm|generative/report|report.py|schema' -S .Repository: vllm-project/speculators
Length of output: 16665
Use the actual GuideLLM token-total field here The output_token_count.successful.total_sum lookup doesn’t match GuideLLM 0.7.1’s JSON schema, so this branch will always fall back to output_tokens_per_second.successful.mean and the aggregate system-throughput path never runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/evaluate/perf_utils.py` around lines 209 - 223, Update
_system_tps_from_bench to read GuideLLM 0.7.1’s actual successful output
token-total field instead of output_token_count.successful.total_sum. Preserve
the existing duration-based aggregate calculation and fallback to
output_tokens_per_second.successful.mean when the token total is unavailable.
|
I think the "per GPU" normalization isn't needed in this case since we aren't comparing different parallelism or GPU nodes. Just to simplify the process |
speculatorsbot
left a comment
speculatorsbot
left a comment
There was a problem hiding this comment.
Clean, well-scoped PR. The interactivity metric implementation follows existing patterns in perf_utils.py and plot.py consistently, and the CSV/JSON/artifact fallback chain handles backward compatibility correctly.
The main open item is @mgoin's design question about whether per-GPU normalization (and the --num-gpus flag it requires) is needed. Resolving that may simplify the implementation.
DCO sign-off: Both commits (ab78007, 0af154b) are missing the required Signed-off-by line. You can fix this with git rebase --signoff main before the next push.
Recommend approving once @mgoin's question is addressed and DCO is fixed.
🤖 Generated with Claude Code using the /pr-review skill
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews
🔴 Require approval from approved reviewers listWaiting for any of
This rule is failing.All pull requests must have at least one approving review from a member of the approved reviewers list before merging.
|
3 participants
Purpose
Plot goodput-style curves using 1000/ITL for interactivity and system throughput per GPU, with --num-gpus since GPU count is not in result files.
Tests
Checklist
I have filled in: