Repository navigation
fix(benchmarks): increase PD benchmark timeout from 240s to 480s - #681
Conversation
📝 WalkthroughWalkthroughThe change adds HuggingFace cache directory mounting to the Docker configuration in the benchmarks test setup. The modification locates the HF_HOME environment variable (defaulting to ~/.cache/huggingface), and if present, mounts it as a volume into the container while setting the HF_HOME variable inside the container. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello, 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 a persistent CI issue where the PD benchmark ( Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request increases the timeout for the PD benchmark test to prevent CI failures, which is a necessary change. The implementation is straightforward. I have one suggestion to improve code maintainability by defining the new timeout value as a constant rather than using a magic number.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_pd_perf.py`:
- Line 22: Add a brief inline comment next to the timeout_sec=480 setting in
test_pd_perf.py explaining why the test needs an extended timeout (e.g.,
"extended to 480s because CI environment can take longer to boot/complete
performance setup or ray cluster provisioning; default is 240s"), similar to the
existing comment for max_requests_per_run so future maintainers understand the
rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c880591-aefb-4ad7-8b6f-f25f406c82e0
📒 Files selected for processing (1)
e2e_test/benchmarks/test_pd_perf.py
genai-bench runs inside an ephemeral Docker container (--rm) that has no access to the host's HuggingFace cache. Each invocation downloads the tokenizer fresh from HuggingFace, which intermittently hangs and causes benchmark timeouts (exit code -9). Mount the host's HF cache directory (~/.cache/huggingface) into the container so genai-bench reuses tokenizers already downloaded by the sglang workers that run on the host. Also revert the timeout_sec=480 workaround from test_pd_perf since the root cause is the HF download hang, not insufficient timeout. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
a41f8e4 to
ae122e0
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)
e2e_test/benchmarks/conftest.py (1)
56-66: 🧹 Nitpick | 🔵 TrivialMinor redundancy with existing
HF_HOMEpass-through.When
HF_HOMEis set in the environment and the directory exists, line 61 explicitly sets-e HF_HOME={hf_home}, but lines 64-66 will also add-e HF_HOME(pass-through). This results in Docker receiving the same variable twice, which is harmless but redundant.Consider excluding
HF_HOMEfrom the loop when it's already handled by the new mount logic:♻️ Suggested cleanup
if os.path.isdir(hf_home): cmd.extend(["-v", f"{hf_home}:{hf_home}", "-e", f"HF_HOME={hf_home}"]) + hf_home_handled = True + else: + hf_home_handled = False # Pass through environment variables the container may need - for var in ("HF_TOKEN", "HF_HOME"): + for var in ("HF_TOKEN",) if hf_home_handled else ("HF_TOKEN", "HF_HOME"): if os.environ.get(var): cmd.extend(["-e", var])Alternatively, simply remove
"HF_HOME"from the tuple since the new block now handles it explicitly (the mount logic covers both the env-set and default cases).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/benchmarks/conftest.py` around lines 56 - 66, The code redundantly adds HF_HOME twice to cmd; update the pass-through loop in conftest.py to skip HF_HOME when it's already handled by the hf_home mount logic (referencing the hf_home variable and the cmd.extend calls), e.g. either remove "HF_HOME" from the tuple and only iterate ("HF_TOKEN",) or add a guard inside the for loop (if var == "HF_HOME" and os.path.isdir(hf_home): continue) so HF_HOME is not appended twice.
🤖 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/conftest.py`:
- Around line 56-66: The code redundantly adds HF_HOME twice to cmd; update the
pass-through loop in conftest.py to skip HF_HOME when it's already handled by
the hf_home mount logic (referencing the hf_home variable and the cmd.extend
calls), e.g. either remove "HF_HOME" from the tuple and only iterate
("HF_TOKEN",) or add a guard inside the for loop (if var == "HF_HOME" and
os.path.isdir(hf_home): continue) so HF_HOME is not appended twice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6f844a4-a172-401a-bb4e-aea164201334
📒 Files selected for processing (1)
e2e_test/benchmarks/conftest.py
1 participant
Summary
The PD benchmark (
test_pd_perf[pd_http]) consistently times out at the default 240s genai-bench subprocess timeout, causing CI failures with exit code -9.What changed
e2e_test/benchmarks/test_pd_perf.py: Added explicittimeout_sec=480to thegenai_bench_runnercallWhy
PD setup spins up 4 SGLang workers (2 prefill + 2 decode) plus a disaggregated gateway, which takes significantly longer to complete 200 requests compared to regular benchmarks. The default 240s timeout (from
conftest.py) is insufficient.Test plan
genai-bench timed out after 240s)Summary by CodeRabbit