Repository navigation
Conversation
📝 WalkthroughWalkthroughBenchmark execution moved from invoking a local CLI to running genai-bench inside a Docker container; CI workflows were updated to pull and use GENAI_BENCH_IMAGE and expose E2E_LOG_DIR; test infra now passes log_dir into ModelPool and captures per-worker logs. A new nightly model entry for openai/gpt-oss-20b was added. Changes
Sequence DiagramsequenceDiagram
participant CI as CI Workflow
participant Test as Test Runner
participant Docker as Docker Engine
participant Container as genai-bench Container
participant Router as Model Router/Server
participant Logs as E2E Log Dir
CI->>Docker: Pull GENAI_BENCH_IMAGE
Test->>Docker: Start container (docker run) with volumes & env (GENAI_BENCH_IMAGE, HF_*, E2E_LOG_DIR)
Docker->>Container: Launch genai-bench
Container->>Router: Connect to router_url (http / grpc) and run workload
Container->>Logs: Write results & per-worker logs to mounted E2E_LOG_DIR
Container-->>Docker: Exit when finished
Test->>Logs: Collect and parse results
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)
No actionable comments were generated in the recent review. 🎉 Comment |
Summary of ChangesHello @key4ng, 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 refactors the end-to-end benchmarking setup by transitioning 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 refactors the genai-bench execution in e2e tests to use Docker, which improves environment consistency. It also adds a new model to the nightly performance tests. My review includes a suggestion to improve the maintainability of the Docker image versioning and points out a side effect of the new model configuration that leads to redundant test runs.
key4ng
changed the title
Removed HF_HOME mounting and streamlined environment variable passing.
Removed user specification from Docker run command.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
e2e_test/benchmarks/test_nightly_perf.py (1)
98-103:⚠️ Potential issue | 🔴 CriticalTensor parallelism mismatch in nightly matrix—update tp from 1 to 2.
The model
openai/gpt-oss-20bexists in MODEL_SPECS withtp: 2(line 90, e2e_test/infra/model_specs.py), but the nightly matrix at line 102 specifiestp: 1. This configuration mismatch will cause runtime failures. Update to usetp: 2to match the model's definition.e2e_test/benchmarks/conftest.py (1)
35-95:⚠️ Potential issue | 🟡 MinorMount E2E_LOG_DIR into the container when it's outside the working directory.
If
E2E_LOG_DIRis set to an absolute path outside the base directory, the container won't have access to write logs there. The current code passes--log-dirwithout mounting the directory, causing writes to fail inside the container.🔧 Suggested fix
- base_dir = str(Path.cwd()) + base_dir_path = Path.cwd() + base_dir = str(base_dir_path) @@ - log_dir = os.environ.get("E2E_LOG_DIR") - if log_dir: - cmd.extend(["--log-dir", log_dir]) + log_dir = os.environ.get("E2E_LOG_DIR") + if log_dir: + log_dir_path = Path(log_dir) + if not log_dir_path.is_absolute(): + log_dir_path = base_dir_path / log_dir_path + log_dir_abs = str(log_dir_path.resolve()) + try: + log_dir_path.resolve().relative_to(base_dir_path) + except ValueError: + cmd.extend(["-v", f"{log_dir_abs}:{log_dir_abs}"]) + cmd.extend(["--log-dir", log_dir_abs])
🤖 Fix all issues with AI agents
In `@e2e_test/infra/model_pool.py`:
- Around line 327-341: The ModelPool currently accumulates open per-worker log
file handles (tracked only in self._log_files and closed in shutdown()), causing
FD leaks when instances are evicted and recreated; modify ModelPool to track log
file handles per instance key (the same key used in self.instances, e.g.,
"model_id:mode") and ensure the file handle for that key is closed and removed
from the tracking structure whenever an instance is evicted (the eviction path
that removes entries from self.instances), and also update all places that open
worker logs to register the handle under that instance key and to cleanly
close/remove it on eviction or final shutdown (references: ModelPool.__init__,
self._log_files, self.instances, shutdown()).
🧹 Nitpick comments (1)
.github/workflows/nightly-benchmark.yml (1)
152-155: Consider enabling E2E_LOG_DIR for multi-worker/H200 jobs too.Line 152: single-worker now sets
E2E_LOG_DIR; if you want per-worker logs consistently across nightly runs, mirror this env + mkdir in the multi-worker and single-worker-h200 blocks.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e_test/infra/model_pool.py`:
- Around line 589-610: If subprocess.Popen can raise after opening a log file,
ensure the opened file handle is closed and removed from self._log_files to
avoid FD leaks: wrap the Popen call in a try/except/finally (or try/except)
around the block where log_file is created (refer to variables log_file and
self._log_files and the Popen invocation that assigns proc), and on exception
close log_file and pop self._log_files[key] (and if created on disk optionally
delete the file); apply the same cleanup logic to the other launch path
referenced around lines 1257-1277 (the HTTP + gRPC launch sections).
|
stored genai-bench worker logs 
|
Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
2 participants
Description
Problem
Solution
Changes
Test Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit