Repository navigation
ci(nightly): Add vLLM HTTP support to nightly benchmarks - #502
Conversation
Summary of ChangesHello @CatherineSue, 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 addresses the limitation of vLLM nightly benchmarks being restricted to gRPC, by integrating HTTP protocol support. The changes introduce the necessary infrastructure to launch and manage vLLM HTTP workers, ensuring that both HTTP and gRPC protocols are now benchmarked for all vLLM runtimes. This significantly broadens the scope of performance evaluation for vLLM models. 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
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds HTTP vLLM worker launch and routing in ModelPool, centralizes vLLM server command construction, unifies subprocess spawning, adjusts HTTP vLLM health-check flow to use /health fallback, and updates CI benchmark workflow to run vLLM over both HTTP and gRPC with a PR-aware BENCH_TEST_MODE. Changes
Sequence DiagramsequenceDiagram
participant TestRunner as Test Runner
participant ModelPool as ModelPool
participant Launcher as vLLM_HTTP_Launcher
participant ProcMgr as ProcessManager
participant Worker as vLLM_HTTP_Worker
participant Health as HealthCheck
TestRunner->>ModelPool: launch_model(mode="http", backend="vllm")
ModelPool->>ModelPool: detect HTTP + vLLM
ModelPool->>Launcher: _launch_vllm_http_worker(model_spec,...)
Launcher->>Launcher: _build_vllm_cmd(...)
Launcher->>ProcMgr: _spawn_worker_process(cmd, env, key, port)
ProcMgr->>Worker: start subprocess (HTTP vLLM)
Launcher->>Health: wait for readiness (health_wait)
Health->>Worker: GET /health (skip deep /health_generate)
Worker-->>Health: 200 OK
Health-->>Launcher: ready
Launcher->>ModelPool: return ModelInstance (_skip_deep_health_check=True)
ModelPool-->>TestRunner: ModelInstance ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
Comment |
CatherineSue
changed the title
![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 successfully adds HTTP support for vLLM to the nightly benchmarks, which is a great enhancement for performance tracking. The changes are logical, including the introduction of a shared command builder for vLLM and a dedicated launcher for the HTTP worker. My review includes a couple of suggestions to improve the code's robustness and maintainability by refining error handling and addressing significant code duplication in the worker launching logic.
| instance = self._launch_vllm_http_worker( | ||
| model_id=model_id, | ||
| model_spec=spec, | ||
| gpu_slot=gpu_slot, | ||
| startup_timeout=600, | ||
| instance_key=instance_key, | ||
| ) | ||
| assert instance is not None | ||
| return instance |
There was a problem hiding this comment.
Using assert instance is not None can be problematic as it will raise an AssertionError on failure. Assertions can be disabled in production environments (e.g., with Python's -O flag) and are generally less descriptive for handling expected failure modes like a worker failing to launch. It would be more robust to explicitly check for None and raise a RuntimeError with a clear message. This pattern is also present for the gRPC worker launch and could be improved there as well for consistency.
| instance = self._launch_vllm_http_worker( | |
| model_id=model_id, | |
| model_spec=spec, | |
| gpu_slot=gpu_slot, | |
| startup_timeout=600, | |
| instance_key=instance_key, | |
| ) | |
| assert instance is not None | |
| return instance | |
| instance = self._launch_vllm_http_worker( | |
| model_id=model_id, | |
| model_spec=spec, | |
| gpu_slot=gpu_slot, | |
| startup_timeout=600, | |
| instance_key=instance_key, | |
| ) | |
| if instance is None: | |
| raise RuntimeError(f"Failed to launch vLLM HTTP worker for {model_id}") | |
| return instance |
| def _launch_vllm_http_worker( | ||
| self, | ||
| model_id: str, | ||
| model_spec: dict, | ||
| gpu_slot: GPUSlot, | ||
| startup_timeout: int, | ||
| instance_key: str | None = None, | ||
| ) -> ModelInstance | None: | ||
| """Launch a vLLM HTTP worker. | ||
|
|
||
| Args: | ||
| model_id: Model identifier. | ||
| model_spec: Model specification dict from MODEL_SPECS. | ||
| gpu_slot: GPU slot assignment. | ||
| startup_timeout: Timeout for worker to become healthy. | ||
| instance_key: Custom instance key, or None to auto-generate. | ||
|
|
||
| Returns: | ||
| The launched ModelInstance, or None if launch fails. | ||
| """ | ||
| model_path = model_spec["model"] | ||
| tp_size = model_spec.get("tp", 1) | ||
| port = gpu_slot.port | ||
| assert port is not None | ||
|
|
||
| env = os.environ.copy() | ||
| env["CUDA_VISIBLE_DEVICES"] = gpu_slot.cuda_visible_devices() | ||
|
|
||
| cmd = self._build_vllm_http_cmd(model_path, DEFAULT_HOST, port, tp_size, model_spec) | ||
|
|
||
| key = instance_key or f"{model_id}:vllm-http" | ||
| logger.info( | ||
| "Launching vLLM HTTP worker %s on GPUs %s port %d", | ||
| key, | ||
| gpu_slot.gpu_ids, | ||
| port, | ||
| ) | ||
|
|
||
| show_output = os.environ.get(ENV_SHOW_WORKER_LOGS, "0") == "1" | ||
|
|
||
| stdout_target: int | IO[Any] | None = None | ||
| stderr_target: int | IO[Any] | None = None | ||
| if not show_output: | ||
| if self.log_dir: | ||
| os.makedirs(self.log_dir, exist_ok=True) | ||
| safe_key = key.replace("/", "__").replace(":", "_") | ||
| log_file = open(os.path.join(self.log_dir, f"worker-{safe_key}-{port}.log"), "w") | ||
| self._log_files[key] = log_file | ||
| stdout_target = log_file | ||
| stderr_target = subprocess.STDOUT | ||
| else: | ||
| stdout_target = subprocess.DEVNULL | ||
| stderr_target = subprocess.DEVNULL | ||
|
|
||
| try: | ||
| proc = subprocess.Popen( | ||
| cmd, | ||
| env=env, | ||
| stdout=stdout_target, | ||
| stderr=stderr_target, | ||
| start_new_session=True, | ||
| ) | ||
| except Exception: | ||
| lf = self._log_files.pop(key, None) | ||
| if lf is not None: | ||
| lf.close() | ||
| raise | ||
|
|
||
| base_url = f"http://{DEFAULT_HOST}:{port}" | ||
| instance = ModelInstance( | ||
| model_id=model_id, | ||
| mode=ConnectionMode.HTTP, | ||
| model_path=model_path, | ||
| base_url=base_url, | ||
| port=port, | ||
| process=proc, | ||
| gpu_slot=gpu_slot, | ||
| key=key, | ||
| worker_type=WorkerType.REGULAR, | ||
| bootstrap_port=None, | ||
| last_used=time.time(), | ||
| _skip_deep_health_check=True, | ||
| ) | ||
| self.instances[key] = instance | ||
|
|
||
| try: | ||
| self._wait_worker_healthy(instance, startup_timeout) | ||
| return instance | ||
| except Exception as e: | ||
| logger.error("Failed to start vLLM HTTP worker %s: %s", key, e) | ||
| self._evict_instance(key) | ||
| return None | ||
|
|
There was a problem hiding this comment.
The new _launch_vllm_http_worker method contains a significant amount of code that is also present in _launch_grpc_worker and the SGLang worker launch logic within _launch_model. This duplicated logic includes setting up logging, creating the subprocess, instantiating ModelInstance, and waiting for the worker to become healthy.
To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, I recommend refactoring this common logic into a shared private helper method. This would centralize the worker launching boilerplate, making the code cleaner and easier to manage in the future.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cd76979d8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| cmd = self._build_vllm_http_cmd(model_path, DEFAULT_HOST, port, tp_size, model_spec) | ||
|
|
||
| key = instance_key or f"{model_id}:vllm-http" |
There was a problem hiding this comment.
The default key f"{model_id}:vllm-http" diverges from the regular-worker key format (f"{model_id}:{mode.value}") that _get_unlocked and _wait_for_instance use. When a vLLM HTTP worker is launched on-demand (for example, after startup deferred it due to GPU pressure), the instance is stored under :vllm-http but the caller waits for :http, producing a KeyError and failing backend setup even though the process started.
Useful? React with 👍 / 👎.
| variant: | ||
| - { id: sglang, runtime: sglang, grpc_only: "false", setup_vllm: false, setup_trtllm: false, extra_deps: "genai-bench" } | ||
| - { id: vllm, runtime: vllm, grpc_only: "true", setup_vllm: true, setup_trtllm: false, extra_deps: "genai-bench" } | ||
| - { id: vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm: false, extra_deps: "genai-bench" } |
There was a problem hiding this comment.
Flipping this matrix row to grpc_only: "false" makes the single-worker-h200 Llama-4 vLLM run require both protocols, but that model is TP=8 on an 8-GPU runner, so one protocol is necessarily deferred and launched via ModelPool.get(). In that on-demand path, vLLM gRPC workers are keyed as <model>:vllm-grpc while get() waits on <model>:grpc, so the second protocol launch fails; this workflow change therefore introduces a consistently failing nightly configuration.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/nightly-benchmark.yml:
- Line 95: The inline YAML maps with entries like "{ id: vllm, runtime: vllm,
grpc_only: "false", setup_vllm: true, setup_trtllm: false }" have extra spaces
inside braces and should be normalized for yamllint; edit each offending inline
map (the ones containing keys id, runtime, grpc_only, setup_vllm, setup_trtllm)
to use a single space after the opening brace, a single space after each comma,
and a single space before the closing brace (e.g. "{ id: vllm, runtime: vllm,
grpc_only: "false", setup_vllm: true, setup_trtllm: false }"), and apply the
same spacing normalization to the other two occurrences.
In `@e2e_test/infra/model_pool.py`:
- Around line 1435-1487: The default instance key for vLLM HTTP workers is
inconsistent with HTTP lookups; change the key construction in
_launch_vllm_http_worker from key = instance_key or f"{model_id}:vllm-http" to
use the HTTP mode value (e.g., key = instance_key or
f"{model_id}:{ConnectionMode.HTTP.value}") so it matches how on-demand code and
_wait_for_instance(key) expect the key; update any derived names (safe_key) that
use key so they remain consistent.
| variant: | ||
| - { id: sglang, runtime: sglang, grpc_only: "false", setup_vllm: false, setup_trtllm: false } | ||
| - { id: vllm, runtime: vllm, grpc_only: "true", setup_vllm: true, setup_trtllm: false } | ||
| - { id: vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm: false } |
There was a problem hiding this comment.
Fix YAML inline map spacing to satisfy yamllint.
YAMLlint reports “too many spaces inside braces” at Line 95, Line 202, and Line 307. Normalize spacing in the inline maps.
Suggested fix
- - { id: vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm: false }
+ - { id: vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm: false }
@@
- - { id: vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm: false, extra_deps: "genai-bench" }
+ - { id: vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm: false, extra_deps: "genai-bench" }
@@
- - { id: vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm: false, extra_deps: "genai-bench" }
+ - { id: vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm: false, extra_deps: "genai-bench" }Also applies to: 202-202, 307-307
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 95-95: too many spaces inside braces
(braces)
[error] 95-95: too many spaces inside braces
(braces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/nightly-benchmark.yml at line 95, The inline YAML maps
with entries like "{ id: vllm, runtime: vllm, grpc_only: "false",
setup_vllm: true, setup_trtllm: false }" have extra spaces inside braces and
should be normalized for yamllint; edit each offending inline map (the ones
containing keys id, runtime, grpc_only, setup_vllm, setup_trtllm) to use a
single space after the opening brace, a single space after each comma, and a
single space before the closing brace (e.g. "{ id: vllm, runtime: vllm,
grpc_only: "false", setup_vllm: true, setup_trtllm: false }"), and apply the
same spacing normalization to the other two occurrences.
Signed-off-by: Chang Su <chang.s.su@oracle.com>
0cd7697 to
64054e3
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/nightly-benchmark.yml:
- Line 95: Normalize spacing inside the inline YAML maps (e.g., the map starting
with "{ id: vllm, runtime: vllm, grpc_only: "false", setup_vllm: true,
setup_trtllm: false }") by removing extra spaces after "{" and before "}" and
collapsing multiple spaces between entries to single spaces so it becomes "{id:
vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm:
false}"; apply the same normalization to the other inline maps flagged (the
entries at the same pattern on the other lines referenced).
In `@e2e_test/infra/model_pool.py`:
- Around line 1435-1487: The instance key default currently uses
f"{model_id}:vllm-http" which mismatches lookup keys; change the default key
computation in the vLLM HTTP launch path (where instance_key and key are set) to
use f"{model_id}:{ConnectionMode.HTTP.value}" so it matches lookups made by
_wait_for_instance and other code that expects ConnectionMode.HTTP.value; ensure
any places that populate self._log_files or pop by key (and the
ModelInstance.key field) use this corrected key.
…s launch - Extract _spawn_worker_process() helper on ModelPool that handles output routing (log file / DEVNULL / terminal) and subprocess.Popen with log file cleanup on failure. Replaces ~30 lines of identical code in _launch_model, _launch_grpc_worker, and _launch_vllm_http_worker. - Remove _build_vllm_http_cmd() — trivial 1-line wrapper around _build_vllm_cmd. Inline the _build_vllm_cmd call directly in _launch_vllm_http_worker, consistent with how _build_grpc_cmd already calls _build_vllm_cmd. - Move `import select` to module top and remove redundant inline `import os` / `import select` in _wait_all_healthy. No behavioral changes — pure mechanical extraction. Signed-off-by: Simon Lin <simon@simonlin.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/infra/model_pool.py`:
- Around line 555-573: Replace the bare assertion after launching the vLLM HTTP
worker with explicit error handling: check the return of
self._launch_vllm_http_worker (called in the HTTP + is_vllm() branch) and raise
a descriptive RuntimeError if it returned None (include model_id, instance_key
and information that vLLM HTTP worker launch failed) instead of using assert;
follow the same pattern used by get_grpc_worker for error message and flow so
failures are explicit and debuggable.
---
Duplicate comments:
In `@e2e_test/infra/model_pool.py`:
- Line 1401: The default instance key assignment uses f"{model_id}:vllm-http"
which mismatches the HTTP mode key used by on-demand lookups; update the default
for instance_key (the variable set in the key assignment) to use the same HTTP
suffix used by _wait_for_instance and on-demand launches (so _get_unlocked
calling _launch_model without instance_key will generate the key that
_wait_for_instance expects), ensuring consistency between _get_unlocked,
_launch_model, and _wait_for_instance.
- Add pull_request trigger to nightly-benchmark.yml, filtered to e2e_test/** and the workflow file itself - Set BENCH_TEST_MODE env var at workflow level: "1" for PR runs, "0" for nightly cron and manual dispatch - Read BENCH_TEST_MODE in test_nightly_perf.py instead of hardcoded _TEST_MODE = False. When enabled, benchmarks use 1 concurrency, D(100,100) scenario, and 10 max requests for a fast smoke test Signed-off-by: Simon Lin <simon@simonlin.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/nightly-benchmark.yml:
- Line 101: Fix the YAML inline map spacing for the entries that contain the
vllm map (the inline map that currently reads like "{ id: vllm, runtime: vllm,
grpc_only: "false", setup_vllm: true, setup_trtllm: false }"); remove the extra
spaces inside the braces so keys follow a single space after commas e.g. "{id:
vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm:
false}", and apply the same spacing correction to the other identical inline
maps in the file that reference id: vllm.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f12f00401
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| pull_request: | ||
| paths: | ||
| - e2e_test/** | ||
| - .github/workflows/nightly-benchmark.yml |
There was a problem hiding this comment.
Adding pull_request as a trigger here makes PR runs share the existing workflow-level concurrency group nightly-benchmark (same file, concurrency.group), and with cancel-in-progress: false each PR run now queues ahead of later runs. In periods with multiple PRs, scheduled nightly runs can be delayed indefinitely, so the workflow no longer reliably runs on its nightly cadence even though this change intends PR mode to be a lightweight addition.
Useful? React with 👍 / 👎.
- Fix instance key mismatch in _launch_model dispatch: vLLM HTTP and gRPC worker launchers used non-canonical keys (e.g. "model:vllm-http" instead of "model:http"), causing KeyError when _get_unlocked tried to find the instance after on-demand launch. Now computes effective_key matching the canonical format before dispatching. - Replace assert statements with explicit RuntimeError in _launch_model dispatch paths for gpu_slot and instance None checks. - Fix concurrency group collision: PR runs now use a separate group (nightly-benchmark-pull_request) so they don't block nightly cron runs. - Increase test mode timeout from 300s to 600s: genai-bench Docker container needs to download model tokenizers from HuggingFace on first run, which exceeds 300s for larger models (e.g. Qwen3-30B). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/nightly-benchmark.yml:
- Line 101: The inline YAML mapping for the vLLM benchmark entry has
inconsistent spacing inside the brace-delimited map (the { id: vllm, runtime:
vllm, grpc_only: "false", setup_vllm: true, setup_trtllm: false } entry);
normalize spacing so commas and colons have single spaces (i.e., consistent
"key: value" and single space after each comma) for the entry with id: vllm, and
apply the same normalization to the other inline mappings for vllm found
elsewhere in the file (the other id: vllm occurrences).
In `@e2e_test/infra/model_pool.py`:
- Line 1409: The fallback key uses a hardcoded "vllm-http" string which is
inconsistent with the canonical key format; update the assignment in
model_pool.py so that when instance_key is falsy it builds the fallback using
the enum value (i.e., use ConnectionMode.HTTP.value) instead of the literal
"vllm-http" — change the expression that sets key (currently `key = instance_key
or f"{model_id}:vllm-http"`) to use f"{model_id}:{ConnectionMode.HTTP.value}" so
it matches how `_launch_model`/effective_key are formed.
genai-bench Docker containers were downloading model tokenizers from HuggingFace on every run, taking >600s on some CI runners and causing timeout failures. When ROUTER_LOCAL_MODEL_PATH is set and the model directory exists on disk (e.g. /raid/models/Qwen/Qwen3-30B-A3B), use the local path for --model-tokenizer instead of the HF identifier. The local model directory is already volume-mounted into the container. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Add SGLang vs vLLM comparison alongside existing gRPC vs HTTP
comparison in the nightly benchmark summary report. With vLLM HTTP
support added, the summary now covers both comparison axes:
protocol (gRPC vs HTTP) and runtime (SGLang vs vLLM).
What changed:
- e2e_test/benchmarks/nightly_summarize.py:
- Add RuntimeComparisonPoint dataclass for cross-runtime data points
- Add build_runtime_comparisons() to match runs across runtimes
for the same model/protocol/worker/scenario/concurrency
- Generalize _fmt_winner() with label_a/label_b params (backward
compatible, defaults to gRPC/HTTP)
- Extract _aggregate_table() helper for reuse across sections
- Add per-runtime breakdown to aggregate section (SGLang: gRPC vs
HTTP, vLLM: gRPC vs HTTP)
- Add new _section_runtime_comparison() with aggregate, per-protocol,
and per-model runtime comparison tables
- Update _section_key_findings() with runtime comparison findings
- Update glossary with SGLang/vLLM comparison column descriptions
- Update report title to "Nightly Benchmark Summary" and sub-header
with runtime counts and runtime comparison point count
- Add runtime_comparisons field to SummaryResult
Why: With vLLM HTTP added to nightly benchmarks, we now run 4 variants
per model (SGLang HTTP/gRPC, vLLM HTTP/gRPC). The summary report needs
to compare runtimes (SGLang vs vLLM) in addition to protocols (gRPC vs
HTTP) to give a comprehensive view of performance differences.
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5587d9df6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Refactor _section_runtime_comparison() and the runtime findings in
_section_key_findings() to iterate over all distinct runtime pairs
instead of assuming a single pair from the first data point.
What changed:
- e2e_test/benchmarks/nightly_summarize.py:
- Extract _group_by_runtime_pair() helper to group
RuntimeComparisonPoints by (runtime_a, runtime_b) tuple
- Extract _rt_aggregate_table() helper for reuse across pairs
- _section_runtime_comparison() now renders a full section per
runtime pair (aggregate, per-protocol, per-model tables)
- _section_key_findings() now renders runtime findings per pair
- Glossary uses generic "{Runtime} X%" instead of hardcoded labels
Why: With 3 runtimes (SGLang, vLLM, TRT-LLM) there are 3 pairs to
compare. The previous code only read labels from the first comparison
point, mixing all pairs into one section.
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
b8bf74a to
46d4a47
Compare
2 participants
Description
Problem
vLLM nightly benchmarks were restricted to gRPC-only (
grpc_only: "true"), so HTTP protocol benchmarks were never run for vLLM workers.Solution
Add a vLLM HTTP worker launcher in
model_pool.pyand setgrpc_onlyto"false"for vLLM in the nightly workflow matrix so both HTTP and gRPC protocols are benchmarked for all runtimes.Changes
e2e_test/infra/model_pool.py: Add_build_vllm_cmdshared helper (used by both gRPC and HTTP paths),_build_vllm_http_cmd, and_launch_vllm_http_worker. Add_skip_deep_health_checkflag onModelInstancesince vLLM HTTP doesn't serve/health_generate. Route HTTP + vLLM requests to the new launcher in_launch_worker..github/workflows/nightly-benchmark.yml: Flipgrpc_onlyfrom"true"to"false"for all vLLM variants (single-gpu, multi-gpu, llama4-single, llama4-multi).e2e_test/benchmarks/test_nightly_perf.py: Update module docstring to reflect that both protocols now run for all runtimes.Test Plan
_build_vllm_cmdpreserves identical flags).Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests