Repository navigation
[Bugfix] Keep diffusion LoRA buffers resident on the compute device - #8874
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/diffusion/index.md, docs/design/module/diffusion/diffusion_model_integration.md. Module owners: @david6666666 @Isotr0py @princepride Routing: @david6666666 via module of the changed files, module named in the PR description; @Isotr0py via module of the changed files, module named in the PR description; @princepride via module of the changed files, module named in the PR description @he-yufeng, 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. |
|
This validates the ownership choice: the manager records the execution device, and |
|
Thanks for running the H200 pass, @Dong1017 — reallocation with the base offloaded is the case the fix is load-bearing for, and your matrix (rank grow with base on CPU, then base-only move back) exercises exactly the recreate path that used to leak buffers back to the host side. Glad the numbers line up. |
The manager only homed the stacked LoRA buffers to the execution device under DLO. Everywhere else the buffers kept whatever device the base weights happened to sit on at creation or rank-growth reinit time, so an adapter registered during an offloaded weight sync was stranded on CPU and apply() matmulled device activations against host buffers after wake (vllm-project#8817). Sleep/offload never sees these sidecars, so the only safe placement is the compute device under every strategy. Layers without the re-homing hook keep the legacy placement. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
f155ab0 to
303f68d
Compare
|
Rebased onto current main (da572cd, which includes the vLLM v0.32.0 alignment). The conflict was additive only: upstream's new Re-verified locally after the rebase: the full |
|
Post-rebase verification on 303f68d (CPU, torch 2.14.0, vLLM v0.32.0 source tree): |
Gaohan123
left a comment
There was a problem hiding this comment.
LGTM. And please submit a PR to branch v0.32.0. Thanks
…llm-project#8874) Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> (cherry picked from commit e900011) Signed-off-by: tzhouam <tzhouam@connect.ust.hk>
Purpose
Fixes #8817.
DiffusionLoRAManageronly homed the stacked LoRA buffers (lora_a_stacked/lora_b_stacked) to the execution device under DLO. Under every other offload strategy the buffers kept whatever device the base weights happened to sit on at creation time, andset_loraonlycopy_s into them. An adapter registered while the diffusion weights are offloaded (the standard RL weight-sync moment, where engines sleep to free memory) therefore stranded the buffers on CPU, and after wakeDiffusionBaseLinearLayerWithLoRA.apply()matmulled device activations against host buffers. The same hole existed on the rank-growth path:_ensure_max_lora_rankre-runscreate_lora_weights, which re-allocated on the offloaded weight device.Sleep/offload never sees these sidecars (the offloader package has no notion of LoRA), so nothing ever re-homes them. The only safe placement is the compute device, under every strategy.
Fix
DiffusionLoRAManagernow always records the execution device as_resident_lora_deviceinstead of gating it toOffloadStrategy.DISTRIBUTED_LAYER_WISE._set_diffusion_lora_buffer_devicewhenever the layer offers the hook. Layers without the hook keep the legacy placement (buffers follow the base weight device), so third-party layer types that predate the hook are unaffected.DiffusionBaseLinearLayerWithLoRA: itscreate_lora_weightsalready re-homes via the recorded device, which now also covers the rank-growth reinit path, pinning the second exposure from the issue.Tests
New
test_lora_manager_homes_lora_buffers_to_compute_device_without_dlointests/diffusion/lora/test_lora_manager.py: a non-DLO pipeline, a recording layer, and a distinct compute device (meta). It asserts the replacement path homes the buffers and records the device thatcreate_lora_weightsconsults on rank growth. It fails on current main and passes with this change. The existing DLO counterpart and the layer-level reallocation test keep passing unchanged.Validation
pytest tests/diffusion/lora/ -o addopts="": 73 passed; the 3hunyuan_image3failures and the 2 collection errors are pre-existing on main (byte-identical failure set with the fix stashed).git stashof the manager change) and passes with it.pre-commit runon the changed files: all hooks pass, including the locally enforced gates (check-spdx-header, mypy-3.10, CI-marks, forbidden imports, torch.cuda ratchet)._replace_layers_with_loraandcreate_lora_weightspaths the engine uses.AI assistance: Kimi Code drafted the patch, test, and PR description. I reviewed the changes and verified the test reproduces the stranded-buffer behavior from #8817.