Repository navigation
feat(benchmark): add vLLM HTTP to nightly benchmark - #481
paxiaatucsdedu wants to merge 2 commits into
Conversation
Summary of ChangesHello @paxiaatucsdedu, 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 extends the nightly performance benchmarks to include vLLM over HTTP, addressing a previous limitation where only gRPC was tested. This change ensures a more comprehensive performance evaluation for vLLM across both communication protocols, with corresponding updates to the test configuration and documentation to reflect the expanded scope. Highlights
Changelog
Ignored Files
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:
📝 WalkthroughWalkthroughEnable HTTP runs for vLLM in the nightly benchmark workflow (set vllm variants' Changes
Sequence Diagram(s)sequenceDiagram
participant PR as "Pull Request"
participant GH as "GitHub Actions"
participant Job as "nightly-benchmark job"
participant Harness as "test_nightly_perf.py"
participant Pool as "model_pool (launcher)"
participant vLLM as "vLLM runtime"
participant sgLang as "sgLang runtime"
PR->>GH: push / pull_request trigger
GH->>Job: start nightly-benchmark matrix
Job->>Harness: invoke nightly benchmark script (TEST_MODE)
Harness->>Pool: request model launch (mode=http/grpc)
alt mode=http and is_vllm()
Pool->>vLLM: build vllm api_server cmd (--model + vllm args)
vLLM-->>Pool: started
else non-vllm http or grpc
Pool->>sgLang: build sgLang http/grpc cmd (+ gRPC/embedding/PD args)
sgLang-->>Pool: started
end
Harness->>vLLM: run tests (if vLLM)
Harness->>sgLang: run tests (if sgLang)
vLLM-->>Harness: test results
sgLang-->>Harness: test results
Harness-->>Job: report results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
![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: 532f934ca1
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/nightly-benchmark.yml (2)
417-419:⚠️ Potential issue | 🟡 MinorCommented-out
multi-worker-h200block still hasgrpc_only: "true"for vllm.If this block is uncommented in a future PR, the HTTP protocol tests for vllm will be silently skipped, inconsistent with the intent of this change. Update the stale value while the context is fresh.
🔧 Suggested update inside the comment block
- # - { 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" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/nightly-benchmark.yml around lines 417 - 419, The commented-out multi-worker-h200 variant still sets the vllm variant's grpc_only: "true", which would skip HTTP protocol tests if that block is later uncommented; update the vllm variant in that commented block to grpc_only: "false" (or remove the grpc_only override) so the vllm entry matches the intended HTTP testing behavior; look for the commented block labeled multi-worker-h200 and the variant entries referencing id: vllm and grpc_only to make the change.
162-164: 🧹 Nitpick | 🔵 TrivialDead code: the
grpc_only == "true"filter can never trigger.All three active jobs (single-worker, multi-worker, single-worker-h200) now have every variant set to
grpc_only: "false". TheK_FILTERaugmentation block (… and grpc) is permanently unreachable. Consider either removing these blocks or retaining them with a comment explaining the intent (e.g., "reserved for future gRPC-only variants").🧹 Minimal cleanup (apply to all three jobs)
- K_FILTER="${{ matrix.model.test_class }}" - if [ "${{ matrix.variant.grpc_only }}" == "true" ]; then - K_FILTER="${{ matrix.model.test_class }} and grpc" - fi + K_FILTER="${{ matrix.model.test_class }}"Also applies to: 269-271, 379-381
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/nightly-benchmark.yml around lines 162 - 164, The K_FILTER augmentation block that appends "and grpc" is dead because matrix.variant.grpc_only is always set to "false"; update the workflow by either removing the unreachable conditional that checks matrix.variant.grpc_only and the associated K_FILTER modification, or keep the conditional but add a clear comment like "reserved for future gRPC-only variants" to explain intent; locate the conditional using the symbols K_FILTER and matrix.variant.grpc_only (also update the identical blocks at the other occurrences mentioned) and apply the same change consistently.
🤖 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 98: Remove the extra padding spaces inside the inline YAML mapping braces
for the vllm variant rows (e.g., the sequence item starting with "- { id: vllm,
runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm: false }")
so the key:value pairs are not separated by multi-space alignment; alternatively
add an inline YAMLlint disable comment for the `braces` rule on those same vllm
variant rows (and the analogous rows for the other variants with similar
formatting) to satisfy the linter.
- Around line 6-8: The workflow contains a pull_request trigger that targets the
feature branch "pan/vllm-http-nightly-benchmark" which is only needed for
testing that branch; remove the pull_request: block (or at least delete the
branches: - pan/vllm-http-nightly-benchmark entry) so the workflow on main no
longer references that feature branch, leaving only the intended triggers for
the nightly-benchmark workflow.
In `@e2e_test/benchmarks/test_nightly_perf.py`:
- Line 33: The file sets the global flag _TEST_MODE = True which forces nightly
benchmarks to run in reduced test mode; change _TEST_MODE to False (restore
production behavior) in e2e_test/benchmarks/test_nightly_perf.py and ensure no
subsequent test-only overrides exist (search for other assignments or
environment gating around _TEST_MODE) so scheduled runs use the full
concurrency/requests/scenario/timeout sweep rather than the limited test
parameters.
---
Outside diff comments:
In @.github/workflows/nightly-benchmark.yml:
- Around line 417-419: The commented-out multi-worker-h200 variant still sets
the vllm variant's grpc_only: "true", which would skip HTTP protocol tests if
that block is later uncommented; update the vllm variant in that commented block
to grpc_only: "false" (or remove the grpc_only override) so the vllm entry
matches the intended HTTP testing behavior; look for the commented block labeled
multi-worker-h200 and the variant entries referencing id: vllm and grpc_only to
make the change.
- Around line 162-164: The K_FILTER augmentation block that appends "and grpc"
is dead because matrix.variant.grpc_only is always set to "false"; update the
workflow by either removing the unreachable conditional that checks
matrix.variant.grpc_only and the associated K_FILTER modification, or keep the
conditional but add a clear comment like "reserved for future gRPC-only
variants" to explain intent; locate the conditional using the symbols K_FILTER
and matrix.variant.grpc_only (also update the identical blocks at the other
occurrences mentioned) and apply the same change consistently.
![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
The pull request successfully updates the nightly benchmark to include vLLM HTTP performance, which is a valuable addition. The changes to the Python files are minimal and directly address the problem statement. The module docstring in test_nightly_perf.py has been updated to reflect the change, and a test mode flag has been set to True for local testing purposes. Overall, the changes are clear and functional.
532f934 to
2ef7f53
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ef7f53936
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/nightly-benchmark.yml (1)
419-419:⚠️ Potential issue | 🟡 MinorStale
grpc_only: "true"in commented-outmulti-worker-h200vLLM variant.All three active jobs were updated to
grpc_only: "false", but the commented-outmulti-worker-h200block was not. A developer enabling this job in the future would silently get gRPC-only vLLM runs, contrary to the intent of this PR.🔧 Proposed fix
-# - { 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" }🤖 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 419, Update the commented-out multi-worker-h200 vLLM variant so its grpc_only flag matches the active jobs: change the stub line containing id: vllm, runtime: vllm, grpc_only: "true", setup_vllm: true, setup_trtllm: false, extra_deps: "genai-bench" to use grpc_only: "false" (i.e., edit the commented block referencing the vllm variant / multi-worker-h200 so grpc_only is "false" instead of "true").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/nightly-benchmark.yml:
- Line 419: Update the commented-out multi-worker-h200 vLLM variant so its
grpc_only flag matches the active jobs: change the stub line containing id:
vllm, runtime: vllm, grpc_only: "true", setup_vllm: true, setup_trtllm: false,
extra_deps: "genai-bench" to use grpc_only: "false" (i.e., edit the commented
block referencing the vllm variant / multi-worker-h200 so grpc_only is "false"
instead of "true").
---
Duplicate comments:
In @.github/workflows/nightly-benchmark.yml:
- Around line 6-8: Remove the temporary pull_request trigger for the feature
branch in .github/workflows/nightly-benchmark.yml by deleting the pull_request:
block (the branches: - pan/vllm-http-nightly-benchmark entry) so the workflow no
longer contains a dead trigger tied to that feature branch before merging to
main.
- Line 98: The inline YAML mapping for the vllm matrix entries violates
YAMLlint's braces spacing rule; update each occurrence of the mapping like { id:
vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm:
false } to use consistent single spaces after commas and no extra padding so it
reads { id: vllm, runtime: vllm, grpc_only: "false", setup_vllm: true,
setup_trtllm: false }; apply the same spacing fix to the other two identical
mappings (the ones with id: vllm elsewhere in the file).
In `@e2e_test/benchmarks/test_nightly_perf.py`:
- Line 33: The test flag _TEST_MODE is set to True and must be reverted to
False; change the constant _TEST_MODE = True back to _TEST_MODE = False in the
file (search for the symbol _TEST_MODE) so scheduled nightly runs use production
benchmark parameters and not the degraded test settings, then run the
benchmark/test suite to verify behavior and commit the change.
2ef7f53 to
61e659d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61e659d19a
ℹ️ 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".
| on: | ||
| schedule: | ||
| - cron: '0 8 * * *' | ||
| pull_request: |
There was a problem hiding this comment.
instead of branch matching
just run it during pull request
then revert this later
There was a problem hiding this comment.
The latest push still has pull_request: in the workflow. Please remove it before pushing for review — reviewers should not need to mentally track which lines are "temporary". Same applies to _TEST_MODE = True in test_nightly_perf.py.
There was a problem hiding this comment.
I'll keep pull_request: and _TEST_MODE = True while iterating on the PR so I can validate changes on each push without waiting for the nightly schedule. I'll remove both in the final commit before merge. I will squash everything into a clean commit at that point.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 570d59bd57
ℹ️ 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".
9484a01 to
3d28003
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d2800337c
ℹ️ 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".
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 96: Fix the YAMLlint "braces" errors by normalizing the spacing inside
the inline map braces for the vllm variant rows: replace entries like `{ id:
vllm, runtime: vllm, grpc_only: "false", setup_vllm: true, setup_trtllm:
false }` (and the equivalent entries on the other two vllm variant lines) with a
single-space style such as `{ id: vllm, runtime: vllm, grpc_only: "false",
setup_vllm: true, setup_trtllm: false }` so there are no extra alignment spaces
inside the braces and YAMLlint braces rule passes.
- Line 6: Remove the bare pull_request trigger from the workflow to avoid
running costly GPU jobs on every PR: delete the "pull_request:" stanza
(including any empty or branch-scoped variants like "pull_request: branches:
[...]" if present) so the workflow only runs via the existing "schedule" and
"workflow_dispatch" triggers; ensure no new PR trigger is added and keep
schedule and workflow_dispatch intact.
In `@e2e_test/benchmarks/test_nightly_perf.py`:
- Line 33: The _TEST_MODE flag is currently set to True causing nightly runs to
use degraded benchmark parameters; change the module-level variable _TEST_MODE
from True back to False in test_nightly_perf.py so scheduled nightly benchmarks
run the full production sweep (restore _TEST_MODE = False), then verify the
configured concurrency, request counts, scenarios (e.g., D(100,100)), and
timeout are back to their intended production values before merging.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec0e336fcc
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e_test/infra/model_pool.py (1)
299-300:⚠️ Potential issue | 🟡 MinorStale class docstring
The
ModelPoolclass docstring says "Manages long-running SGLang worker processes" (line 300), but the pool now also manages vLLM HTTP workers.✏️ Proposed fix
- """Manages long-running SGLang worker processes across GPUs. + """Manages long-running model worker processes (SGLang, vLLM) across GPUs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/infra/model_pool.py` around lines 299 - 300, The ModelPool class docstring is stale — update the docstring for the ModelPool class to reflect that it manages both long-running SGLang worker processes and vLLM HTTP workers; locate the ModelPool class declaration and replace or extend the existing string to mention vLLM HTTP workers explicitly and any relevant behavior differences so the description accurately represents responsibilities.
🤖 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 536-555: The HTTP vLLM command construction duplicates logic and
hardcoded defaults from _build_grpc_cmd; extract the common construction into a
shared helper (e.g., a new static method _build_vllm_cmd or module-level helper)
that accepts differences like entrypoint module and grpc flag, or create
module-level constants for DEFAULT_MAX_MODEL_LEN and
DEFAULT_GPU_MEMORY_UTILIZATION and call a single builder from both the HTTP path
and _build_grpc_cmd; update the current inline list (the cmd definition that
uses "vllm.entrypoints.openai.api_server", model_path, DEFAULT_HOST, port,
tp_size, "--max-model-len", "16384", "--gpu-memory-utilization", "0.9", and
vllm_args) to call the new shared builder so defaults and vllm_args handling are
centralized.
- Around line 546-555: The command builder (_build_grpc_cmd) currently appends
hardcoded "--max-model-len 16384" and "--gpu-memory-utilization 0.9" then
extends with spec.get("vllm_args", []), which can create duplicate flags; change
the logic to inspect spec.get("vllm_args", []) (variable extra) first and only
append the default "--max-model-len" and "--gpu-memory-utilization" to cmd when
those keys are not present in extra (match both short/long forms and values),
keeping use of tp_size and existing cmd construction intact so duplicates are
avoided.
- Around line 534-555: The deep_health_check always calls /health_generate
causing vLLM HTTP workers to fail; add a boolean field use_health_generate: bool
= True to the ModelInstance dataclass, update deep_health_check to only call
/health_generate when self.use_health_generate is True (otherwise rely on
/health or the existing health_check logic), and when creating the ModelInstance
in the vLLM HTTP launch path inside _launch_model (the branch that builds the
vllm.entrypoints.openai.api_server cmd) set use_health_generate=False so vLLM
instances skip the nonexistent /health_generate endpoint.
---
Outside diff comments:
In `@e2e_test/infra/model_pool.py`:
- Around line 299-300: The ModelPool class docstring is stale — update the
docstring for the ModelPool class to reflect that it manages both long-running
SGLang worker processes and vLLM HTTP workers; locate the ModelPool class
declaration and replace or extend the existing string to mention vLLM HTTP
workers explicitly and any relevant behavior differences so the description
accurately represents responsibilities.
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 `@e2e_test/benchmarks/test_nightly_perf.py`:
- Line 33: The _TEST_MODE flag is currently set to True which forces degraded
benchmark parameters; change the constant _TEST_MODE to False so nightly runs
use full benchmark settings (restore normal concurrency, request counts,
scenarios, and timeouts) and prevent silent degraded results—update the
declaration of _TEST_MODE in test_nightly_perf.py accordingly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d29ef0df7
ℹ️ 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".
CatherineSue
left a comment
CatherineSue
left a comment
There was a problem hiding this comment.
The approach of inlining vLLM-specific command construction inside _launch_model doesn't follow the existing pattern. gRPC already handles runtime dispatch cleanly via _build_grpc_cmd / _launch_grpc_worker — HTTP should follow the same structure rather than scattering if is_vllm() branches in the shared launch path.
This also introduces several issues: deep_health_check will break (vLLM doesn't serve /health_generate), hardcoded defaults are duplicated across two locations, and SGLang-specific flags (embedding, PD disagg, worker_args) are silently skipped for vLLM HTTP without explicit intent.
@CatherineSue Of course. Let me use the same structure for HTTP VLLM.
This line is purely for testing if the code works. I will remove it before merging. |
Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
f03797a to
86486b8
Compare
CatherineSue
left a comment
CatherineSue
left a comment
There was a problem hiding this comment.
I think the overall structure of the file is way too redundant.
|
|
||
| # vLLM HTTP does not support /health_generate, use /health instead | ||
| if self.mode == ConnectionMode.HTTP and is_vllm(): | ||
| return self._http_health_check(timeout) |
There was a problem hiding this comment.
Can we double check on vllm's health check endpoint? I remember they have something similar to /health_generate
There was a problem hiding this comment.
Sure. I searched /health key word in vllm code. Did not find something similar to /health_generate.
| "--tensor-parallel-size", | ||
| str(tp_size), | ||
| "--max-model-len", | ||
| "16384", |
There was a problem hiding this comment.
This override is repeatitive everywhere. Is this specific to vLLM? If it does, how does it work with different models? Do any of them have specific overrides?
Is this required by both gRPC and http mode for vLLM? If it does, why don't we introduce a new function to consolidate it insead of having it spreaded everywhere?
There was a problem hiding this comment.
Is this specific to vLLM?
Yes.
If it does, how does it work with different models?
Models that need something different override it via vllm_args in model_specs.py.
Do any of them have specific overrides?
Yes. The specific overrides are in model_specs.py.
Is this required by both gRPC and http mode for vLLM? If it does, why don't we introduce a new function to consolidate it insead of having it spreaded everywhere?
Yes. The defaults were already hardcoded in _build_grpc_cmd on main. I will put them into a shared _VLLM_DEFAULT_ARGS constant in this PR.
|
|
||
| cmd = self._build_vllm_http_cmd(model_path, DEFAULT_HOST, port, tp_size, model_spec) | ||
|
|
||
| if instance_key: |
There was a problem hiding this comment.
You can simply use a key = instance_key or xxx for this, instead of having a 4-line if-else.
There was a problem hiding this comment.
Of course. I see that _launch_grpc_worker uses the same 4-line if/else pattern in the main branch. Should I update that one as well in this PR?
Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
|
Hi @paxiaatucsdedu, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
3 participants
Description
Problem
The nightly benchmark only ran vLLM over gRPC, without vLLM HTTP performance benchmark.
Solution
Remove the
grpc_only: "true"restriction on the vLLM variant in the nightly benchmark workflow.Changes
.github/workflows/nightly-benchmark.yml: Setgrpc_only: "false"for the vLLM variant in all three jobs (single-worker, multi-worker, single-worker-h200).e2e_test/benchmarks/test_nightly_perf.py: Updated module docstring to reflect that both protocols now run.Test Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Tests
Bug Fixes