Skip to content

[Bugfix][Qwen-Image] Read resolved offload strategy - #8197

Closed
Ai-Eastern wants to merge 1 commit into
vllm-project:mainfrom
Ai-Eastern:fix/qwen-image-resolved-offload-8194
Closed

Ai-Eastern wants to merge 1 commit into
vllm-project:mainfrom
Ai-Eastern:fix/qwen-image-resolved-offload-8194

Conversation

@Ai-Eastern

@Ai-Eastern Ai-Eastern commented Sep 27, 2026 •

Copy link
Copy Markdown

Purpose

Fixes #8194. The Qwen-Image pipeline still read enable_cpu_offload directly after the offload policy migration in #7327. This triggers the legacy-reader CI assertion and makes initial text-encoder/VAE placement depend on a compatibility alias. Read the resolved MODEL_LEVEL strategy instead; keep layerwise placement and the loader's DiT device choice intact.

Reproduction and root cause

On the issue's base commit, run:

pytest -q tests/diffusion/offloader/test_legacy_flag_readers.py

The assertion reports diffusion/models/qwen_image/pipeline_qwen_image.py:344. The remaining legacy read is in QwenImagePipeline.__init__, where it chooses the initial text-encoder/VAE device. A new CPU-marked mocked pipeline test covers compact mode="module" and mode="layer" configs independently of the old flag.

Test Plan

pytest -q tests/diffusion/offloader/test_legacy_flag_readers.py tests/diffusion/models/qwen_image/test_qwen_image_pipeline_device.py
pytest -q tests/diffusion/offloader/test_legacy_flag_readers.py tests/diffusion/models/qwen_image/test_qwen_image_pipeline_device.py -m "core_model and cpu" --run-level=core_model

Prerequisites: a supported Python environment with the repository's development dependencies. These tests use CPU and mocked model loading; no model weights are needed.

vLLM Version: 0.30.0 in the issue's CI report; local tests used the 0.30.0+cpu wheel in a Linux container.

vLLM-Omni Commit: 7ab582ae2545476284bef8903bec1c0ddc44a082 (PR base).

Test Result

  • Passed: the two repository pytest modules in a local Linux CPU container (python:3.12-slim-trixie, vLLM 0.30.0+cpu, PyTorch 2.13.0+cpu): 5 passed with the plain command and 5 passed with -m "core_model and cpu" --run-level=core_model. These are CPU tests with mocked model loading; no model weights or GPU inference were used.
  • Passed: a local stdlib-only probe exercised the production strategy resolver and placement expressions for legacy model offload, compact module offload, and compact layer offload. It also scanned runtime files using the existing legacy-reader pattern with normalized paths. This probe is not part of the PR.
  • Passed: the original test_no_runtime_module_reads_the_legacy_offload_flags test body against this branch's source in an isolated Windows pytest runner (1 passed). The runner used a lightweight vllm_omni package stub, UTF-8 mode, and a Windows-path adaptation for the test's POSIX-style compatibility allowlist; this is narrower than a run through the repository test environment.
  • Passed: Python 3.12 compileall on both changed files; uvx --python 3.12 ruff check and ruff format --check on both changed files; git diff --check.
  • Passed: repository hooks check-mark, check-spdx-header, check-forbidden-imports, and check-torch-cuda-call on the changed files.
  • Not run: GPU/model inference and full local pre-commit. GitHub build, DCO, documentation, and pre-commit checks passed; maintainer review is pending.

AI assistance: Codex drafted the source change, regression test, local verification probe, and this PR description. The human author reviewed the diff and remains responsible for the final contribution and review responses.

Signed-off-by: Ai-Eastern <a897889110@gmail.com>
@Ai-Eastern
Ai-Eastern marked this pull request as ready for review September 27, 2026 03:14
@vllm-omni-review-bot

Copy link
Copy Markdown

This PR appears to belong to: docs/design/module/diffusion/offloader.md, docs/design/module/diffusion/diffusion_model_integration.md, docs/design/module/diffusion/index.md.

Module owners: @wtomin @david6666666 @Isotr0py

Routing: @wtomin via module of the changed files, module named in the PR description, CODEOWNERS; @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

@Ai-Eastern, 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.

@vllm-omni-review-bot

Copy link
Copy Markdown

Omni ReviewBot triage note

Automated triage of commit 417cccb58a53 produced:

  • Priority: high. Prompt maintainer attention is suggested.

These are automated triage suggestions only — the final decision belongs to the maintainers.

@Ai-Eastern

Ai-Eastern commented Sep 27, 2026 •

Copy link
Copy Markdown
Author

Self-review: I reviewed the Qwen-Image offload change and its regression test. Encoder/VAE placement follows the resolved strategy, and DiT keeps the loader-selected device. The compact module and layer cases are covered. Both targeted pytest commands passed locally in a Linux CPU container: 5 passed each. GPU/model inference was not run.

@vllm-omni-review-bot

Copy link
Copy Markdown
Omni ReviewBot routing record

Assigned Direct under experiment vllm-omni-strict-5050-20260829.

@vllm-omni-review-bot vllm-omni-review-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omni ReviewBot review

PR description

Qwen-Image still chose the initial text-encoder and VAE device from the legacy enable_cpu_offload alias after the offload-policy migration. The pipeline now asks the shared strategy resolver and parks those modules on CPU only when the resolved strategy is model-level. Layerwise and other non-model-level paths still place encoder/VAE on the pipeline device, and the DiT still follows the loader device context. A mocked CPU test adds compact mode="module" and mode="layer" cases so placement is no longer tied to the old flag.

Change flow

flowchart TD
    A["[EXISTING] od_config offload fields"]:::existing --> B["[EXISTING] resolve_offload_strategy"]:::existing
    B --> C["[CHANGED] QwenImagePipeline encoder/VAE placement"]:::changed
    C --> D["[EXISTING] MODEL_LEVEL parks encoder/VAE on CPU"]:::existing
    C --> E["[EXISTING] DiT stays on loader device"]:::existing
    C --> F["[NEW] compact module vs layer device test"]:::new
    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
Loading

CI at 417cccb58a53 (2026-09-27T20:30:14.040539+00:00): required check(s) blocking: buildkite/vllm-omni (missing), and -5 more.

No actionable findings.

@hsliuustc0106 hsliuustc0106 added the bug Something isn't working label Sep 28, 2026
@Ai-Eastern

Copy link
Copy Markdown
Author

Both targeted CPU pytest commands passed locally (5 passed each; details are in the PR description). I don't have access to an H100, so I couldn't run the Qwen-Image L2 job locally. Could a maintainer advise how to proceed with the ready label and Buildkite/H100 validation?

@NumberWan

Copy link
Copy Markdown
Contributor

LGTM

@NickCao

NickCao commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

Already done in #7695

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

5 participants