Repository navigation
Conversation
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to belong to: docs/design/module/benchmarking.md. Module owners: @alex-jw-brooks @Bounty-hunter @natureofnature, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b956e9db6
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@ZacheryAU PTAL |
|
@amy-why-3459 @R2-Y PTAL |
|
Thanks @natureofnature — the split from #5102 is the right shape, and
Nits: this is named |
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
|
Correction to my previous repository-layout reply: the dedicated-subcommand interpretation is superseded. Updated in
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 247b2765d0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9cf06df94
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _clear_artifacts(directory, (*SUCCESS_ARTIFACTS, ".failed.json")) | ||
| _atomic_write_wav(directory / "output.wav", pcm, OUTPUT_SAMPLE_RATE) |
There was a problem hiding this comment.
Keep public artifact publication under the output lock
When two processes call the retained public run_omniinteract_case() for the same case/output root, or one does so while vllm bench serve holds its lease, these independent file replacements can still interleave: the public path never acquires omniinteract_output_lock, so one writer can remove another writer's files and expose a .done marker with a mixed bundle. Fresh evidence beyond the earlier artifact-lock thread is that a9cf06df removed the per-case locking from these helpers while adding the lease only around benchmarks.serve.main(); make the public mutation path participate in the same output-root lock.
Useful? React with 👍 / 👎.
cdfba3e to
e9bc9e6
Compare
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
e9bc9e6 to
8475872
Compare
| output.audio_duration = case_result.audio_bytes / (24_000 * 2) | ||
| output.audio_frames = case_result.audio_bytes // 2 | ||
| session_metrics = case_result.duplex_session_metrics | ||
| output.ttft = float(session_metrics.get("mean_ttft_ms") or 0.0) / 1000.0 |
There was a problem hiding this comment.
[P2] Do not use response-local timing as a session-global token timeline
output.start_time marks the beginning of the entire WebSocket/video session, while output.ttft is the mean delay from each response.created event and output.itl flattens intervals from multiple responses. Downstream metrics reconstruct absolute token timestamps as output.start_time + output.ttft + cumulative_itl.
For example, if a response is created 100 seconds into a video, its tokens are incorrectly placed near the beginning of the session. Flattening multiple responses also omits the first token and inter-response gap for every response after the first. This can substantially inflate or otherwise distort max_output_tokens_per_s and the token timeline plot.
Please retain each response's response.created offset relative to stream_start and construct the timeline per response. If exact absolute timing is unavailable, the OmniInteract backend should report max_output_tokens_per_s as unavailable instead of feeding response-local timing into the session-global metric.
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
Gaohan123
left a comment
There was a problem hiding this comment.
Please add some e2e benchmark results in the PR description. Thanks
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
|
@Gaohan123 Added current-head MiniCPM-o 4.5 E2E benchmark metrics for one |
This is the benchmark-runner split from #5102, after the runtime fixes in
#6360 and #6406. It intentionally leaves the 12-video Nightly workflow and
production configuration to a follow-up PR.
Purpose
vllm bench serve --omnidataset for theofficial
1q1a,1q1a_math, and1qnalayouts.the standard readiness, warmup, request-rate, concurrency, metrics, and
--save-resultlifecycle.with deterministic selection, safe atomic extraction, bounded media tools,
realtime pacing, and bounded concurrency.
transcript, and
.done; write evaluator-ready per-case artifacts plus thebatch summary and official manifest.
The PR does not change scheduler, connector, model execution, prompt rollover,
or production YAML behavior. It validates transport/lifecycle/artifacts but
does not judge answer accuracy. Nightly orchestration remains a separate
follow-up.
Reviewer follow-up
benchmarks/data_modules, options are registered incli_args.py, executionstays in
vllm bench serve, and usage is documented with other servingdatasets rather than in a standalone command or page.
timing. OmniInteract defaults to three prompts when
--num-promptsisomitted; explicit
0still selects all. The documentation states that allselected media remains resident in client memory and that concurrency does
not bound preparation memory.
_Playbackis the single serialized timeline for response ACK andWAV/transcript generation. Completion requires stable response identity and
the accepted final-input decision; invalid audio metadata and incomplete
response pairs fail rather than being silently accepted.
collectors, base64 audio, or a dense video-length buffer. WAV materialization
is one case at a time.
ffmpeg duration/output and the final Python PCM buffer are independently
bounded.
.donelast. The standard CLI holds one cross-process output-root lock forthe complete run, so case bundles and their aggregate manifest cannot be
interleaved by another process. The public case runner only captures artifact
context; the locked batch finalizer is the sole publication owner.
ineligible and omitted from
official_eval_manifest.jsonl. Artifactpublication failure revokes artifact/case success while preserving completed
serving metrics; recovery failures are logged and reported in the batch
summary instead of aborting metric finalization.
OPENAI_API_KEY, thenexplicit
--header, thenx-request-id. OmniInteract requires an explicit--endpoint /v1/realtime; an incompatible endpoint fails fast.response.createdas their origin.LISTEN-only sessions and missing stage-0 timing omit unavailable samples
instead of contributing false zeroes. Per-request missing TPOT remains aligned
with goodput inputs, measured zero latency remains valid, and exact ITL is
reported only when every response has complete stage-0 intervals.
while still accepting a target event already received by the collector.
hf_fs(). Archive extraction validatesmembers, publishes a fingerprinted tree atomically, and handles concurrent
publishers.
Usage
--endpoint /v1/realtimeis required because the upstream serving-benchmarkdefault targets the completions endpoint.
Test Plan
E2E Benchmark Result
Current head
7713e794, MiniCPM-o 4.5, one deterministic case per subset,--max-concurrency 1 --num-warmups 0:1q1a1q1a_math1qnaaudio_clipped=1)All three runs completed with
Successful requests: 1,Failed requests: 0,and a per-case
.doneartifact. The1qnarow is reported for lifecycle andperformance evidence only; its clipped playback horizon correctly kept it out
of
official_eval_manifest.jsonl. Peak output-token throughput is reported asunavailable for these Duplex sessions because response-local timings cannot
reconstruct a valid session-global token timeline.
Regression tests on the same code: focused OmniInteract/metrics selection
52 passed; full
tests/benchmarkssuite 179 passed.BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.
(anything written below this line will be removed by GitHub Actions)