Repository navigation
[Performance] Optimize MammothModa2 AR → DiT hidden-state transfer - #7102
linyueqian merged 23 commits into
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/input_output_modality_contracts.md. Module owners: @Sy0307 @amy-why-3459 @Gaohan123 @alex-jw-brooks @kunkunblueberry, 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. |
c93f4db to
08e86da
Compare
|
Self-review completed. I reviewed the full MammothModa2 AR → DiT path, including the request-end hidden-state accumulation, full-payload handoff, DiT-side input handling, scheduler gating, and compatibility with the existing For Phase 1, the main work is now complete and validated end to end: AR hidden-state slices remain device-resident during request execution, per-step host materialization is removed, and the final payload is materialized once at request end through the existing OmniPayload / transfer-manager / connector path. Focused regression tests pass ( The remaining limitation is the actual cross-stage transport. The existing generic vLLM-Omni connector path still uses host-mediated transfer; there is currently no directly reusable true GPU-to-GPU D2D transport interface that I can plug into this model path without changing the underlying communication infrastructure. Therefore, I have intentionally kept this PR scoped to the model-side/request-end transfer optimization and left the generic connector backend unchanged. The remaining work is to profile the request-end D2H/H2D and serialization overhead, then determine whether a separate GPU-aware transport implementation (e.g. CUDA IPC / NCCL / UCX / asynchronous P2P transport) is justified. So the current status is:
|
08e86da to
2553778
Compare
2553778 to
226af4c
Compare
|
Reviewed head The three available targeted suites passed 89 tests (stage-input processor, GPU AR runner, and scheduling coordinator). No new correctness regression was confirmed, but the lifecycle and memory tradeoffs need further validation. [P2, inherited lifecycle follow-up] Reconcile accumulated states on preemption/resume. The GPU snapshot path feeds an append-only accumulator that survives preemption. Recomputed positions can be appended again; a method-level replay probe produced 9 hidden rows where 5 were expected. The adapter's length check rejects the payload, and the connector skips delivery. The legacy host-side path also has this alignment limitation, so this is an inherited follow-up rather than a newly introduced regression. Please add scheduler-driven preemption/resume and cancellation coverage, checking downstream completion and release of retained tensors. A synthetic 4,160 × 3,584 BF16 trajectory using the snapshot/accumulator methods retained 28.44 MiB on GPU and peaked at 56.88 MiB during concatenation. Releasing the probe references returned memory to baseline; this does not establish engine-level completion/cancellation cleanup. Please account for active and paused requests and concurrent completion—the AR profile permits 100 sequences. Requested A/B evaluation and detailed profiling Please compare a pinned baseline with the final PR head on the same hardware, checkpoint revision, runtime versions, device placement, memory budgets, prompts, seeds, resolution, steps, and CFG. Keep unrelated features fixed. Include representative short/long prompts and image sizes, single-request latency, and a concurrency sweep within supported limits. Please provide:
The PR's reported command references Please also coordinate with #7119: both PRs add the same stage-input test file, and enabling prefix caching bypasses deferred D2H. Preserve both test suites and evaluate that combination separately. |
Performance StatusPhase 1 has now been validated end to end on 2 × A100 40GB. The request-end implementation successfully removes the per-step hidden-state host materialization from the MammothModa2 AR path. In a worker-local PyTorch profiler capture, stage-0 However, this result should not yet be interpreted as an end-to-end latency improvement or as elimination of the cross-stage host transfer. The existing connector path is unchanged and still performs host-mediated transport after the request-end payload is materialized. A first Nsight comparison was collected, but the baseline and optimized captures covered unequal sets of spawned CUDA workers ( The current status is therefore:
The remaining performance work is to obtain a worker-complete, apples-to-apples Nsight trace and attribute:
This profiling work will determine whether the remaining connector transport is significant enough to justify a separate Phase 2 investigation of GPU-aware asynchronous/P2P transport. |
|
fix DCO |
|
is there any A/B test result? |
Signed-off-by: kunkunblueberry <1833921874@qq.com>
Omni ReviewBot: three questions on the performance claim@kunkunblueberry 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. |
|
Thanks for the detailed review. I went back through the implementation and re-evaluated the design from the architecture, lifecycle, memory, and performance perspectives rather than continuing to optimize the current path incrementally. For this round, I decided to step back from the request-end GPU accumulation approach and restore MammothModa2 to the standard stage-forwarding behavior: The main reason is that Mammoth's DiT is not an incremental consumer. It needs the complete I also looked carefully at the memory trade-off. The full AR result has to be retained somewhere until AR completion in either design. The previous optimization mainly changed the residency: So the optimization does not eliminate the retention requirement; it moves the retention from CPU to GPU and changes when the host transfer/materialization happens. In return, it introduces additional GPU memory pressure, request lifecycle complexity, replay/preemption considerations, positional reconciliation, and more complicated failure handling. I did measure a real local effect from the optimization: the AR-side Based on that, I decided that continuing to add lifecycle- or generic-runtime-level machinery for the request-end optimization would not have a good complexity/performance trade-off for this PR. I therefore removed the optimization-specific pieces, including the retained GPU accumulator, positional reconciliation, the positional payload contract, the request-end BF16 payload path, and the associated tests. I also restored the scheduler/runner/connector changes so that the generic runtime no longer contains Mammoth-specific behavior. One architectural principle I tried to follow throughout this revision is: rather than making the generic scheduler, runner, coordinator, or connector aware of Mammoth-specific semantics. I explicitly revisited the places where it would have been possible to “fix” the problem by modifying the generic layer. In particular, the request-end implementation exposed questions around preemption/resume, retained state reconciliation, and producer-side error propagation. My conclusion was that these are not good reasons to introduce Mammoth-specific behavior into generic infrastructure when the existing framework already provides the standard completed-stage forwarding path. So the current revision is intentionally much smaller and more conservative. The production path is back to the model's standard: and the tests are focused on validating that actual topology rather than introducing a synthetic per-step DiT streaming test that does not match Mammoth's execution semantics. For validation, I have completed the local/static checks and replaced the optimization-specific tests with focused CPU adapter/topology/forwarding coverage. I have not yet run the Linux pytest suite or the 2xA100 E2E test because the available GPU resources are currently occupied. I don't want to claim those results before actually running them. I will run the Linux test suite first and then the fixed 2xA100 E2E validation with the same checkpoint, device placement, resolution, diffusion steps, and seed, and submit the resulting test evidence with the next update as soon as the resources are available. For the remaining performance question, I think there is still a potentially interesting direction, but I would prefer to discuss it separately rather than expanding this PR again. Instead of retaining the whole payload on GPU, a possible future design would be to pipeline: The potential benefit there would come from overlapping AR computation with data movement rather than simply reducing the number of host materializations. It may also have better concurrency and memory characteristics because the complete hidden state would not need to remain resident on the AR GPU. However, I have not assumed that the current Omni architecture supports this cleanly; it may require changes to the stage-transfer abstraction and generic runtime lifecycle. I would therefore prefer to keep that as a separate design/performance discussion after this PR is validated, rather than introducing architectural changes without first establishing that the existing abstraction can support them. At this point, I believe the current implementation is substantially cleaner and closer to the existing Omni architecture, and I have tried to keep the scope limited to the minimum model-specific changes required for MammothModa2. I think the main functionality is now addressed, but I would still appreciate reviewers' assessment of whether the current design and scope are appropriate before considering any further architectural changes. |
Signed-off-by: kunkunblueberry <1833921874@qq.com>
Keep MammothModa2 AR hidden states in their source BF16/FP16 dtype across the existing AR-to-DiT EngineCore payload boundary, and defer conversion until the DiT consumer moves conditions to the model device/dtype. This removes the BF16/FP16 -> FP32 -> payload -> BF16/FP16 expansion from the standard path without changing scheduler, connector selection, EngineCore APIs, request lifecycle, cancellation, queueing, stage topology, prefix cache, or chunked prefill behavior. The code change is intentionally limited to MammothModa2 AR-to-DiT bridge code plus regression tests. Benchmark details and the streaming/lifecycle analysis are PR self-review material rather than files committed to the patch. Validation: python -m compileall on changed Python files; git diff --check; pre-commit run --files on the two changed runtime files and MammothModa2 stage-input test with SKIP=actionlint,markdownlint-cli2 because no workflow/docs/README files are part of this patch. Relevant hooks passed: debug statements, EOF, line endings, trailing whitespace, ruff, typos, mypy, test marks, SPDX, forbidden imports, torch.cuda guard, Buildkite no-files, suggestion. Signed-off-by: kunkunblueberry <1833921874@qq.com>
b1b047f to
68f1d9f
Compare
|
Re-review for #7075 / v0.30.0. Tests below ran on [P1] Restore the existing Talker streaming-window handler. The scheduler deletion is still present, with the current path falling through at lines 818–819. Removing [P2] Fix the forwarding test's client fixture. The new head's fixture is still a Validation on PYTHONDONTWRITEBYTECODE=1 PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest \
-p pytest_mock -p no:cacheprovider -o addopts='' -q \
tests/core/sched/test_omni_ar_scheduler_streaming.py \
tests/model_executor/stage_input_processors/test_mammoth_moda2.py \
tests/distributed/omni_connectors/test_basic_connectors.pyA/B and profiling follow-up. GPU accumulation is removed, but the new head now preserves FP16/BF16 across AR-runtime review from @tzhouam or @yinpeiqi would help assess the scheduler regression. Preserve both PRs' stage-input tests when integrating with #7119. |
Restore the generic sender-only Talker streaming-window handler that was removed during the Mammoth forwarding rollback. Exercise Mammoth forwarding through the real stage-client helper. Signed-off-by: kunkunblueberry <1833921874@qq.com>
|
Re-review update for #7075 / #7102 at head 43d1e54. Reviewer findings
Current-head validation On the A100 test environment (Python 3.12.13, torch 2.13.0+cu132, vLLM 0.28.0), the exact reviewer-targeted command passed on this head: The warnings are the known vLLM/vLLM-Omni version metadata warning and pytest configuration warnings, not test failures. compileall over the touched Python code and git diff --check against the PR base also passed. The exact GitHub PR head has passing DCO, pre-commit, Python 3.11 build, Python 3.12 build, and ReadTheDocs checks. Performance evidence and its boundary The executable contents of the two Mammoth production runtime files in this head are identical to the saved A100 profiling candidate; their only difference is two SPDX header lines per file. Reusing those artifacts is therefore source-semantic, not a claim that a fresh A/B was run. The fixed 2x A100 measurements establish the payload effect: stage-1 pageable H2D payload -50.0%, total H2D bytes -44.0%, and GPU H2D time -10.9%. The 10-run E2E result was 6.367506 s -> 6.306694 s (-0.96%), but with substantial variance; it is directional evidence only, not a statistically significant end-to-end latency claim. Timed output hashes and peak GPU memory were unchanged. I also attempted one current-head fixed-seed AR-to-DiT smoke on the replacement server. Its image has vLLM 0.28.0 but lacks vllm.entrypoints.generate.base.protocol, which the current Omni API imports before model initialization. The attempt stopped before CUDA/model execution. I did not alter the shared environment or claim that fresh forwarding passed; the saved candidate output and the source-equivalence check are recorded separately. A full fresh integration smoke should be rerun only in the compatible vLLM environment. Scope This PR preserves the existing completed-AR stage handoff and only removes the unnecessary FP32 expansion in Mammoth-specific payload handling. It does not claim streaming D2H/H2D, prefix-cache/chunked-prefill reconstruction, cross-request overlap, staging-pool ownership, cancellation handling, or async lifecycle safety. Those require a separate Layer-2 request-lifecycle design and tests. |
|
Decision-log update for head 43d1e54 This updates the earlier smoke caveat: the current head has now been exercised through a real two-stage Mammoth request. Reviewer regression validation The requested suites were rerun on the detached current-head worktree: P1 remains a restoration: the scheduler has no net diff from PR base bad5098. P2 uses a real StageEngineCoreClient instance and reaches production client dispatch. Current-head Mammoth forwarding / E2E smoke The fixed MammothModa2-Preview request ran on 2x NVIDIA A100-PCIE-40GB with Python 3.12.13 and torch 2.13.0+cu132:
The replacement server still carries vLLM 0.28 while this rebased Omni head targets vLLM 0.29. To avoid claiming an untested compatibility shim, the smoke used an ephemeral physical runner that changed only the saved harness import of clone_sampling_params to the equivalent copy.deepcopy function. This bypasses the new OpenAI package import unavailable in vLLM 0.28; it does not alter product source, deployment YAML, model weights, request data, stage topology, or the AR-to-DiT path. The temporary runner was deleted after the run. This is valid current-source forwarding evidence, but it is not a substitute for a future vLLM-0.29 compatibility-matrix run. Static gates and hygiene
No A/B or Nsight workload was rerun. The prior transfer measurements remain applicable because the two Mammoth runtime files have unchanged executable content relative to the saved candidate, apart from SPDX headers. No streaming, prefix-cache/chunked-prefill lifecycle, or cross-request async-safety claim is added. |
Signed-off-by: kunkunblueberry <1833921874@qq.com>
linyueqian
left a comment
There was a problem hiding this comment.
Approving. The change is small and the justification is the part that matters, so I checked it rather than taking it on trust. The comment being deleted claimed the float32 widening existed because "the serializer uses numpy, which has no bf16". That is no longer true: _SAFE_TORCH_DTYPES in kv_transfer_manager.py lists bfloat16 explicitly, and this PR adds test_bfloat16_tensor_serialization proving OmniSerializer round-trips a bf16 tensor with both dtype and values intact. Pinning the assumption with a test instead of asserting it in a comment is the right way to retire a stale one.
The other half also holds. Dropping .to(dtype=torch.float32) from _split_ar_conditions is safe because forward already casts both conditions at lines 266-267 with .to(device=model_device, dtype=target_dtype), where target_dtype comes from the refiner's own parameters, so the selected rows reach the model in exactly the dtype they did before. What changes is only that they stop being widened while sitting on the host and crossing the IPC boundary, which is where the doubled staging footprint and H2D traffic came from.
CI: buildkite/vllm-omni green at this head. Intel and AMD carry the standing inherited reds, covered by ci_approval_override_head.
Validation: static review of the four-file author diff taken from the GitHub file list rather than a merge-base diff, since this branch has main merged into it and a merge-base comparison shows 85 unrelated files. Verified the serializer dtype table, the new round-trip test, and the downstream cast in forward. Fork head, so the branch was not executed.
…llm-project#7102) Signed-off-by: kunkunblueberry <1833921874@qq.com> Signed-off-by: kunkun <72174834+kunkunblueberry@users.noreply.github.com> Signed-off-by: Matthieu Laneuville <matthieu.laneuville@surf.nl>
…llm-project#7102) Signed-off-by: kunkunblueberry <1833921874@qq.com> Signed-off-by: kunkun <72174834+kunkunblueberry@users.noreply.github.com>
Summary
This PR has gone through several iterations around MammothModa2 AR -> DiT hidden-state transfer. The current final direction is intentionally narrower than the earlier request-end/full-payload experiments:
The latest submitted code should be reviewed as a measured Layer-1 payload optimization, not as a new streaming transport runtime.
Related issues: #7075, #7087
Current code scope
The final precision optimization changes only MammothModa2 bridge code and tests:
vllm_omni/model_executor/stage_input_processors/mammoth_moda2.pyfull_hidden_statesto FP32 inar2dit; keep the AR source dtype and make the payload contiguous.vllm_omni/diffusion/models/mammoth_moda2/pipeline_mammothmoda2_dit.py.to(device=model_device, dtype=target_dtype)conversion.tests/model_executor/stage_input_processors/test_mammoth_moda2.pyNo benchmark scripts, raw logs, Nsight traces, or local evidence archives are committed in this update.
Why the earlier broader path was not retained
Earlier versions of this PR explored a request-end full-payload path. That direction attempted to retain AR hidden-state slices on the producing device, accumulate them across a request, and forward a request-end payload to DiT through a fuller cross-stage handoff.
That exploration was useful, but it crossed from a transfer optimization into request lifecycle design. In particular, it touched assumptions around:
The lifecycle risk is not theoretical. If a producer-side or streaming payload buffer is reused before all async copy/consumer dependencies are resolved, then a later request can observe stale readiness or consume another request's data. That class of problem needs an explicit Layer-2 contract, not a local patch hidden inside the MammothModa2 adapter.
The PR was therefore pulled back to the standard AR -> DiT forwarding path. The final retained optimization is only the precision/payload-volume point that has direct evidence and a much smaller compatibility surface.
Data-path change
Before this update, the effective handoff expanded the AR tensor before transport:
The current path is:
This changes the transport representation only. It does not change token order, answer boundary, generated-token look-ahead handling, masks, or the arithmetic dtype used by the DiT model.
Correctness contract
The payload still represents a complete logical AR sequence:
[logical_token_rows, hidden_size]answer_start_index = len(prompt_token_ids)token_id >= gen_vocab_start_indexThe EngineCore additional-information serializer already stores tensor payloads as raw bytes plus shape and dtype metadata, so BF16/FP16 can cross this boundary without introducing a new serializer protocol.
Prefix-cache and chunked-prefill reconstruction are deliberately not solved here. If AR execution skips cached prefix rows or splits prefill chunks, upstream must still reconstruct the complete logical hidden-state/token sequence before this MammothModa2 AR -> DiT boundary.
A/B and profiling evidence
A100 end-to-end A/B
Environment:
2.13.0+cu132, CUDA13.2;/data/vllm-workspace/models/MammothModa2-Preview;256 x 256;2;142;All corresponding timed output image hashes matched between baseline and candidate.
Interpretation: the end-to-end result is directionally positive, but because the sample variance is non-trivial, I do not want to overclaim this as a statistically significant E2E speedup. The stronger evidence is at the payload/transfer boundary.
100-run Layer-1 payload microbenchmark
Workload:
1824;256;1568;4096;The payload round trip preserved shape, dtype, contiguity, and values. The text/image condition shapes were unchanged.
Nsight Systems A/B
The Nsight comparison used the same A100 host, model, prompt, seed, resolution, and two diffusion steps. It captured stage-0 and stage-1 worker activity, CUDA kernels, memcpy records, and GPU allocation records.
cudaMemcpyAsyncAPI timecudaStreamSynchronizecountThis confirms the payload-volume effect. It also confirms the boundary of the change: D2H is not optimized by this patch, allocation behavior is unchanged, and the production path is not streaming.
V100 compatibility proxy
A separate FP16-only proxy ran on a shared Tesla V100S-PCIE-32GB. The V100 environment had an older vLLM/vLLM-Omni runtime and no matching Mammoth checkpoint/API, so this is supporting transport evidence rather than current-branch Mammoth end-to-end evidence.
This proxy supports the precision/payload direction and the future streaming hypothesis, but it is not used as an E2E production claim.
Why streaming is deferred
The current MammothModa2 condition path is still:
This path does not directly use
SharedMemoryConnectorfor the Mammoth condition handoff. Replacing the default connector would therefore not fix this path without first changing the stage topology and lifecycle contract.A production streaming design would need at least:
Without those pieces, the optimization can corrupt multi-request behavior: request A can still have an async copy in flight while its staging slot is reused for request B; a delayed event from A can then mark B as ready, or B can consume A's payload. That is why I am not mixing streaming into this patch.
What this PR does not claim
This PR does not claim to implement:
Those are follow-up lifecycle/system problems. The current transfer-stage work found one low-risk optimization point with clear evidence; other candidates were either not on the current path, crossed into lifecycle architecture, or had poor expected return relative to risk.
Validation
Local/static:
Relevant pre-commit hooks passed:
actionlintandmarkdownlint-cli2were skipped for the focused local run because this final patch does not include workflow, docs, README, or benchmark files. The first full pre-commit attempt was blocked by local dependency/environment setup rather than by a code failure.Functional/performance validation is summarized above. The benchmark scripts, raw logs, image outputs, Nsight traces, and local evidence archive were kept outside the commit and are available locally if deeper forensic review is needed.
AI assistance disclosure
I used AI assistance to inspect the code path, compare alternative designs, draft tests, summarize benchmark evidence, and prepare this self-review. The final patch is intentionally small and should still be reviewed normally by maintainers.