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/vllm_omni_config.md. Module owners: @lishunyang12 @alex-jw-brooks @lsyyysky, 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. |
| # state is produced for it. | ||
| generated_token_ids = [20, 21, 22] | ||
| prompt = {"additional_information": {"image_height": [256], "image_width": [256]}} | ||
| miss = ar2dit([_ar_output(prompt_token_ids, generated_token_ids, miss_hidden)], prompt)[0] |
There was a problem hiding this comment.
Nit: the miss/hit path here is exercised entirely through mocks (SimpleNamespace input batch / completion) with the cache and ar2dit called directly, so the engine-level contract on a real cache hit — num_computed_tokens accounting feeding a truncated prefill into ar2dit — isn't covered. Worth a follow-up that drives the hit path through the actual stage-input-processor wiring.
|
The numerics evidence is currently n=1: one 34-token prompt, seed 42, one image, and "highly similar" as the qualitative bar. Since the whole reason this stays opt-in is that BF16 attention on a cache-hit tail isn't bitwise equivalent to full prefill, could we get validation over a real workload dataset instead of a single prompt? Specifically:
That would give the opt-in default a real evidence base. The CPU unit test + config layering look correct otherwise. |
|
Reviewed head [P2, existing coverage finding] Exercise a real cache hit through the engine and configured stage path. This follows up on the existing unresolved thread. At test lines 71–89, cached-token accounting and the hit marker are supplied manually, the cache helper is called directly, and fabricated completion objects are passed to Requested A/B evaluation and detailed profiling Please provide a reproducible harness covering A: caching disabled, B1: caching enabled with a verified miss, and B2: caching enabled with a verified hit. Keep code/checkpoint revisions, hardware, runtime versions, placement, prompts, seeds, resolution, steps, CFG, and other features identical. Warm model/kernel execution separately from prefix-cache state so startup or compilation does not explain the result. Record cache state and cached/created token counts for every request. Please include:
The current example invokes generation once and does not itself reproduce a warm cache-hit comparison. Please include the live repeated-request harness with the reported real-model evidence. Please coordinate with #7102: both PRs add the same test file, and prefix caching makes |
Omni ReviewBot: three questions on the performance claim@lsyyysky this PR reads as a performance or value claim:
Before the full evidence checklist, three short questions:
When you answer, the evidence that settles it is: base and head SHA, hardware, model, workload, warm-up and repeat count, mean or percentiles with their spread, and a correctness/quality-equivalence signal; an end-to-end claim also needs stage attribution. |
|
hi @hsliuustc0106 , Thanks for the review. The PR now includes:
I also tested the merged branch with #7102. The prefix-cache path passes, but #7102’s request-end payload path blocks before sending AR output to DiT, including with caching disabled. #7102’s flush lifecycle needs to be resolved first. |
|
Re-reviewed head The earlier mock-only integration finding is addressed. I independently ran the live-engine test: 1 passed in 274.29 s, plus 33 passing prefix-cache unit tests. The test executes real requests through the configured AR→DiT path, observes the actual Two benchmark issues remain: [P2] Accept genuine block-boundary hits. The B2 validator requires zero cache-creation tokens. A warmed 32-token prompt legitimately reports 16 cached and 16 created tokens, as the passing live test explicitly expects. An isolated call to the current validator rejects that valid sample with [P2] Sample the physical GPU that actually runs the engine. MemoryMonitor takes a raw NVML index, defaulting to physical GPU 0, while the deploy config uses logical CUDA device 0. With a GPU allocation exposing physical GPU 2, the engine runs on 2 but the default monitor samples 0 and can report the wrong peak memory/hardware. Resolve the engine's device to its physical NVML handle/UUID and validate any explicit override. This does not invalidate the supplied results if the author's engine actually ran on physical GPU 0. Validation environment: NVIDIA L20X, Python 3.12, torch 2.13.0, vLLM 0.28.0; MammothModa2-Preview checkpoint PYTHONDONTWRITEBYTECODE=1 PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest \
-p pytest_mock -p no:cacheprovider -o addopts='' --run-level=full_model -s -v \
tests/e2e/offline_inference/test_mammoth_moda2_prefix_cache.pyA/B and profiling assessment. The checked-in ten-request results support the reported 84.5% TTFT and 6.0% AR-only latency reductions for the synthetic 5,627-token workload. Recomputed AR latency mean ± sample SD: A Please retain the A/B1/B2 protocol and add quantitative image-quality comparisons across prompts/seeds, full transfer/DiT/E2E stage attribution, CPU hidden-state-cache memory, and accessible representative raw traces with commands and analysis of copies, synchronization and reconstruction. The mixed-batch test also currently checks accounting without comparing both members' conditioning to separate references; that is a useful next coverage extension. Benchmark review from @alex-jw-brooks or @Bounty-hunter would help. Preserve both stage-input test suites when integrating #7102. |
Omni ReviewBot triage noteResolved as of |
4dedfce to
011293c
Compare
|
@hsliuustc0106 Thanks for the detailed re-review. The requested follow-up validation is now available. Reviewer follow-up validationThe two benchmark P2 findings are addressed:
The mixed-batch E2E test now compares both warmed and cold members against CPU prefix-cache memoryThe B1/B2 CPU hidden-state cache allocates 1,062,813,696 bytes Full AR → DiT attributionFor the same 5,627-token, 50-step workload, using 10 unprofiled requests:
The earlier 5,986.45 / 5,997.09 / 5,634.94 ms A/B1/B2 values are AR-only Image qualityAcross three prompts × three seeds, cache-disabled A versus warm-hit B2:
The quality run used an independent request-local DiT seed fix, intentionally Full-pipeline profiler traces
Profiler runs are diagnostic only; the latency values above come from
Follow-up reproduction artifacts |
Signed-off-by: siyuan.lei <siyuanlei37@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…refix-cache Signed-off-by: siyuan.lei <siyuanlei37@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # benchmarks/mammoth_moda2/README.md
Thanks for the review. All three items have been addressed: Updated the E2E test to use ar2diffusion and OmniDiffusionSamplingParams, and added it to test-weekly.yml. It passes locally on A100 |
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: e8737c29-fb6a-48e9-9356-5b4dd28f44b3) — check |
|
@hsliuustc0106 The prefix-cache side looks right to me now: |
Signed-off-by: siyuan.lei <siyuanlei37@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ty |
|
Re-reviewed head
The earlier block-boundary validator, GPU mapping, numerical documentation, E2E migration, weekly CI wiring, and scope concerns are addressed in the current source. I found no new defect in the prefix-cache preset itself. Validation for this re-review: static syntax and diff checks passed; native/GPU tests were not rerun. Wheel builds, pre-commit, DCO, and CodeQL passed; docs remain pending. No matching-head Buildkite test result is reported. |
Signed-off-by: siyuan.lei <siyuanlei37@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: siyuan.lei <siyuanlei37@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
done |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Re-reviewed tip e22ee7a.
Production delta is the opt-in AR→DiT prefix-cache deploy preset plus docs/tests/benchmarks. ZhengWG's earlier scope/E2E/AR-yaml asks look addressed in source; Numerical Behavior doc fix is correct. I found no new defect in the preset or live-engine test shape.
Please (1) refresh the PR Purpose bullets — ar_prefix_cache.yaml is gone; (2) @ZhengWG re-ACK/dismiss the lingering CHANGES_REQUESTED on this tip so merge isn't blocked on a stale review. Optional: mention the preset in the MammothModa2 recipe.
|
@ZhengWG The earlier E2E, scope, and AR-only preset concerns have been addressed. The PR description is now updated to match the current source. Could you please re-ACK the latest head and dismiss the stale CHANGES_REQUESTED review? Thanks! |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Re-reviewed tip 7e111ca (main merge only since e22ee7a).
Still no production defect in the opt-in AR→DiT prefix-cache preset, docs, or live-engine E2E shape. Code remains APPROVE-ready from my side.
Process blockers for merge:
- @ZhengWG please re-ACK/dismiss the lingering CHANGES_REQUESTED on 43590f2 — your Oct-13 functional ACK already covers the tip intent.
- Please drop the stale Purpose bullet about a CPU stage-input unit test (not in the tip file list). Optional: mention mammoth_moda2_prefix_cache.yaml in the MammothModa2 recipe.
|
@hsliuustc0106 Sorry for the delay. Approved last head now. cc @lsyyysky |
Omni ReviewBot review statusReview for commit |
Omni ReviewBot routing recordAssigned Strict on claude-code (qwen3.8-max) under experiment |
Omni ReviewBot routing recordAssigned Strict on claude-code (qwen3.8-flash) under experiment |

