Repository navigation
Conversation
…eout Rewrite nightly_summarize.py to produce a comprehensive gRPC vs HTTP comparison report instead of raw per-config data tables. The new summary is written to GITHUB_STEP_SUMMARY and includes: - Aggregate comparison table (avg/median % diff per metric with winner) - Performance by concurrency level (TTFT, E2E, throughput breakdowns) - Win/loss scorecard at 1%/2%/5%/10% thresholds - Top 15 largest gRPC wins (>10% improvement) - Per-model summary table plus collapsible detail tables The overview table columns are now dynamically discovered from experiment data instead of hardcoded, so adding new runtimes (trtllm) or protocols (http for vllm) will automatically produce new columns and comparisons. Also fix _TIMEOUT_SEC in test_nightly_perf.py from 10800 (3h) to match the workflow timeout-minutes of 1440 (24h). The 3h timeout was causing premature SIGKILL of genai-bench processes. Files changed: - e2e_test/benchmarks/nightly_summarize.py: full rewrite with comparison logic, dynamic column discovery, _KNOWN_RUNTIMES set for extensibility - e2e_test/benchmarks/test_nightly_perf.py: _TIMEOUT_SEC = 1440 * 60
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive gRPC vs HTTP comparison reporting system for nightly benchmarks. The primary file is refactored to parse benchmark results, build cross-protocol comparisons, and generate detailed reports with aggregated metrics, per-concurrency breakdowns, and win/loss scorecards. A test timeout is increased to 24 hours to accommodate extended benchmark runs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 significantly upgrades the nightly benchmark reporting system by transforming raw data into insightful comparative analyses between gRPC and HTTP protocols. It introduces several new, detailed report sections that highlight performance differences, trends across concurrency levels, and specific wins, making it much easier to interpret benchmark results. Additionally, a critical timeout setting for benchmark runs has been corrected to prevent false failures and ensure complete data collection. Highlights
Changelog
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
|
![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 enhances the nightly benchmark summary by rewriting nightly_summarize.py to produce a detailed gRPC vs. HTTP performance comparison. The new report is well-structured with aggregate stats, per-concurrency breakdowns, and win/loss scorecards, which is a massive improvement for performance analysis. The code is well-organized and modular. I've added a few suggestions to further improve robustness and maintainability. The timeout fix in test_nightly_perf.py is also a welcome correction.
| runtime = "vllm" | ||
| # Fallback: detect runtime from folder name | ||
| folder_lower = folder.name.lower() | ||
| for rt in _KNOWN_RUNTIMES: |
There was a problem hiding this comment.
The current logic for detecting the runtime from the folder name could be brittle. Since the iteration order over a set is not guaranteed, this could lead to incorrect parsing if _KNOWN_RUNTIMES ever contains a name that is a substring of another (e.g., "foo" and "foobar"). To make this more robust, you can sort the known runtimes by length in descending order before iterating.
| for rt in _KNOWN_RUNTIMES: | |
| for rt in sorted(list(_KNOWN_RUNTIMES), key=len, reverse=True): |
| # TTFT mean | ||
| lines.extend( | ||
| [ | ||
| "<details>", | ||
| "<summary><b>TTFT Mean by Concurrency</b></summary>", | ||
| "", | ||
| "| Concurrency | gRPC avg | HTTP avg | Diff % | Winner |", | ||
| "|---:|---:|---:|---:|:---|", | ||
| ] | ||
| ) | ||
| for conc in conc_levels: | ||
| cps = by_conc[conc] | ||
| g_avg = sum(cp.grpc.ttft_mean * 1000 for cp in cps) / len(cps) | ||
| h_avg = sum(cp.http.ttft_mean * 1000 for cp in cps) / len(cps) | ||
| pct = _pct(g_avg, h_avg) | ||
| lines.append( | ||
| f"| {conc} | {g_avg:.0f}ms | {h_avg:.0f}ms | {_fmt_pct(pct)} | " | ||
| f"{_winner(pct, lower_is_better=True)} |" | ||
| ) | ||
| lines.extend(["", "</details>", ""]) | ||
|
|
||
| # E2E mean | ||
| lines.extend( | ||
| [ | ||
| "<details>", | ||
| "<summary><b>E2E Latency Mean by Concurrency</b></summary>", | ||
| "", | ||
| "| Concurrency | gRPC avg | HTTP avg | Diff % | Winner |", | ||
| "|---:|---:|---:|---:|:---|", | ||
| ] | ||
| ) | ||
| for conc in conc_levels: | ||
| cps = by_conc[conc] | ||
| g_avg = sum(cp.grpc.e2e_mean * 1000 for cp in cps) / len(cps) | ||
| h_avg = sum(cp.http.e2e_mean * 1000 for cp in cps) / len(cps) | ||
| pct = _pct(g_avg, h_avg) | ||
| lines.append( | ||
| f"| {conc} | {g_avg:.0f}ms | {h_avg:.0f}ms | {_fmt_pct(pct)} | " | ||
| f"{_winner(pct, lower_is_better=True)} |" | ||
| ) | ||
| lines.extend(["", "</details>", ""]) | ||
|
|
||
| # Output throughput | ||
| lines.extend( | ||
| [ | ||
| "<details>", | ||
| "<summary><b>Output Throughput by Concurrency</b></summary>", | ||
| "", | ||
| "| Concurrency | gRPC avg | HTTP avg | Diff % | Winner |", | ||
| "|---:|---:|---:|---:|:---|", | ||
| ] | ||
| ) | ||
| for conc in conc_levels: | ||
| cps = by_conc[conc] | ||
| g_avg = sum(cp.grpc.output_throughput for cp in cps) / len(cps) | ||
| h_avg = sum(cp.http.output_throughput for cp in cps) / len(cps) | ||
| pct = _pct(g_avg, h_avg) | ||
| lines.append( | ||
| f"| {conc} | {_fmt_throughput(g_avg)} tok/s | " | ||
| f"{_fmt_throughput(h_avg)} tok/s | {_fmt_pct(pct)} | " | ||
| f"{_winner(pct, lower_is_better=False)} |" | ||
| ) | ||
| lines.extend(["", "</details>", ""]) |
There was a problem hiding this comment.
There's significant code duplication in this function for generating the tables for TTFT, E2E latency, and Output Throughput. The structure of each table generation block is nearly identical. Consider refactoring this into a helper function to improve maintainability and reduce code size. The helper could take parameters like the table title, metric field name, and whether it's a latency or throughput metric.
| if lower_better: | ||
| if pct < -thresh: | ||
| grpc_w += 1 | ||
| elif pct > thresh: | ||
| http_w += 1 | ||
| else: | ||
| within += 1 | ||
| else: | ||
| if pct > thresh: | ||
| grpc_w += 1 | ||
| elif pct < -thresh: | ||
| http_w += 1 | ||
| else: | ||
| within += 1 |
There was a problem hiding this comment.
The nested if/else logic to count gRPC vs. HTTP wins can be simplified. You can first check if the percentage difference is within the threshold, and if not, use a single conditional expression to determine the winner based on the lower_better flag. This makes the logic more concise and easier to read.
if abs(pct) <= thresh:
within += 1
else:
# gRPC wins if pct is negative for latency, or positive for throughput
is_grpc_win = (pct < 0) if lower_better else (pct > 0)
if is_grpc_win:
grpc_w += 1
else:
http_w += 1![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 45-65: The grouping keys on ExperimentInfo currently omit hardware
details; update the ExperimentInfo.group_key and ExperimentInfo.table_key
properties to include gpu_type and gpu_count so experiments are not compared
across different GPUs or counts — modify the f-strings in the ExperimentInfo
class (properties group_key and table_key) to append gpu_type and gpu_count
(e.g., include them separated with '|' in group_key and with '_' in table_key)
so grouping and table columns incorporate both GPU type and GPU count.
🧹 Nitpick comments (1)
e2e_test/benchmarks/nightly_summarize.py (1)
307-364: Consider flagging error_rate in the overview status.
A run with non-zero error_rate but non-zero throughput would still show ✅; including error_rate improves the signal.♻️ Suggested tweak
- has_errors = any(r.rps == 0 or r.output_throughput == 0 for r in exp.runs) + has_errors = any( + r.error_rate > 0 or r.rps == 0 or r.output_throughput == 0 + for r in exp.runs + )
Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
…red`` + debug dump The xfail helper was written against an old TokenSpeed main where ``create_grammar_backend()`` was defined but never called and ``Request.grammar`` was never assigned. That's no longer the case — TokenSpeed's grammar pipeline is fully wired as of #236 + #381 + #395: - ``request_handler.py`` instantiates ``GrammarManager`` - ``event_loop.py`` routes new requests through ``process_req_with_grammar`` + ``get_ready_grammar_requests``, with an async compile queue and abort support - ``greedy.py`` / ``flashinfer*.py`` both apply ``sampling_info.vocab_mask`` to logits before sampling when a grammar is active - speculative (``eagle_utils.py:773``) and disagg decode paths already call ``grammar.accept_token`` So ``tokenspeed#361``'s surface (``sampling_params.json_schema`` → ``req.grammar`` → vocab mask) now works end-to-end. The three ``tool_choice=required/specific`` tests that were xfailed should run on tokenspeed just like they do on sglang/vllm/trtllm; CI will confirm. Also drops the short-lived debug dumps in ``servicer.py`` and the ``TOKENSPEED_DEBUG_OUTPUT=1`` toggle in ``e2e-gpu-job.yml`` — those were staged to diagnose why meta-llama tool-call tests diverged from unsloth. The diagnosis is done: output trimming is correct, chunks are incremental, the remaining unconstrained (tool_choice=auto) drift is a sampling-variance quality issue orthogonal to this PR. Signed-off-by: yetone <yetoneful@gmail.com>
…red`` + debug dump The xfail helper was written against an old TokenSpeed main where ``create_grammar_backend()`` was defined but never called and ``Request.grammar`` was never assigned. That's no longer the case — TokenSpeed's grammar pipeline is fully wired as of #236 + #381 + #395: - ``request_handler.py`` instantiates ``GrammarManager`` - ``event_loop.py`` routes new requests through ``process_req_with_grammar`` + ``get_ready_grammar_requests``, with an async compile queue and abort support - ``greedy.py`` / ``flashinfer*.py`` both apply ``sampling_info.vocab_mask`` to logits before sampling when a grammar is active - speculative (``eagle_utils.py:773``) and disagg decode paths already call ``grammar.accept_token`` So ``tokenspeed#361``'s surface (``sampling_params.json_schema`` → ``req.grammar`` → vocab mask) now works end-to-end. The three ``tool_choice=required/specific`` tests that were xfailed should run on tokenspeed just like they do on sglang/vllm/trtllm; CI will confirm. Also drops the short-lived debug dumps in ``servicer.py`` and the ``TOKENSPEED_DEBUG_OUTPUT=1`` toggle in ``e2e-gpu-job.yml`` — those were staged to diagnose why meta-llama tool-call tests diverged from unsloth. The diagnosis is done: output trimming is correct, chunks are incremental, the remaining unconstrained (tool_choice=auto) drift is a sampling-variance quality issue orthogonal to this PR. Signed-off-by: yetone <yetoneful@gmail.com>
…red`` + debug dump The xfail helper was written against an old TokenSpeed main where ``create_grammar_backend()`` was defined but never called and ``Request.grammar`` was never assigned. That's no longer the case — TokenSpeed's grammar pipeline is fully wired as of #236 + #381 + #395: - ``request_handler.py`` instantiates ``GrammarManager`` - ``event_loop.py`` routes new requests through ``process_req_with_grammar`` + ``get_ready_grammar_requests``, with an async compile queue and abort support - ``greedy.py`` / ``flashinfer*.py`` both apply ``sampling_info.vocab_mask`` to logits before sampling when a grammar is active - speculative (``eagle_utils.py:773``) and disagg decode paths already call ``grammar.accept_token`` So ``tokenspeed#361``'s surface (``sampling_params.json_schema`` → ``req.grammar`` → vocab mask) now works end-to-end. The three ``tool_choice=required/specific`` tests that were xfailed should run on tokenspeed just like they do on sglang/vllm/trtllm; CI will confirm. Also drops the short-lived debug dumps in ``servicer.py`` and the ``TOKENSPEED_DEBUG_OUTPUT=1`` toggle in ``e2e-gpu-job.yml`` — those were staged to diagnose why meta-llama tool-call tests diverged from unsloth. The diagnosis is done: output trimming is correct, chunks are incremental, the remaining unconstrained (tool_choice=auto) drift is a sampling-variance quality issue orthogonal to this PR. Signed-off-by: yetone <yetoneful@gmail.com>
…red`` + debug dump The xfail helper was written against an old TokenSpeed main where ``create_grammar_backend()`` was defined but never called and ``Request.grammar`` was never assigned. That's no longer the case — TokenSpeed's grammar pipeline is fully wired as of #236 + #381 + #395: - ``request_handler.py`` instantiates ``GrammarManager`` - ``event_loop.py`` routes new requests through ``process_req_with_grammar`` + ``get_ready_grammar_requests``, with an async compile queue and abort support - ``greedy.py`` / ``flashinfer*.py`` both apply ``sampling_info.vocab_mask`` to logits before sampling when a grammar is active - speculative (``eagle_utils.py:773``) and disagg decode paths already call ``grammar.accept_token`` So ``tokenspeed#361``'s surface (``sampling_params.json_schema`` → ``req.grammar`` → vocab mask) now works end-to-end. The three ``tool_choice=required/specific`` tests that were xfailed should run on tokenspeed just like they do on sglang/vllm/trtllm; CI will confirm. Also drops the short-lived debug dumps in ``servicer.py`` and the ``TOKENSPEED_DEBUG_OUTPUT=1`` toggle in ``e2e-gpu-job.yml`` — those were staged to diagnose why meta-llama tool-call tests diverged from unsloth. The diagnosis is done: output trimming is correct, chunks are incremental, the remaining unconstrained (tool_choice=auto) drift is a sampling-variance quality issue orthogonal to this PR. Signed-off-by: yetone <yetoneful@gmail.com>
1 participant
Summary
nightly_summarize.pyto produce a comprehensive gRPC vs HTTP comparison report for the GitHub Actions summary page, replacing the previous raw-data-only tables_TIMEOUT_SECintest_nightly_perf.pyfrom 3h to match workflow's 24h timeoutWhat changed
e2e_test/benchmarks/nightly_summarize.py— full rewrite:trtllm) or protocols (e.g. HTTP vLLM) automatically produces new columns and comparisons_KNOWN_RUNTIMESset: single place to register new runtimes (sglang,vllm,trtllm)(model, runtime, worker_type, scenario, concurrency), filtering out error-rate > 0e2e_test/benchmarks/test_nightly_perf.py:_TIMEOUT_SECfrom10800(3h) to1440 * 60(24h) to matchtimeout-minutes: 1440in the workflow. The 3h limit was causing premature SIGKILL of genai-bench subprocesses.Why
The previous summary page only showed raw data tables per model/config — no comparison between gRPC and HTTP, making it hard to draw conclusions without downloading artifacts and analyzing offline. The new summary surfaces the same analysis that was previously done manually.
The timeout fix prevents false failures where genai-bench is killed after 3h even though the workflow allows 24h.
Test plan
nightly_summarize.pyagainst downloaded artifacts from run #21807599943 — produces 495 matched comparison points, all sections render correctlySummary by CodeRabbit
Release Notes