Repository navigation
test(e2e): add Llama-4 and Llama-3.3 70b to nightly benchmarks - #900
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
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 three Llama models to nightly benchmarks and model specs, enables per-model 128K traffic-scenario overrides for nightly tests, and changes the benchmark command builder to derive the --api-backend from the E2E_RUNTIME environment variable. The nightly CI matrix is updated to include the new models. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
Summary of ChangesHello, 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 enhances the nightly benchmark coverage by incorporating several new Llama model variants: Llama-4-Scout, Llama-3.3-70B, and its FP8-quantized counterpart. The primary goal is to establish a robust system for continuously monitoring the inference performance of these critical models across both SGLang and vLLM engines, ensuring that any performance regressions or improvements are promptly identified. This expansion provides a more comprehensive performance overview for key large language models. Highlights
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. 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 integrates three new Llama models (Llama-4-Scout-17B-16E-Instruct, Llama-3.3-70B-Instruct, and Llama-3.3-70B-Instruct-FP8-dynamic) into the nightly performance benchmarks. This includes adding them to the benchmark test list and defining their comprehensive model specifications, including worker and vLLM arguments. A critical issue was identified in the model specifications for Llama-3.3-70B-Instruct and RedHatAI/Llama-3.3-70B-Instruct-FP8-dynamic, where the SGLang worker argument --mem-frac is invalid and should be corrected to --mem-fraction-static to ensure proper GPU memory management and prevent worker failures.
![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
🤖 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:
- Around line 124-126: The inline mapping entries currently have extra spaces
inside the braces and fail YAMLlint; update each inline mapping for the entries
referencing meta-llama/Llama-4-Scout-17B-16E-Instruct,
meta-llama/Llama-3.3-70B-Instruct, and
RedHatAI/Llama-3.3-70B-Instruct-FP8-dynamic to remove spaces directly after '{'
and before '}' (e.g., change " - { id: ..., slug: ..., test_class: ... }" to " -
{id: ..., slug: ..., test_class: ...}") so the braces have no internal
leading/trailing spaces and the YAML linter passes.
In `@e2e_test/infra/model_specs.py`:
- Around line 130-145: The repeated 70B model configuration arrays should be
extracted into shared constants to avoid drift: create two module-level
constants (e.g., SHARED_70B_WORKER_ARGS and SHARED_70B_VLLM_ARGS) containing the
identical worker_args and vllm_args values and replace the inline arrays in the
"meta-llama/Llama-3.3-70B-Instruct" entry and the matching 70B entries (the
dense and FP8 entries referenced around lines 146-161) with references to those
constants; keep the existing use of _resolve_model_path and preserve the exact
argument values when moving them to the constants.
- Around line 135-138: In the worker_args arrays (the "worker_args" lists in
this diff), replace the unsupported SGLang flag "--mem-frac" with the correct
flag "--mem-fraction-static" so workers start correctly; update both occurrences
(the list containing "--trust-remote-code", "--mem-frac=0.9" and the second
similar list later in the file) to use "--mem-fraction-static=0.9" (preserving
the numeric value).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e1a23a6f-97e8-41fa-ab0e-1618131c9fbf
📒 Files selected for processing (3)
.github/workflows/nightly-benchmark.ymle2e_test/benchmarks/test_nightly_perf.pye2e_test/infra/model_specs.py
| # Llama-3.3-70B - Nightly benchmarks | ||
| "meta-llama/Llama-3.3-70B-Instruct": { | ||
| "model": _resolve_model_path("meta-llama/Llama-3.3-70B-Instruct"), | ||
| "tp": 4, | ||
| "features": ["chat", "streaming", "function_calling"], | ||
| "worker_args": [ | ||
| "--trust-remote-code", | ||
| "--mem-frac=0.9", | ||
| ], | ||
| "vllm_args": [ | ||
| "--trust-remote-code", | ||
| "--max-model-len=131072", | ||
| "--gpu-memory-utilization=0.9", | ||
| "--enable-chunked-prefill", | ||
| ], | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract shared 70B args to avoid config drift.
The dense and FP8 70B entries repeat identical worker_args and vllm_args. Pull these into shared constants to keep future tuning changes synchronized.
Also applies to: 146-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e_test/infra/model_specs.py` around lines 130 - 145, The repeated 70B model
configuration arrays should be extracted into shared constants to avoid drift:
create two module-level constants (e.g., SHARED_70B_WORKER_ARGS and
SHARED_70B_VLLM_ARGS) containing the identical worker_args and vllm_args values
and replace the inline arrays in the "meta-llama/Llama-3.3-70B-Instruct" entry
and the matching 70B entries (the dense and FP8 entries referenced around lines
146-161) with references to those constants; keep the existing use of
_resolve_model_path and preserve the exact argument values when moving them to
the constants.
CatherineSue
left a comment
CatherineSue
left a comment
There was a problem hiding this comment.
nit: PR title needs to be updated
| "model": _resolve_model_path("meta-llama/Llama-4-Scout-17B-16E-Instruct"), | ||
| "tp": 4, | ||
| "features": ["chat", "streaming", "function_calling", "moe"], | ||
| "worker_args": [ |
There was a problem hiding this comment.
We distinguish this by worker_args (sglang) and vllm_args (vllm)? It is quite misleading.
We should rename both better to reflect:
- they are for worker
- either for sglang or vllm or trtllm
There was a problem hiding this comment.
Yes I totally agreed the naming is confusing — worker_args actually means SGLang-specific args. This naming convention is pre-existing across all models in model_specs.py. If you like, I can do the rename (worker_args → sglang_args) in a separate refactoring PR to keep this one scoped to model additions. What do you think?
paxiaatucsdedu
changed the title
paxiaatucsdedu
changed the title
@CatherineSue Sure I updated the PR title. |
|
@CatherineSue |
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)
e2e_test/benchmarks/test_nightly_perf.py (1)
70-76:⚠️ Potential issue | 🟠 MajorKeep
BENCH_TEST_MODEpinned to the smoke scenario.Line 76 now lets model-specific
traffic_scenariovalues override_TEST_TRAFFIC_SCENARIO. All three new nightly entries pass_TEXT_SCENARIOS_128K, so a smoke run will still execute the full six-scenario set, includingD(128000,200), which defeats the fast-path and makesBENCH_TEST_MODEmuch more timeout-prone.💡 Proposed fix
if _TEST_MODE: + kwargs.pop("traffic_scenario", None) genai_bench_runner( router_url=gateway.base_url, model_path=model_path, experiment_folder=experiment_folder, num_concurrency=_TEST_NUM_CONCURRENCY, - traffic_scenario=kwargs.pop("traffic_scenario", _TEST_TRAFFIC_SCENARIO), + traffic_scenario=_TEST_TRAFFIC_SCENARIO, max_requests_per_run=_TEST_MAX_REQUESTS, timeout_sec=600, server_engine=runtime_display, gpu_type=gpu_type, gpu_count=gpu_count, **kwargs, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/benchmarks/test_nightly_perf.py` around lines 70 - 76, The test-mode branch is letting callers override the smoke traffic via kwargs.pop("traffic_scenario", _TEST_TRAFFIC_SCENARIO); change the genai_bench_runner invocation inside the _TEST_MODE block to explicitly pass traffic_scenario=_TEST_TRAFFIC_SCENARIO (and stop popping it from kwargs) so BENCH_TEST_MODE always runs the lightweight smoke scenario—update the call that uses genai_bench_runner and remove or ignore kwargs.pop("traffic_scenario", ...) to enforce the pinned smoke scenario.
🤖 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 `@e2e_test/benchmarks/test_nightly_perf.py`:
- Around line 70-76: The test-mode branch is letting callers override the smoke
traffic via kwargs.pop("traffic_scenario", _TEST_TRAFFIC_SCENARIO); change the
genai_bench_runner invocation inside the _TEST_MODE block to explicitly pass
traffic_scenario=_TEST_TRAFFIC_SCENARIO (and stop popping it from kwargs) so
BENCH_TEST_MODE always runs the lightweight smoke scenario—update the call that
uses genai_bench_runner and remove or ignore kwargs.pop("traffic_scenario", ...)
to enforce the pinned smoke scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67175f9d-b50e-4909-9e1b-e26f296d239b
📒 Files selected for processing (2)
e2e_test/benchmarks/conftest.pye2e_test/benchmarks/test_nightly_perf.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e_test/benchmarks/test_nightly_perf.py (1)
71-84:⚠️ Potential issue | 🟠 MajorKeep
BENCH_TEST_MODEpinned to the smoke scenario.Line 77 now lets per-model
traffic_scenariooverrides win in test mode. For the new 128K entries,e2e_test/benchmarks/conftest.py:103-108expands that list into six--traffic-scenarioflags, so BENCH_TEST_MODE stops being the cheapD(100,100)sanity path and starts exercising the full 128K matrix.🛠️ Proposed fix
if _TEST_MODE: + kwargs.pop("traffic_scenario", None) genai_bench_runner( router_url=gateway.base_url, model_path=model_path, experiment_folder=experiment_folder, num_concurrency=_TEST_NUM_CONCURRENCY, - traffic_scenario=kwargs.pop("traffic_scenario", _TEST_TRAFFIC_SCENARIO), + traffic_scenario=_TEST_TRAFFIC_SCENARIO, max_requests_per_run=_TEST_MAX_REQUESTS, timeout_sec=600, server_engine=runtime_display,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/benchmarks/test_nightly_perf.py` around lines 71 - 84, When _TEST_MODE is true we must force the smoke scenario instead of allowing per-model overrides: update the genai_bench_runner call inside the _TEST_MODE block to pass traffic_scenario=_TEST_TRAFFIC_SCENARIO explicitly (do not use kwargs.pop("traffic_scenario", ...)) and ensure any incoming traffic_scenario in kwargs is removed or ignored so it cannot override the test-mode value; key symbols to change are the _TEST_MODE conditional, the genai_bench_runner call, the traffic_scenario argument, _TEST_TRAFFIC_SCENARIO, and kwargs to prevent leakage.
🤖 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/benchmarks/test_nightly_perf.py`:
- Around line 38-45: The _TEXT_SCENARIOS_128K list is a duplicated copy of the
baseline scenarios; instead, create a single baseline constant (e.g.,
TEXT_SCENARIOS_BASELINE) and derive _TEXT_SCENARIOS_128K from that constant
(e.g., _TEXT_SCENARIOS_128K = derive_128k(TEXT_SCENARIOS_BASELINE) or
slice/transform as needed), then replace all uses of the duplicated list in the
model table and elsewhere to reference the new baseline-derived variable (ensure
any other size-specific variants like 121-141 are similarly derived from the
same baseline constant).
---
Outside diff comments:
In `@e2e_test/benchmarks/test_nightly_perf.py`:
- Around line 71-84: When _TEST_MODE is true we must force the smoke scenario
instead of allowing per-model overrides: update the genai_bench_runner call
inside the _TEST_MODE block to pass traffic_scenario=_TEST_TRAFFIC_SCENARIO
explicitly (do not use kwargs.pop("traffic_scenario", ...)) and ensure any
incoming traffic_scenario in kwargs is removed or ignored so it cannot override
the test-mode value; key symbols to change are the _TEST_MODE conditional, the
genai_bench_runner call, the traffic_scenario argument, _TEST_TRAFFIC_SCENARIO,
and kwargs to prevent leakage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 520c47e8-0940-41c0-a2fd-db4034d8aa9b
📒 Files selected for processing (1)
e2e_test/benchmarks/test_nightly_perf.py
3659f8b to
3e6b563
Compare
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/benchmarks/conftest.py`:
- Around line 75-84: The code silently falls back to "openai" when an
unrecognized E2E_RUNTIME is provided; modify the block that computes runtime and
api_backend so that after setting runtime = os.environ.get("E2E_RUNTIME",
"").lower() and api_backend = {"sglang": "sglang", "vllm": "vllm"}.get(runtime,
"openai"), you emit a warning if runtime is non-empty and not a key in the
mapping (e.g., using Python's logging.warning or pytest logging) indicating the
unrecognized E2E_RUNTIME value and that you're falling back to "openai"; keep
the existing cmd.extend(...) behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5849f5d2-0ab0-42f1-8a36-a5bb328bb0c0
📒 Files selected for processing (1)
e2e_test/benchmarks/conftest.py
|
@CatherineSue Is it possible to manually download the model on the H100 or make sure the HF_TOKEN have access to the meta models? |
c26bcd4 to
f8f5227
Compare
Thanks for your help |
2ec83f8 to
8148c07
Compare
|
Hi @paxiaatucsdedu,
|
Hi @key4ng
|
|
cc: @CatherineSue not sure if we should include hicache param for nightly test, could you take a look |
3450499 to
e6459e7
Compare
e6459e7 to
0cf03d9
Compare
|
@CatherineSue Tests passed: https://github.com/lightseekorg/smg/actions/runs/23662600790 |
Include three Llama variants in nightly performance runs: meta-llama/Llama-4-Scout-17B-16E-Instruct, meta-llama/Llama-3.3-70B-Instruct, and RedHatAI/Llama-3.3-70B-Instruct-FP8-dynamic. Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
- Added pull request trigger for specific paths in the nightly benchmark workflow. - Commented out previous model configurations to streamline the benchmark process. - Set job conditions to false for multi-worker and single-worker jobs to prevent execution. Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
…re_eos for SGLang Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
- Add pull_request trigger for benchmark workflow (per b0cc8fd pattern) - Comment out existing models to reduce resource usage during testing - Set multi-worker and single-worker-h200 jobs to if:false Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
…cate Scout entry Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
SGLang v0.5.9 and vLLM v0.18.0 both have native support for Llama-4 and Llama-3.3 architectures in their model registries, so --trust-remote-code is not needed for these models. Removed from: - meta-llama/Llama-4-Scout-17B-16E-Instruct (worker_args + vllm_args) - meta-llama/Llama-3.3-70B-Instruct (worker_args + vllm_args) - RedHatAI/Llama-3.3-70B-Instruct-FP8-dynamic (worker_args + vllm_args) Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
0cf03d9 to
02a457e
Compare
|
@CatherineSue |
|
@slin1237 Addressed all your comments and requested changes. All benchmark tests passed: https://github.com/lightseekorg/smg/actions/runs/23662600790 |
…roject#900) Signed-off-by: paxiaatucsdedu <paxia@ucsd.edu>
4 participants
Description
Problem
The nightly benchmark suite currently lacks coverage for several important Llama model variants: the Llama-4-Scout MoE model, the Llama-3.3-70B dense model, and its FP8-quantized counterpart. Without benchmark data for these models, we cannot track their inference performance over time across SGLang and vLLM engines.
Solution
Add model specifications, workflow entries, and test class definitions for all three models. All models are configured to run on
4-gpu-h100runners.Changes
e2e_test/infra/model_specs.py:meta-llama/Llama-4-Scout-17B-16E-Instructmeta-llama/Llama-3.3-70B-InstructRedHatAI/Llama-3.3-70B-Instruct-FP8-dynamic.github/workflows/nightly-benchmark.yml:single-worker(H100) job matrix.e2e_test/benchmarks/test_nightly_perf.py:Llama4Scout,Llama70b, andLlama70bFp8entries to_NIGHTLY_MODELSlistTest Plan
ruff check e2e_test/passes with no new lint errors.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Tests
Chores