Repository navigation
[Bugfix][Diffusion] Fuse distilled LoRA weights before HSDP sharding - #6948
Conversation
Fuse distilled LoRA weights into base model weights during pipeline loading before HSDP / FSDP2 sharding into DTensors occurs, preventing DTensor/Tensor mixing errors on GPU and glibc heap corruption crashes on Ascend NPU (vllm-project#6944). Fixes vllm-project#6944 Signed-off-by: samithuang <285365963@qq.com>
|
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/diffusion_model_integration.md. Module owners: @fhfuih @Bounty-hunter @wtomin @RuixiangMa @SamitHuang, 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. |
| sources=ordinary_sources, | ||
| planned_weights=host_weight_plan.bindings, | ||
| ) | ||
| self._maybe_fuse_distilled_lora(model) |
There was a problem hiding this comment.
[P2] On an HWR warm final-layout hit this runs on already-final tensors. The cold path publishes the snapshot after fusion (line 683), so a warm restore brings back fused weights — and lora_is_fused is False on the fresh pipeline object because the flag is instance state the store doesn't record. If a warm hit can co-occur with --lora-backend distill (I found no exclusion), the deltas are applied a second time — silently wrong weights, and under HSDP this is add_ on sharded DTensors, i.e. the exact #6944 crash on the warm path. Suggest recording lora_is_fused in the HWR state (or skipping fusion when warm_snapshot is present) so warm and cold agree.
There was a problem hiding this comment.
Updated _maybe_fuse_distilled_lora to check self._hwr_state.get("warm_snapshot"), marking model.lora_is_fused = True and skipping re-fusion on warm HWR restore. Added unit tests in test_diffusers_loader.py covering both warm-restore skip and cold-path fusion.
There was a problem hiding this comment.
Verified in 967a66c: cold loads can't see a warm snapshot (state resets per load_model, and warm_snapshot is only written on a LOCAL_HIT restore, after prior-load fusion), so the guard is exact. Worth noting the scenario was already unreachable in production since lora_path makes HWR ineligible (host_weight_loader.py:134), but making the invariant local and pinning it with the two new tests is the right call.
|
|
||
| @property | ||
| def lora_is_fused(self) -> bool: | ||
| return getattr(self, "_lora_is_fused", False) or bool(getattr(self, "_lora_loaded", None)) |
There was a problem hiding this comment.
[P3] Two small things: (1) this getter conflates loaded with fused — true today only because every load through these mixins fuses in-place (add_/sub_), which a reader can't know; a one-line docstring ("mixin loads are in-place fusions, so _lora_loaded non-empty ⇒ fused") would keep a future adapter-style load path from silently disabling the dynamic manager. Also _lora_is_fused: bool = False is assigned, breaking the "annotated, not assigned" convention on the lines above. (2) In _maybe_fuse_distilled_lora, the backend triple-check is redundant — LoRABackend(str, Enum) makes == LoRABackend.DISTILL or == "distill" sufficient; the .value fallback is dead.
There was a problem hiding this comment.
Thanks! Updated LoraLoaderMixin to only annotate _lora_is_fused: bool, added the clarifying docstring to lora_is_fused, and simplified the backend check in _maybe_fuse_distilled_lora.
There was a problem hiding this comment.
Fixed — bare annotation plus the docstring reads well, and no bare _lora_is_fused access exists, so no behavior change. Thanks.
- Handle warm HWR restores by setting `lora_is_fused` and skipping re-fusion - Clean up `_lora_is_fused` type annotation in `LoraLoaderMixin` and add docstring - Simplify backend comparison in `_maybe_fuse_distilled_lora` - Add unit tests for warm HWR restore behavior with distilled LoRA Signed-off-by: samithuang <285365963@qq.com>
|
Update:
|
|
@hsliuustc0106 @yenuo26 @bjf-frz PTAL, thx |
- Set `--hsdp-shard-size` default to -1 to auto-calculate from world size - Forward `use_hsdp`, `hsdp_shard_size`, and `hsdp_replicate_size` to Omni engine kwargs Signed-off-by: samithuang <285365963@qq.com>
hsliuustc0106
left a comment
There was a problem hiding this comment.
LGTM — both threads resolved, thanks.
|
Please resolve CI conflicts |
… video output metadata Signed-off-by: samithuang <285365963@qq.com>
Signed-off-by: Samit <285365963@qq.com>
…llm-project#6948) Signed-off-by: samithuang <285365963@qq.com> Signed-off-by: Samit <285365963@qq.com> Signed-off-by: ZhengWG <zwg0606@gmail.com>
…llm-project#6948) Signed-off-by: samithuang <285365963@qq.com> Signed-off-by: Samit <285365963@qq.com>
Fixes #6944
Purpose
When serving diffusion models (such as
Wan2.2-T2V-A14B-Diffusers) with--lora-backend distilland--use-hsdp, the service failed during LoRA weight fusion:RuntimeError: aten.add_.Tensor got mixed torch.Tensor and DTensor, need to convert all torch.Tensor to DTensor before calling distributed operators!.corrupted size vs. prev_size.Root Cause
Distilled LoRA fusion was previously performed in
DiffusionWorker.init_lora_manager()afterload_model()had completed. Under HSDP,_load_model_with_hsdpalready transformed the transformer block parameters into shardedDTensors. Whenload_lora_into_moduleexecutedparams.add_(delta)using regular un-shardedtorch.Tensordeltas, in-place addition on DTensors failed or corrupted sharded memory.Solution
_maybe_fuse_distilled_lorainDiffusersPipelineLoaderto fuse distilled LoRA weights immediately after base weights are loaded and before HSDP / FSDP2 sharding (apply_hsdp_to_model) or post-load quantization runs.lora_is_fusedproperty toLoraLoaderMixinso pipelines track fusion state, and updatedDiffusionWorker.init_lora_manager()to skip re-fusing if already fused at load time.lora_is_fusedtracking, and worker lifecycle.Test Plan
Added unit tests in:
tests/diffusion/model_loader/test_diffusers_loader.pytest_hsdp_fuses_distilled_lora_before_shardingtest_hsdp_fuses_multi_file_distilled_lora_wan22test_load_model_fuses_distilled_lora_non_hsdptests/diffusion/lora/test_loader.pytest_lora_loader_mixin_lora_is_fused_propertytests/diffusion/test_diffusion_worker.pyTestDiffusionWorkerInitLoraManagervLLM Version: 0.28.0
vLLM-Omni Commit: 629d82e
Test Result