Purpose
Refs #7075.
Add opt-in automatic prefix caching for MammothModa2:
mammoth_moda2_prefix_cache.yamlfor AR → DiT generation.Prefix caching remains opt-in because BF16 attention may produce numerical differences between full prefill and cache-hit tail execution.
Test Plan
CPU unit test
Live-engine E2E test
MAMMOTH_MODA2_TEST_MODEL=/root/models/MammothModa2-Preview \ pytest -s -v -o addopts='' \ tests/e2e/offline_inference/test_mammoth_moda2_prefix_cache.py \ --run-level=full_modelThe E2E test:
Omniengine withmammoth_moda2_prefix_cache.yaml.token request whose first cache block differs.
warmed.cached_tokens > 0andcold.cached_tokens == 0.ar2diffusionresult without replacing it.Performance protocol
All performance measurements use:
MammothModa2-PreviewConfigurations:
cached_tokens=0.vLLM Version: 0.28.0
vLLM-Omni Commit: 86546ef
Test Results
Unit and live-engine tests
1 passed in 154.16s.For the 28-, 32-, and 37-token E2E prompts, cache-hit requests reuse 16, 16, and 32 tokens respectively.
The test verifies:
full_hidden_states.shape[0] == len(full_token_ids).0.999.0.999.0.8.A/B1/B2 Performance
Long-prompt workload
The long workload uses a synthetic 5,627-token prompt to measure prefix-cache scaling. B1 creates 5,616 cache tokens on every request, while B2 reuses 5,616 tokens and computes only the remaining 11 prompt tokens.
A — cache disabled:
B1 — cache enabled, verified miss:
cached_tokens=0,cache_creation_tokens=5616B2 — cache enabled, verified hit:
cached_tokens=5616,cache_creation_tokens=0B2 compared with B1:
Shorter-prompt behavior
With a 587-token prompt and the same 273-token output:
For this shorter prompt, the approximately 19 ms prefill saving is too small relative to the 273-token AR decode and normal run-to-run variation, so it does not produce a measurable E2E improvement.
The long-prompt result demonstrates that prefix caching improves E2E latency once prefill becomes a sufficiently large portion of the request.
Profiler Analysis
A diagnostic PyTorch Profiler comparison was collected separately using the 587-token AR-only workload. Profiler timings are not used as formal latency measurements.
Observed behavior:
The trace confirms that prefix caching skips the expected prefill work. It does not optimize autoregressive decode, explaining why E2E benefit depends strongly on the ratio of prompt length to generated length.
The profiler workload is AR-only and therefore does not quantify DiT execution or AR-to-DiT H2D transfer.
Numerical Behavior
The cached prefix is reconstructed exactly. The newly computed BF16 tail is not guaranteed to be bitwise equivalent to full prefill because attention executes with a different query shape.
The following image is retained as an illustrative manual comparison and is not treated as multi-prompt or cross-seed quantitative evidence:
Reproducibility artifacts