Repository navigation
Conversation
|
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/entrypoints.md. Module owners: @alex-jw-brooks @linyueqian @NickCao @wtomin, 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. |
|
I simply use
|
|
I hit the same issue when testing Wan 2.2. It seems #5676 allows the legacy Since #1851 made cc @LyxWxj |
|
I investigated the usages of I recommend removing od_config.max_num_seqs = batch_sizeAs a result, Current usage of
|
@yJader Thanks for your comments. I believe #5676 worked fine with omni = AsyncOmni(
model=args.model,
diffusion_batch_size=configured_batch_size, # configured_batch_size is defined via args.batch_size
request_batch_max_wait_ms=250.0 if concurrency > 1 else 0.0,
enforce_eager=True,
dtype="bfloat16",
boundary_ratio=0.875,
flow_shift=5.0,
log_stats=True,
)The major problem this PR tries to solve is when enabling request-level batching in online serving mode, As for whether we should remove |
|
I also noticed this bug and set diffusion_batch_size = int(
explicit_batch if explicit_batch is not None else max_num_seqs
)in verl-omni in verl-project/verl-omni#408. But this PR is still worth merging for root cause fixing |
There was a problem hiding this comment.
Pull request overview
This PR fixes omni online serving so the diffusion stage’s effective request batching follows the standard --max-num-seqs knob when diffusion_batch_size is not explicitly provided, avoiding the previous silent fallback to batch size 1.
Changes:
- Update
OmniBase.__init__to resolvediffusion_batch_sizefrommax_num_seqswhen not explicitly set. - Preserve explicit
diffusion_batch_sizeprecedence while keeping the fallback behavior at 1 when neither value is present. - Add inline rationale documenting that diffusion stage init overwrites
od_config.max_num_seqswith the resolved batch size.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Stage init overwrites ``od_config.max_num_seqs`` with this value. | ||
| # ``vllm serve --omni --max-num-seqs N`` is the documented request-batch | ||
| # knob; use it when ``diffusion_batch_size`` is not passed explicitly. | ||
| explicit_batch = kwargs.pop("diffusion_batch_size", None) | ||
| max_num_seqs = kwargs.get("max_num_seqs") or 1 | ||
| diffusion_batch_size = int(explicit_batch if explicit_batch is not None else max_num_seqs) |
linyueqian
left a comment
There was a problem hiding this comment.
Reviewed at 4d003faa. Current main already fixes the original problem through the scheduler path added in #6525: --max-num-seqs reaches per-stage runtime overrides and od_config.max_num_seqs, which is the value the diffusion scheduler uses for request concurrency.
This PR only changes the legacy diffusion_batch_size value passed through AsyncOmniEngine and StageRuntime. The subprocess client logs it, and the inline client stores it only for logging. It never changes scheduler capacity. The new comment therefore contradicts the current implementation and its tests.
Please close this PR as obsolete rather than rebasing it. If there is a separate intended use for the legacy client batch value, that should be demonstrated through a current consumer and a focused test.
Validation: static review of the changed file plus the current merged tree and diffusion client/runtime call path. No PR code was executed locally.
| async_chunk = kwargs.get("async_chunk") | ||
| output_modalities = kwargs.pop("output_modalities", None) | ||
| diffusion_batch_size: int = kwargs.pop("diffusion_batch_size", 1) | ||
| # Stage init overwrites ``od_config.max_num_seqs`` with this value. |
There was a problem hiding this comment.
[blocking] This comment is false on current main. #6525 removed the batch_size -> od_config.max_num_seqs overwrite and made --max-num-seqs flow through stage runtime overrides into the diffusion scheduler directly. The legacy value changed here is only logged by the subprocess and inline clients, so this fallback cannot increase request concurrency. The intended fix has already shipped; please close this PR as obsolete rather than rebasing an ineffective assignment.
Omni ReviewBot: no human activity for 20 days@wtomin this pull request has had no human commit, comment or review since 2026-09-02. Please consider marking this PR as draft until work can resume. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Omni ReviewBot routing recordAssigned Strict on zcode (GLM-5.3-Flash) under experiment |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: 3e94d4ed-a015-490a-8f91-c499eebe9305) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: d90e3b0f-d209-45ff-91ce-f63541b755ac) — check |
Omni ReviewBot attempt recordReview attempt ended as failed (step 'review' (agent.review_diff): unhandled error: RuntimeError: zcode exited 1 without a result event: statusCode: undefined } Error: Turn execution failed (traceId: ee40a5c2-4d03-4c17-9d22-226aeab11648) — check |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
Changes since the previous review
- 0 new inline finding(s); 1 finding(s) below.
CI at
4d003faac45d(2026-10-10T03:02:10.893629+00:00): verification incomplete; required-check status is unknown. Observed Buildkite:buildkite/vllm-omni(passed).
Note: The assigned review arm
strict/zcode/GLM-5.3-Flashcould not complete this review, so it was produced by the fallback armdirect/cursor/auto. It is excluded from the routing experiment.
Full review analysis
PR description
This PR changes OmniBase.__init__ so an omitted diffusion_batch_size is copied from max_num_seqs, and stays 1 only when that value is also missing. An explicit diffusion_batch_size still wins, and max_num_seqs is left in the kwargs forwarded to AsyncOmniEngine. The intended effect is that vllm serve --omni --max-num-seqs N supplies diffusion batch size N instead of the previous implicit default of 1. The stage-init code that the new comment says writes this value onto od_config.max_num_seqs is not in the materialized snapshot.
Change flow
flowchart LR
cli["[EXISTING] explicit max_num_seqs kwarg"]:::existing
explicit["[EXISTING] explicit diffusion_batch_size"]:::existing
resolve["[CHANGED] OmniBase.__init__ default"]:::changed
engine["[EXISTING] AsyncOmniEngine"]:::existing
cli --> resolve
explicit --> resolve
resolve --> engine
classDef existing fill:#e5e7eb,stroke:#6b7280,color:#111827
classDef changed fill:#fef3c7,stroke:#d97706,color:#451a03,stroke-width:2px
classDef new fill:#dcfce7,stroke:#16a34a,color:#052e16,stroke-width:2px
classDef removed fill:#fee2e2,stroke:#dc2626,color:#450a0a,stroke-width:2px
Findings
- [P1] Diffusion batch default has no regression test or test plan —
vllm_omni/entrypoints/omni_base.py:185
Existing thread: #6405 (comment)
Evidence for Diffusion batch default has no regression test or test plan
OmniBase.__init__ now sets diffusion_batch_size from max_num_seqs when diffusion_batch_size is omitted (line 185) and passes that value into AsyncOmniEngine (line 210). The diff adds no test for max_num_seqs=4 forwarding batch size 4, for an explicit diffusion_batch_size still winning, or for the missing-both fallback of 1. The PR body also has no Test Plan or Test Result. On this tree, .buildkite/cuda/test-ready.yml step "Simple · Engine&Entrypoints Test" runs pytest -sv tests/entrypoints tests/engine -m 'core_model and cpu', and "Entrypoints Test" runs pytest -sv tests/entrypoints/ -m 'core_model and cuda' --run-level 'core_model'. The PR does not report either selector. Without that pin, the online path can regress to batch size 1 again with no failing test. Add a constructor unit test for those three states and record the CPU entrypoint selector in the test plan.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
Omni ReviewBot: finding feedback[p1] Diffusion batch default has no regression test or test plan — See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Summary
This PR makes
vllm serve --omni --max-num-seqs Ncorrectly control the default diffusion request batch size when--diffusion-batch-sizeis not explicitly provided (BTW,--diffusion-batch-sizeis not a valid CLI argument forvllm serve).Previously, even if users started omni serving with:
the diffusion model could still run with
diffusion_batch_size=1, so the documented request batching knob did not actually take effect for diffusion workloads.While using
AsyncOmni(diffusion_batch_size=N)is working correctly, this PR wants to enable diffusion batching currently for online entrypoint.Motivation
During diffusion stage initialization,
od_config.max_num_seqsis overwritten by the resolved diffusion batch size:Because of this, the effective diffusion batching capacity is determined by
diffusion_batch_size.Before this change, when
diffusion_batch_sizewas not explicitly provided, it defaulted to1. This caused the following behavior:--max-num-seqs Nwas accepted by the server configuration.od_config.max_num_seqswithdiffusion_batch_size.diffusion_batch_sizewas still1, diffusion request batching remained effectively limited to batch size1.--max-num-seqsis the documented request batching knob.What Changed
This PR changes omni engine argument handling so that
diffusion_batch_sizeis resolved frommax_num_seqswhen it is not explicitly set:The new behavior is:
diffusion_batch_sizeis explicitly provided, it still takes precedence.diffusion_batch_sizeis not provided, it defaults tomax_num_seqs.1.Effect After This Change
After this PR, users can configure diffusion request batching with the standard serving option:
In this case, the diffusion model will use an effective batch size of
4, unless--diffusion-batch-sizeis explicitly set to another value. An example output is:[RequestBatch] admission wait done waiting=4 max_batch=4 waited_ms=0.0shows the effective batch size. Before this change, when the effective batch size is 1, theWan22Pipeline.diffusetook around 37s.This makes omni serving behavior match user expectations and avoids a silent mismatch where
--max-num-seqsappears configured but diffusion execution still runs with batch size1.Backward Compatibility
This change preserves existing behavior for users who explicitly set
diffusion_batch_size.The only behavior change is for the implicit/default case: diffusion serving now follows the documented
--max-num-seqsbatching setting instead of silently falling back to1.