Repository navigation
chore(e2e): overhaul nightly benchmark summary and trim model list - #392
Conversation
Rewrite nightly_summarize.py for better readability and richer output: - Add glossary explaining all metrics (TTFT, TPOT, E2E, p99, mean), traffic scenarios (D/N/E patterns), and comparison columns - Add auto-generated Key Findings executive summary at the top - Add TPOT (Time Per Output Token) to all summary sections — was parsed from JSON but mostly not displayed - Switch per-model summary and detail tables from mean to p99 metrics which better capture tail latency for production SLAs - Replace confusing +/- percentage format with clear "gRPC X%" / "HTTP X%" / "~" labels using normalized gRPC-advantage direction - Add matplotlib comparison chart generation (aggregate + per-model 2x2 grids of TTFT p99, TPOT p99, E2E p99, Output Throughput vs Concurrency) uploaded as separate artifact - Add error rate tracking section for runs with non-zero errors - Show all gRPC wins >30% instead of capped top-15 at >10% - Add TPOT p99 to aggregate metrics table Remove 5 models that don't add value to the benchmark: - Llama-3.2-1B-Instruct, Qwen2.5-14B-Instruct, DeepSeek-R1-Distill-Qwen-7B (this PR) - Mistral-7B, gpt-oss (removed from matrix + test file) Enable gateway debug logging during nightly benchmarks: - Make log_level and log_dir configurable on Gateway class (was hardcoded to --log-level warn with no --log-dir) - Plumb log_level/log_dir through pytest markers → gateway_config → gateway.start() in setup_backend.py - Set log_level="debug" and log_dir="nightly_gateway_logs" in the nightly test marker so logs are captured as artifacts Workflow changes: - Install matplotlib in summarize job for chart generation - Pass --charts-dir nightly_charts to generate comparison plots - Upload charts as separate artifact - Remove trimmed models from single/multi GPU matrices and TP override JSON Remaining benchmark models: Llama-3.1-8B, Qwen2.5-7B, Qwen3-30B-A3B (single+multi), Llama-4-Maverick (single only)
…cation - Define each metric once in a canonical registry (_M_TTFT_P99, etc.) and compose section-specific lists by reference instead of copy-pasting tuples across 6 separate lists - Merge PER_MODEL_SUMMARY_METRICS and PER_MODEL_DETAIL_METRICS into single PER_MODEL_METRICS used for both summary and detail tables - Replace _raw_pct() + _advantage() with single _advantage() that inlines the percentage calculation - Add _cp_advantage() and _avg_advantage() helpers to eliminate the repeated getattr(cp.grpc, fld) + _advantage() pattern (~15 call sites) - Unify _fmt_winner() and _fmt_winner_bold() into single function with bold parameter - Add _fmt_metric_value() for type-based value formatting, reuse in _section_by_concurrency instead of duplicated if/elif/else dispatch - Extract _group_by_concurrency() helper (was computed 3 times) - Extract _plot_comparison_grid() to deduplicate aggregate vs per-model chart generation (was ~40 lines of identical plotting code) - Add ComparisonPoint.config property to replace f-string duplication - Add SummaryResult dataclass for generate_summary() return type instead of bare tuple - Extract glossary text into _GLOSSARY_LINES constant - Remove unused experiments parameter from generate_charts() - Shorten glossary descriptions to satisfy 120-char line limit
📝 WalkthroughWalkthroughThe pull request reduces the nightly benchmark test matrix by removing several model entries across multiple frameworks, introduces chart generation and upload capabilities to the benchmark summarization workflow, refactors summary generation with a centralized metrics registry and new data structures, and adds logging configuration controls to the gateway backend setup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
Summary of ChangesHello @slin1237, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive overhaul of the nightly benchmark reporting system, significantly enhancing the clarity, detail, and visual presentation of performance metrics. By refining the benchmark model list, implementing advanced summary generation features, and improving underlying code quality, the changes aim to provide more actionable insights and a more efficient benchmarking workflow. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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
🤖 Fix all issues with AI agents
In `@e2e_test/benchmarks/nightly_summarize.py`:
- Around line 562-627: The summary in _section_key_findings uses max(grpc_wins,
http_wins) and min(...) when reporting "{winner} wins X/Y comparisons", which
can misattribute counts if the average direction (avg_adv) differs from the
majority; change it to pick counts based on the winner determined by avg_adv
(use winner_count = grpc_wins if avg_adv > 0 else http_wins and loser_count =
the other) and report winner_count/len(advs) and loser_count instead of max/min
to ensure wins/losses align with the declared winner.
🧹 Nitpick comments (2)
e2e_test/infra/gateway.py (1)
198-201: Allow explicit falsy overrides for log settings.
Truthy checks skip empty-string overrides and can retain prior values if a Gateway instance is reused; usingis not Nonemakes the override behavior explicit.Suggested tweak
- if log_level: - self.log_level = log_level - if log_dir: - self.log_dir = log_dir + if log_level is not None: + self.log_level = log_level + if log_dir is not None: + self.log_dir = log_dire2e_test/benchmarks/test_nightly_perf.py (1)
126-126: Consider per-model log directories to avoid collisions.
Using a singlenightly_gateway_logsfolder can overwrite logs across parametrized runs in the same workspace; scoping by model (and optionally backend) keeps artifacts distinct.Possible refactor
- _worker_count = worker_count + _worker_count = worker_count + safe_model_id = model_id.replace("/", "__") @@ - `@pytest.mark.gateway`(policy="round_robin", log_level="debug", log_dir="nightly_gateway_logs") + `@pytest.mark.gateway`( + policy="round_robin", + log_level="debug", + log_dir=f"nightly_gateway_logs/{safe_model_id}", + )
| def _section_key_findings( | ||
| comparisons: list[ComparisonPoint], | ||
| experiments: list[ExperimentInfo], | ||
| ) -> list[str]: | ||
| """Auto-generated executive summary of the benchmark results.""" | ||
| if not comparisons: | ||
| return [] | ||
|
|
||
| lines = ["### Key Findings", ""] | ||
|
|
||
| # 1. Overall verdict per key metric | ||
| for label, fld, lower_better, _unit in CHART_METRICS: | ||
| advs = [a for cp in comparisons if (a := _cp_advantage(cp, fld, lower_better)) is not None] | ||
| if not advs: | ||
| continue | ||
| avg_adv = sum(advs) / len(advs) | ||
| grpc_wins = sum(1 for a in advs if a > 2) | ||
| http_wins = sum(1 for a in advs if a < -2) | ||
| ties = len(advs) - grpc_wins - http_wins | ||
| if abs(avg_adv) < 1: | ||
| lines.append(f"- **{label}**: No clear winner — essentially tied across all scenarios") | ||
| else: | ||
| winner = "gRPC" if avg_adv > 0 else "HTTP" | ||
| lines.append( | ||
| f"- **{label}**: {winner} wins {max(grpc_wins, http_wins)}/{len(advs)} " | ||
| f"comparisons (avg {abs(avg_adv):.1f}% better), " | ||
| f"{min(grpc_wins, http_wins)} losses, {ties} ties" | ||
| ) | ||
|
|
||
| # 2. Error rates | ||
| total_runs = sum(len(e.runs) for e in experiments) | ||
| error_runs = [(e, r) for e in experiments for r in e.runs if r.error_rate > 0] | ||
| if error_runs: | ||
| lines.append(f"- **Errors**: {len(error_runs)}/{total_runs} runs had non-zero error rates") | ||
| else: | ||
| lines.append(f"- **Errors**: All {total_runs} runs completed with 0% error rate") | ||
|
|
||
| # 3. Biggest outliers | ||
| biggest_grpc_win = biggest_http_win = None | ||
| biggest_grpc_adv = biggest_http_adv = 0.0 | ||
| for cp in comparisons: | ||
| for label, fld, lower_better, _ in CHART_METRICS: | ||
| adv = _cp_advantage(cp, fld, lower_better) | ||
| if adv is not None and adv > biggest_grpc_adv: | ||
| biggest_grpc_adv = adv | ||
| biggest_grpc_win = (cp, label) | ||
| if adv is not None and adv < biggest_http_adv: | ||
| biggest_http_adv = adv | ||
| biggest_http_win = (cp, label) | ||
|
|
||
| if biggest_grpc_win and biggest_grpc_adv > 10: | ||
| cp, metric = biggest_grpc_win | ||
| lines.append( | ||
| f"- **Largest gRPC win**: {biggest_grpc_adv:.0f}% on {metric} " | ||
| f"— {cp.model} `{cp.scenario}` C={cp.concurrency}" | ||
| ) | ||
| if biggest_http_win and abs(biggest_http_adv) > 10: | ||
| cp, metric = biggest_http_win | ||
| lines.append( | ||
| f"- **Largest HTTP win**: {abs(biggest_http_adv):.0f}% on {metric} " | ||
| f"— {cp.model} `{cp.scenario}` C={cp.concurrency}" | ||
| ) | ||
|
|
||
| lines.append("") | ||
| return lines | ||
|
|
There was a problem hiding this comment.
Fix win/loss counts in key findings summary.
The message uses max/min of win counts, which can misattribute wins when the average winner differs from the majority; select counts based on the winner.
Proposed fix
- else:
- winner = "gRPC" if avg_adv > 0 else "HTTP"
- lines.append(
- f"- **{label}**: {winner} wins {max(grpc_wins, http_wins)}/{len(advs)} "
- f"comparisons (avg {abs(avg_adv):.1f}% better), "
- f"{min(grpc_wins, http_wins)} losses, {ties} ties"
- )
+ else:
+ winner = "gRPC" if avg_adv > 0 else "HTTP"
+ win_count = grpc_wins if avg_adv > 0 else http_wins
+ loss_count = http_wins if avg_adv > 0 else grpc_wins
+ lines.append(
+ f"- **{label}**: {winner} wins {win_count}/{len(advs)} "
+ f"comparisons (avg {abs(avg_adv):.1f}% better), "
+ f"{loss_count} losses, {ties} ties"
+ )🤖 Prompt for AI Agents
In `@e2e_test/benchmarks/nightly_summarize.py` around lines 562 - 627, The summary
in _section_key_findings uses max(grpc_wins, http_wins) and min(...) when
reporting "{winner} wins X/Y comparisons", which can misattribute counts if the
average direction (avg_adv) differs from the majority; change it to pick counts
based on the winner determined by avg_adv (use winner_count = grpc_wins if
avg_adv > 0 else http_wins and loser_count = the other) and report
winner_count/len(advs) and loser_count instead of max/min to ensure wins/losses
align with the declared winner.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly overhauls the nightly benchmark summary script, enhancing report readability, informativeness, and maintainability through refactoring, new features like chart generation and a "Key Findings" summary, and enabling gateway debug logging. A security audit identified two high-severity vulnerabilities in the nightly_summarize.py script: a Stored Cross-Site Scripting (XSS) vulnerability due to unsanitized data and an Arbitrary File Write vulnerability from unvalidated GITHUB_STEP_SUMMARY usage. Remediation by sanitizing all external data and validating file paths is strongly recommended. Additionally, there is a suggestion to improve the command-line argument parsing in the summary script for robustness.
| lines.append( | ||
| f"- **Largest gRPC win**: {biggest_grpc_adv:.0f}% on {metric} " | ||
| f"— {cp.model} `{cp.scenario}` C={cp.concurrency}" | ||
| ) |
There was a problem hiding this comment.
The script generates a markdown summary by embedding data (e.g., cp.model, cp.scenario) directly from parsed JSON benchmark files without sanitization. An attacker who can control the content of these JSON files can inject malicious HTML (<script> tags). This will be executed in the browser of anyone viewing the generated report, leading to a Stored XSS vulnerability. It is recommended to sanitize all data read from external files before embedding it into the report by using html.escape().
| with open(summary_file, "a") as f: | ||
| f.write(summary) | ||
| f.write(result.markdown) | ||
| f.write("\n") |
There was a problem hiding this comment.
The script reads the GITHUB_STEP_SUMMARY environment variable and uses its value as a file path to write the summary report. An attacker who can control this environment variable can cause the script to write to arbitrary files on the system, potentially leading to code execution or system compromise. While using GITHUB_STEP_SUMMARY is standard in GitHub Actions, it's crucial to validate the path to ensure it resides within an expected directory, especially if the execution environment can be influenced by external actors.
| args = sys.argv[1:] | ||
| base_dir = Path.cwd() | ||
| charts_dir: Path | None = None | ||
|
|
||
| i = 0 | ||
| while i < len(args): | ||
| if args[i] == "--charts-dir" and i + 1 < len(args): | ||
| charts_dir = Path(args[i + 1]) | ||
| i += 2 | ||
| elif not args[i].startswith("-"): | ||
| base_dir = Path(args[i]) | ||
| i += 1 | ||
| else: | ||
| i += 1 |
There was a problem hiding this comment.
The manual command-line argument parsing using sys.argv is a bit fragile. It might not handle unexpected arguments gracefully and lacks features like auto-generated help text. Using Python's built-in argparse module would make the script more robust and user-friendly.
You'll need to add import argparse at the top of the file.
parser = argparse.ArgumentParser(
description="Generate nightly benchmark summary.",
formatter_class=argparse.ArgumentDefaultsHelpFormatter,
)
parser.add_argument(
"base_dir",
nargs="?",
default=Path.cwd(),
type=Path,
help="Base directory containing benchmark results.",
)
parser.add_argument(
"--charts-dir",
type=Path,
default=None,
help="Directory to save generated charts.",
)
args = parser.parse_args()
base_dir = args.base_dir
charts_dir = args.charts_dir
1 participant
Summary
Overhauls the nightly benchmark summary script with better presentation, new sections, chart generation, and removes unused models from the benchmark matrix. Follows up on #381.
What changed
Benchmark model list trimmed
Summary script rewrite (
nightly_summarize.py)Code quality refactoring
getattr(cp.grpc, fld) + _advantage()patterns via_cp_advantage()/_avg_advantage()helpers_fmt_winner()withboldparam (was two nearly-identical functions)_plot_comparison_grid()for chart generation (was duplicated for aggregate vs per-model)_group_by_concurrency()helper (was computed 3 times)_fmt_metric_value()for type-based formatting (eliminates if/elif dispatch)SummaryResultdataclass instead of bare tuple returnComparisonPoint.configpropertyGateway debug logging
log_levelandlog_dirparameters to gateway start@pytest.mark.gateway(log_level="debug", log_dir="nightly_gateway_logs"))Test plan
python3 e2e_test/benchmarks/nightly_summarize.pyparses without errorsSummary by CodeRabbit
Release Notes
New Features
Improvements