Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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 @NickCao Routing: @alex-jw-brooks via module of the changed files, CODEOWNERS; @Bounty-hunter via module of the changed files, CODEOWNERS; @NickCao via CODEOWNERS @RyanYun09, 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. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
7fc0579 to
a6affc4
Compare
linyueqian
left a comment
There was a problem hiding this comment.
Thanks, this matches the official evaluator. In OpenBMB/Omni-DuplexEval, batch_real_time_description.py passes non_empty_strings([answer1, answer2]) (strip, drop empty) and load_reference_texts deduplicates in first-occurrence order, matching what _gt_references does before build_content_prompt. The prompt wording and PROTOCOL_PIN are unchanged.
Validation: on main at b1f11f1 merged with a6affc4, CPU only, test_omni_duplex_eval_gt_references.py plus test_omni_duplex_eval.py gave 31 passed, 5 skipped. The three new cases fail on unpatched omni_duplex_eval_eval.py and pass with the fix. Small note for the description: the test plan runs -k official_gt_reference against test_omni_duplex_eval.py, but the new cases live in test_omni_duplex_eval_gt_references.py.
Filter empty answers and deduplicate references (first occurrence) before building the pinned content prompt, mirroring non_empty_strings + load_reference_texts in the official OpenBMB/Omni-DuplexEval evaluator. Add a regression test that captures the judge prompt for empty, duplicate, and whitespace-padded answers. Fixes vllm-project#9054 Signed-off-by: RyanYun09 <ryanyun0911@gmail.com>
a6affc4 to
59a60dc
Compare
natureofnature
left a comment
There was a problem hiding this comment.
Reviewed the full two-file change and the dataset → content-prompt path against OpenBMB/Omni-DuplexEval at the pinned revision. Empty/whitespace-only references are dropped, duplicates keep their first occurrence, and temporal/reminder prompts remain unchanged. The three prompt-capture regressions cover the reported cases; @linyueqian's 31-pass/5-skip CPU report uses the same patch and unchanged relevant evaluator, dataset and prompt code. Current head checks are green. This round was static review; I did not rerun tests.
【Conclusion: Found no blocking issues in commit 59a60dc.】
【Action recommended: None.】
Contributor intent (required human answers)
1. What outcome do you want to achieve?
Make vllm-omni's Omni-DuplexEval content-judge prompt match the official evaluator's GT reference handling, so an absent/empty or duplicated
answer1/answer2cannot produce a malformed or duplicatedReference annotationsblock.2. What is the highest-leverage variable for that outcome?
The caller-side reference set passed to
build_content_prompt. The prompt builder already matches the pinned official wording; the divergence is thatevaluate_samplepasses[answer1, answer2]verbatim, while the official pipeline appliesnon_empty_stringsand first-occurrence deduplication before prompt construction.3. How will this work move that variable?
Add
_gt_referencesinomni_duplex_eval_eval.pyto dropNone/blank values (strip-based, order-preserving) and deduplicate while keeping the first occurrence, then pass its result at the content-prompt call site. A dedicated regression test captures the judge prompt foranswer2 == "",answer1 == answer2, and whitespace-padded references.4. Whose customer or user needs does this address, and what are those needs?
Users of
vllm bench omni-duplex-eval --omni evaluateand anyone comparing vllm-omni scores against the official evaluator or another implementation. They need the same judge-prompt semantics so score comparisons are not polluted by gratuitous empty numbered slots or duplicate references.Purpose
Before this change,
evaluate_samplepassed[sample.answer1, sample.answer2]directly intobuild_content_prompt. Withanswer2 == "", the judge saw an empty numbered slot (1.); withanswer1 == answer2, it saw the same GT twice. The official OpenBMB/Omni-DuplexEval pipeline filters non-empty strings and deduplicates references before building the same pinned content prompt.After this change, the content prompt receives only stripped, non-empty, first-occurrence-unique references. The prompt wording and
PROTOCOL_PINare unchanged, and the temporal and proactive-reminder paths are untouched.The affected code is under the draft benchmarking module contract. This change does not alter public CLI options, defaults, configuration, or supported behavior, so no user-guide update is needed.
Fixes #9054
Test Plan
vLLM Version: 0.32.0
vLLM-Omni Commit: 59a60dc (base
324cb5d)Environment: OrbStack container
vllm-omni-029-up; Python 3.11.16, torch 2.13.0+cu130, pytest 9.1.1, PyAV 18.1.0.