Repository navigation
e2e: canonical HF model IDs and nightly benchmark runner split - #373
Conversation
github-actions
Bot
added
documentation
|
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:
📝 WalkthroughWalkthroughThis PR replaces short model IDs with path-based identifiers across e2e tests and infra, updates MODEL_SPECS and defaults, adds E2E_MODEL_TP_OVERRIDES, and refactors the GitHub Actions nightly-benchmark workflow to a per-model matrix with dynamic runners, caching, and per-model backend setup flags. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub as "GitHub Actions"
participant Runner as "Runner (matrix runs-on)"
participant Cache as "Wheel Cache / sccache"
participant Setup as "Backend Setup (vLLM / TRT-LLM / SGLang)"
participant Job as "Per-model Benchmark Job"
participant Artifacts as "Artifact Storage"
GitHub->>Runner: trigger (push / pull_request / workflow_dispatch)
Runner->>Cache: check wheel cache
alt cache miss (wheel)
Runner->>Setup: install Rust, create Python venv, build wheel
Setup->>Cache: upload wheel cache
Setup->>Runner: show sccache stats
end
Runner->>Setup: conditional per-model backend setup (vLLM/TRT/SGLang) based on matrix flags
Runner->>Job: run benchmark with MODEL_SPECS, GPU_TYPE, extra_deps, slug
Job->>Artifacts: upload slug-named artifacts and results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
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 focuses on a significant infrastructure refinement by standardizing model identification within the End-to-End testing and nightly benchmarking systems. By adopting canonical Hugging Face model IDs, the changes enhance the robustness and maintainability of model referencing and test configurations. The update also includes improvements to nightly benchmark execution, such as optimized runner assignments and safer handling of output directory names, contributing to a more reliable and consistent development environment. 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
|
![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 effectively standardizes the end-to-end test suite to use canonical Hugging Face model IDs, removing the old short-name aliases. The changes are comprehensive, touching test files, fixtures, and configuration to ensure consistency. I've found two issues: a critical syntax error in e2e_test/infra/model_specs.py due to a misplaced bracket, and a high-severity issue in e2e_test/benchmarks/test_nightly_perf.py where a debug flag _TEST_MODE seems to have been left enabled, which would prevent the nightly benchmarks from running correctly. Once these are addressed, the PR will be in great shape.
![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
🤖 Fix all issues with AI agents
In `@e2e_test/benchmarks/nightly_summarize.py`:
- Line 81: The inline comment describing the newest folder format in
nightly_summarize.py uses an invalid slash; update that comment to use the
actual double-underscore separator (`__`) used in folder names. Replace the
example "meta-llama/Llama-3.1-8B-Instruct_grpc_sglang_single" with a correctly
separated example such as "meta-llama__Llama-3.1-8B-Instruct_grpc_sglang_single"
so the comment matches the real `model__protocol_runtime_worker_type`
convention.
- Around line 64-75: The docstring entries in nightly_summarize.py list example
folder names with "/" separators (e.g.,
nightly_meta-llama/Llama-3.1-8B-Instruct_http_sglang_single) but the actual test
naming uses safe_model_id = model_id.replace("/", "__") in test_nightly_perf.py,
so update the docstring examples to use the filesystem-safe "__" separator
(e.g., nightly_meta-llama__Llama-3.1-8B-Instruct_http_sglang_single) or
alternatively adjust the parsing logic to accept both formats; reference the
docstring examples in nightly_summarize.py and the safe_model_id replacement in
test_nightly_perf.py when making the change.
In `@e2e_test/benchmarks/test_nightly_perf.py`:
- Line 32: The test flag _TEST_MODE in test_nightly_perf.py is currently True
which forces the suite into a minimal quick-run (single scenario, single
concurrency, low request count and short timeout); change _TEST_MODE = True to
_TEST_MODE = False so the nightly benchmark uses the full default scenarios,
concurrency list, max request counts and the 3-hour timeout; update the constant
in the file (symbol: _TEST_MODE) and ensure the change is committed before
merging so the comprehensive nightly run is executed.
4678025 to
9f4c7ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e_test/benchmarks/test_nightly_perf.py`:
- Around line 57-59: The current slugging replaces "/" with "__" which is lossy
and can cause collisions; change the construction of safe_model_id so it uses a
reversible encoding (e.g., URL-quote or base64) of model_id rather than simple
replacement, then use that encoded string when building experiment_folder while
still storing the original model_id in metadata; update the code that defines
safe_model_id and experiment_folder (referencing safe_model_id, model_id, and
experiment_folder) to perform reversible encoding/decoding to guarantee
uniqueness and avoid overwrites.
| # Keep folder names filesystem-safe while retaining the canonical HF model id in metadata. | ||
| safe_model_id = model_id.replace("/", "__") | ||
| experiment_folder = f"nightly_{safe_model_id}_{backend}_{runtime}_{worker_type}" |
There was a problem hiding this comment.
Avoid artifact collisions from lossy model-id slugging.
Replacing “/” with “” is not reversible; model IDs containing “” can collide and overwrite artifacts. Use a reversible encoding (e.g., URL-quote or base64) to preserve uniqueness.
🔧 Proposed fix (reversible encoding)
- safe_model_id = model_id.replace("/", "__")
+ safe_model_id = quote(model_id, safe="")+from urllib.parse import quote📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Keep folder names filesystem-safe while retaining the canonical HF model id in metadata. | |
| safe_model_id = model_id.replace("/", "__") | |
| experiment_folder = f"nightly_{safe_model_id}_{backend}_{runtime}_{worker_type}" | |
| # Keep folder names filesystem-safe while retaining the canonical HF model id in metadata. | |
| safe_model_id = quote(model_id, safe="") | |
| experiment_folder = f"nightly_{safe_model_id}_{backend}_{runtime}_{worker_type}" |
🤖 Prompt for AI Agents
In `@e2e_test/benchmarks/test_nightly_perf.py` around lines 57 - 59, The current
slugging replaces "/" with "__" which is lossy and can cause collisions; change
the construction of safe_model_id so it uses a reversible encoding (e.g.,
URL-quote or base64) of model_id rather than simple replacement, then use that
encoded string when building experiment_folder while still storing the original
model_id in metadata; update the code that defines safe_model_id and
experiment_folder (referencing safe_model_id, model_id, and experiment_folder)
to perform reversible encoding/decoding to guarantee uniqueness and avoid
overwrites.
9f4c7ea to
d7e302f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/nightly-benchmark.yml:
- Line 219: The commented matrix entry for id
"meta-llama/Llama-4-Maverick-17B-128E-Instruct-FP8" is missing required fields
and will break the workflow if re-enabled; update the commented line to include
slug, runs_on, and gpu_type to match other entries (e.g., add slug:
meta-llama-Llama-4-Maverick-17B-128E-Instruct-FP8, runs_on: '["8-gpu-h200"]',
gpu_type: H200) while keeping test_class: TestNightlyLlama4MaverickMulti so the
full commented entry mirrors the existing matrix format.
🧹 Nitpick comments (2)
.github/workflows/nightly-benchmark.yml (1)
167-168: Consider extracting duplicatedE2E_MODEL_TP_OVERRIDESto workflow-level env.This JSON string is duplicated in both single-worker (line 168) and multi-worker (line 281) jobs. If the model list changes, both need updating. Move to the top-level
env:block to maintain a single source of truth.♻️ Suggested refactor
env: RUSTC_WRAPPER: sccache SCCACHE_GHA_ENABLED: "true" + E2E_MODEL_TP_OVERRIDES: '{"meta-llama/Llama-3.1-8B-Instruct":1,"meta-llama/Llama-3.2-1B-Instruct":1,"Qwen/Qwen2.5-7B-Instruct":1,"Qwen/Qwen2.5-14B-Instruct":1,"deepseek-ai/DeepSeek-R1-Distill-Qwen-7B":1,"Qwen/Qwen3-30B-A3B":1,"mistralai/Mistral-7B-Instruct-v0.3":1,"openai/gpt-oss-20b":1}'Then reference via
${{ env.E2E_MODEL_TP_OVERRIDES }}in both jobs.e2e_test/infra/model_specs.py (1)
136-138: Consider logging a warning on malformed JSON instead of silent pass.Silently ignoring
JSONDecodeErrorcould hide CI misconfigurations. A warning log would help operators diagnose issues while still falling back safely.♻️ Suggested improvement
+import logging + +_logger = logging.getLogger(__name__) + ... except json.JSONDecodeError: - # Ignore malformed override config and fall back to canonical specs. - pass + _logger.warning( + "E2E_MODEL_TP_OVERRIDES contains malformed JSON; using canonical specs" + )
d7e302f to
3b5d7f3
Compare
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)
114-123:⚠️ Potential issue | 🟡 MinorNormalize whitespace in
modelsinput before exact-match filtering.Comma-separated inputs often include spaces (e.g.,
"a, b"), which will cause the exactgrep -Fqto miss matches. Strip whitespace before matching.Proposed fix
- MODELS="${{ github.event.inputs.models || 'all' }}" + MODELS_RAW="${{ github.event.inputs.models || 'all' }}" + MODELS="$(echo "$MODELS_RAW" | tr -d '[:space:]')" RUNTIME="${{ github.event.inputs.runtime || 'all' }}"Also applies to: 224-233
🧹 Nitpick comments (1)
e2e_test/infra/model_specs.py (1)
127-139: Consider logging when TP override JSON is malformed.The validation logic is thorough, but silently ignoring malformed JSON may make CI debugging harder if someone misconfigures
E2E_MODEL_TP_OVERRIDES.💡 Optional: Add debug logging for invalid overrides
+import logging + +logger = logging.getLogger(__name__) + def get_model_spec(model_id: str) -> dict: """Get spec for a specific model, raising KeyError if not found.""" if model_id not in MODEL_SPECS: raise KeyError(f"Unknown model: {model_id}. Available: {list(MODEL_SPECS.keys())}") spec = dict(MODEL_SPECS[model_id]) tp_overrides_json = os.environ.get("E2E_MODEL_TP_OVERRIDES") if tp_overrides_json: try: tp_overrides = json.loads(tp_overrides_json) if isinstance(tp_overrides, dict): override = tp_overrides.get(model_id) if isinstance(override, int) and override > 0: spec["tp"] = override + elif override is not None: + logger.debug("Ignoring invalid TP override for %s: %r", model_id, override) + else: + logger.debug("E2E_MODEL_TP_OVERRIDES is not a dict: %r", tp_overrides) except json.JSONDecodeError: - # Ignore malformed override config and fall back to canonical specs. - pass + logger.debug("Malformed E2E_MODEL_TP_OVERRIDES JSON, using canonical specs") return spec
3b5d7f3 to
c87aa0d
Compare
90d81af to
ff6847c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/nightly-benchmark.yml:
- Around line 42-44: actionlint flags the custom runner label "k8s-runner-gpu"
used in the build-wheel job as unknown; add that label to the actionlint
configuration (e.g., actionlint.yaml) under allowed runner labels so actionlint
recognizes "k8s-runner-gpu" (and any other custom labels) and stops reporting
false failures for the build-wheel job.
In `@e2e_test/fixtures/hooks.py`:
- Around line 108-116: The calculate_test_gpus function currently swallows
missing model IDs by returning 0 when get_model_spec raises KeyError; change
this to surface a hard failure by re-raising or raising a clear exception (e.g.,
raise KeyError or ValueError with a message like "Unknown model_id: {model_id}")
so mis-typed model markers fail fast; update the except block in
calculate_test_gpus to log/raise the descriptive exception instead of returning
0 so callers immediately see configuration errors.
- Move DeepSeek-R1-Distill-Qwen-7B from 8-gpu-h200 to k8s-runner-gpu/4-gpu-h100 for both single and multi worker jobs. DeepSeek has tp=1 and doesn't need H200. Only Llama-4-Maverick (tp=8) remains on 8-gpu-h200. - Switch ci_setup_python_venv.sh from system python3 to uv-managed Python 3.12. The 8-gpu-h200 runners have Python 3.10 which doesn't meet smg's >=3.12 requirement. Using uv to manage Python avoids depending on system packages.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/nightly-benchmark.yml:
- Around line 83-85: Update the misleading comment in the nightly-benchmark
workflow to match the matrix: change the note that currently says "DeepSeek runs
on H200; all other models run on k8s H100" to reflect that DeepSeek is on H100
and Llama-4 is on H200 (the same change should be applied to the repeated
comment at the later block around the other occurrence). Locate the comment near
the matrix entries for "DeepSeek" and "Llama-4" and edit the text so it
accurately states "DeepSeek runs on H100; Llama-4 runs on H200; all other models
run on k8s H100" (or equivalent phrasing matching the matrix).
In `@scripts/ci_setup_python_venv.sh`:
- Around line 8-12: The script currently pipes the remote uv installer via curl
| sh (the curl -LsSf https://astral.sh/uv/install.sh invocation and subsequent
export PATH), which is a supply-chain risk; update the call in the uv installer
block so it either downloads a pinned installer URL (e.g., use a specific
release path rather than /install.sh) or downloads the installer to a temporary
file, verifies its checksum/signature, and only then executes it (keep the same
PATH export). Apply the same pattern to the other scripts that call the
installer (the ci_install_vllm.sh and ci_install_sglang.sh invocations) so they
use pinned version URLs or checksum-verified downloads instead of piping an
unverified remote script to sh.
be3de81 to
5a1563f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/nightly-benchmark.yml:
- Around line 115-121: Normalize whitespace in the MODELS input before doing the
grep match: replace occurrences of surrounding whitespace around commas and trim
leading/trailing spaces so "modelA, modelB" becomes "modelA,modelB" and then use
that normalized variable in the existing SKIP check (the line using MODELS and
the grep with ,${{ matrix.model.id }},). Apply the same normalization in both
places the filter is implemented (the single-worker and multi-worker filter
blocks) and also mirror the change for the analogous block that uses the same
pattern later in the file.
uv venv doesn't seed pip by default, breaking downstream pip install steps. Revert to system python3 -m venv until H200 runners are updated.
5a1563f to
1174dba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/nightly-benchmark.yml:
- Line 218: Update the commented matrix entry for id
"meta-llama/Llama-4-Maverick-17B-128E-Instruct-FP8" to include the missing
fields so downstream parsing (fromJson(matrix.model.runs_on)) and artifact
naming work: add a slug (e.g.,
meta-llama-Llama-4-Maverick-17B-128E-Instruct-FP8), a runs_on JSON string (e.g.,
'["8-gpu-h200"]'), and gpu_type (e.g., H200) to the commented YAML line so it
becomes a complete matrix entry if re-enabled.
🧹 Nitpick comments (1)
.github/workflows/nightly-benchmark.yml (1)
167-167: Consider extractingE2E_MODEL_TP_OVERRIDESto reduce duplication.The same JSON string appears in both single-worker (line 167) and multi-worker (line 279). If TP values change, both must be updated. Consider defining this in the workflow-level
envsection or a reusable composite action.♻️ Example extraction to workflow env
env: RUSTC_WRAPPER: sccache SCCACHE_GHA_ENABLED: "true" E2E_MODEL_TP_OVERRIDES: '{"meta-llama/Llama-3.1-8B-Instruct":1,...}'Then reference as
${{ env.E2E_MODEL_TP_OVERRIDES }}in job steps.
| - { id: Qwen/Qwen3-30B-A3B, slug: Qwen-Qwen3-30B-A3B, test_class: TestNightlyQwen30bMulti, runs_on: '["k8s-runner-gpu","4-gpu-h100"]', gpu_type: H100 } | ||
| - { id: mistralai/Mistral-7B-Instruct-v0.3, slug: mistralai-Mistral-7B-Instruct-v0.3, test_class: TestNightlyMistral7bMulti, runs_on: '["k8s-runner-gpu","4-gpu-h100"]', gpu_type: H100 } | ||
| - { id: openai/gpt-oss-20b, slug: openai-gpt-oss-20b, test_class: TestNightlyGptOssMulti, runs_on: '["k8s-runner-gpu","4-gpu-h100"]', gpu_type: H100 } | ||
| # - { id: meta-llama/Llama-4-Maverick-17B-128E-Instruct-FP8, test_class: TestNightlyLlama4MaverickMulti } # tp=8, keep disabled for nightly |
There was a problem hiding this comment.
Commented matrix entry still incomplete.
The past review flagged that this commented entry is missing slug, runs_on, and gpu_type fields. While marked as addressed, the current code still shows the incomplete format. If re-enabled without these fields, fromJson(matrix.model.runs_on) and artifact naming will fail.
# - { id: meta-llama/Llama-4-Maverick-17B-128E-Instruct-FP8, slug: meta-llama-Llama-4-Maverick-17B-128E-Instruct-FP8, test_class: TestNightlyLlama4MaverickMulti, runs_on: '["8-gpu-h200"]', gpu_type: H200 }🤖 Prompt for AI Agents
In @.github/workflows/nightly-benchmark.yml at line 218, Update the commented
matrix entry for id "meta-llama/Llama-4-Maverick-17B-128E-Instruct-FP8" to
include the missing fields so downstream parsing
(fromJson(matrix.model.runs_on)) and artifact naming work: add a slug (e.g.,
meta-llama-Llama-4-Maverick-17B-128E-Instruct-FP8), a runs_on JSON string (e.g.,
'["8-gpu-h200"]'), and gpu_type (e.g., H200) to the commented YAML line so it
becomes a complete matrix entry if re-enabled.
- Extract H200 models (Llama-4-Maverick) into separate single-worker-h200 job so H200 pool runs independently from H100 max-parallel limits. H200 runners were idle waiting behind H100 jobs in the shared queue. - H100 jobs: runs-on moved to job level, removed per-model runs_on/gpu_type from matrix entries since all H100 models share the same runner config. - H200 jobs: include actions/setup-python@v6 for Python 3.12 since H200 runners have Python 3.10 which doesn't meet smg's >=3.12 requirement. - multi-worker-h200 commented out (Maverick multi already disabled). - Update summarize-benchmarks to depend on single-worker-h200. - Fix docstring in test_nightly_perf.py to reflect multi-runner setup.
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)
113-122:⚠️ Potential issue | 🟡 MinorTrim whitespace from the
modelsinput before matching.The filter logic doesn't handle spaces in comma-separated input. If a user enters
"modelA, modelB"(with spaces), thegrep -Fqmatch will fail because it looks for, modelBinstead ofmodelB.🔧 Proposed fix
run: | MODELS="${{ github.event.inputs.models || 'all' }}" + MODELS="$(echo "$MODELS" | tr -d '[:space:]')" RUNTIME="${{ github.event.inputs.runtime || 'all' }}" SKIP="false" if [ "$MODELS" != "all" ] && ! echo ",$MODELS," | grep -Fq ",${{ matrix.model.id }},"; thenApply the same fix to the filter blocks in
multi-worker(line 217),single-worker-h200(line 315).
🧹 Nitpick comments (1)
.github/workflows/nightly-benchmark.yml (1)
158-159: Consider extractingE2E_MODEL_TP_OVERRIDESto a reusable location.This JSON is duplicated at line 263 (multi-worker job). If model TP requirements change, both locations must be updated manually, risking drift.
Options:
- Define as a workflow-level environment variable
- Use a composite action or reusable workflow
- Store in a JSON file and read it in the step
- Set _TEST_MODE = False to run full benchmarks instead of reduced test runs. - Enable daily cron schedule at midnight (was disabled for PR testing). - Remove pull_request trigger since nightly benchmarks shouldn't run on PRs.
Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
1 participant
Summary
org/model)modelslug + safe folder names)Additional fixes
codespell/ruff/ruff format)--reasoning-parser=gpt-osswhere parser type is requiredValidation
Summary by CodeRabbit
Tests
Chores
Documentation