Skip to content

[Bugfix] Fix dropped diffusion executor backend in default config #2539#2544

Open
reidliu41 wants to merge 2 commits intovllm-project:mainfrom
reidliu41:fix/issue-2539-diffusion-executor-backend
Open

[Bugfix] Fix dropped diffusion executor backend in default config #2539#2544
reidliu41 wants to merge 2 commits intovllm-project:mainfrom
reidliu41:fix/issue-2539-diffusion-executor-backend

Conversation

@reidliu41
Copy link
Copy Markdown
Contributor

PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED.

Purpose

Fix #2539.

The default diffusion stage config builder dropped
distributed_executor_backend, so
vllm serve --omni --distributed-executor-backend ...
was not propagated to diffusion models that use the default fallback stage
config path.

This change preserves the explicitly provided backend in
AsyncOmniEngine._create_default_diffusion_stage_cfg, matching the existing
explicit propagation pattern used for dtype.

Value:

  • fixes incorrect executor selection on the default diffusion config path
  • makes executor-specific diffusion changes testable again
  • adds regression coverage to prevent silent fallback to the default backend

Test Plan

./.venv/bin/python -m pytest tests/entrypoints/test_async_omni_diffusion_config.py tests/entrypoints/test_utils.py

Test Result

$ ./.venv/bin/python -m pytest tests/entrypoints/test_async_omni_diffusion_config.py tests/entrypoints/test_utils.py
================================================= test session starts ==================================================
platform linux -- Python 3.12.9, pytest-9.0.2, pluggy-1.6.0
rootdir: /home/reid/github/vllm-omni
configfile: pyproject.toml
plugins: mock-3.15.1, asyncio-1.3.0, cov-7.1.0, anyio-4.13.0
asyncio: mode=Mode.AUTO, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 26 items

tests/entrypoints/test_async_omni_diffusion_config.py ......                                                     [ 23%]
tests/entrypoints/test_utils.py ....................                                                             [100%]


=========================================== 26 passed, 18 warnings in 2.39s ============================================

Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan. Please provide the test scripts & test commands. Please state the reasons if your codes don't require additional test scripts. For test file guidelines, please check the test style doc
  • The test results. Please paste the results comparison before and after, or the e2e results.
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model. Please run mkdocs serve to sync the documentation editions to ./docs.
  • (Optional) Release notes update. If your change is user-facing, please update the release notes draft.

BEFORE SUBMITTING, PLEASE READ https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md (anything written below this line will be removed by GitHub Actions)

  - preserve distributed_executor_backend in default diffusion stage config
  - add regression tests for default stage creation and resolution

Signed-off-by: reidliu41 <reid201711@gmail.com>
@reidliu41 reidliu41 requested a review from hsliuustc0106 as a code owner April 7, 2026 08:10
@chatgpt-codex-connector
Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@xiaohajiayou
Copy link
Copy Markdown
Contributor

It looks like the issue might not be limited to this parameter — a few other engine args are also affected in the fallback path. I’ve tried to fix them in #2559.

Copy link
Copy Markdown
Collaborator

@lishunyang12 lishunyang12 left a comment

Choose a reason for hiding this comment

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

Looks good. The fix is minimal and correctly follows the existing pattern established by dtype propagation.

A few observations:

  1. Production code: The None guard (and normalized_kwargs["distributed_executor_backend"] is not None) is the right call -- without it, an explicit None would override downstream defaults. Consistent with how other optional fields are handled.

  2. Tests: Both the unit test on _create_default_diffusion_stage_cfg and the integration test through load_and_resolve_stage_configs provide good regression coverage.

  3. Minor note for future consideration (not blocking): This method is accumulating a growing list of one-off conditional propagations (dtype, now distributed_executor_backend). If more kwargs need the same treatment, it might be worth a small helper or a whitelist-driven loop. But that is outside the scope of this bugfix.

LGTM -- approve.

Signed-off-by: reidliu41 <reid201711@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: --distributed-executor-backend option is ignored for diffusion models

3 participants