From 1edc93c3e3dd13489bf28dbdc508cb8473a664d9 Mon Sep 17 00:00:00 2001 From: abinggo <107740309+abinggo@users.noreply.github.com> Date: Fri, 5 Jun 2026 11:27:42 +0800 Subject: [PATCH 1/2] [Bugfix] Require a model for `vllm serve --omni` (fixes #4158) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `vllm serve --omni` with no model silently inherits vLLM's ModelConfig default (Qwen/Qwen3-0.6B). That text LLM is then routed into the diffusion stage, so startup crashes deep in the diffusion worker with a confusing `Model class Qwen3ForCausalLM not found in diffusion model registry` error plus a long cascade of secondary tracebacks — the real cause ("no model was provided") is never surfaced. Fail fast in OmniServeCommand.validate() instead. args.model always carries the default, so explicit provision is detected via the positional model_tag, the --model flag (args.explicit_keys), or per-stage models supplied through --deploy-config / --stage-configs-path. The guard only fires when none of these is present, so the legitimate multi-stage YAML path is unaffected. Adds CPU unit tests: the no-model case raises a clear error, and the positional-model / --model / --stage-configs-path / --deploy-config routes all pass the guard. Signed-off-by: abinggo <107740309+abinggo@users.noreply.github.com> --- tests/entrypoints/test_serve.py | 47 ++++++++++++++++++++++++++++++ vllm_omni/entrypoints/cli/serve.py | 26 +++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/tests/entrypoints/test_serve.py b/tests/entrypoints/test_serve.py index e05983d45b6..09f5e39a8e6 100644 --- a/tests/entrypoints/test_serve.py +++ b/tests/entrypoints/test_serve.py @@ -31,6 +31,53 @@ def test_serve_parser_accepts_no_async_chunk_and_marks_it_explicit() -> None: assert not explicit["async_chunk"] +def _parse_serve_args(argv: list[str]) -> TrackingNamespace: + """Parse a ``serve`` argv through the real Omni parser, returning the + TrackingNamespace (with ``explicit_keys``) that ``validate`` receives.""" + parser = TrackingArgumentParser() + subparsers = parser.add_subparsers(dest="subcommand") + OmniServeCommand().subparser_init(subparsers) + return parser.parse_args(argv) + + +def test_omni_serve_requires_model_when_none_provided() -> None: + """Regression for https://github.com/vllm-project/vllm-omni/issues/4158: + ``vllm serve --omni`` with no model must fail fast instead of silently + falling back to vLLM's default model and crashing in the diffusion worker. + """ + args = _parse_serve_args(["serve", "--omni"]) + cmd = OmniServeCommand() + with pytest.raises(ValueError, match="requires a model"): + cmd.validate(args) + + +@pytest.mark.parametrize( + "argv", + [ + ["serve", "fake-model", "--omni"], # positional model + ["serve", "--omni", "--model", "fake-model"], # --model flag + ["serve", "--omni", "--stage-configs-path", "stages.yaml"], # per-stage YAML + ["serve", "--omni", "--deploy-config", "deploy.yaml"], # deploy YAML + ], +) +def test_omni_serve_accepts_model_or_stage_yaml(argv: list[str], mocker: MockerFixture) -> None: + """A model supplied positionally, via ``--model``, or per-stage through + ``--stage-configs-path`` / ``--deploy-config`` must NOT trip the + require-a-model guard (avoid false positives on the legitimate multi-stage + YAML path). Downstream validation is mocked so only the new guard is + exercised.""" + mocker.patch( + "vllm_omni.diffusion.utils.hf_utils.is_diffusion_model", + return_value=False, + ) + mocker.patch("vllm_omni.entrypoints.cli.serve.validate_parsed_serve_args") + + args = _parse_serve_args(argv) + cmd = OmniServeCommand() + # Must not raise the "requires a model" error for any of these. + cmd.validate(args) + + def _make_headless_args(**kwargs) -> TrackingNamespace: defaults = { "model": "fake-model", diff --git a/vllm_omni/entrypoints/cli/serve.py b/vllm_omni/entrypoints/cli/serve.py index 723e1f2360b..151f839b325 100644 --- a/vllm_omni/entrypoints/cli/serve.py +++ b/vllm_omni/entrypoints/cli/serve.py @@ -113,6 +113,32 @@ def validate(self, args: argparse.Namespace) -> None: if args.stage_id is not None and (args.omni_master_address is None or args.omni_master_port is None): raise ValueError("--stage-id requires both --omni-master-address and --omni-master-port to be set") + # Require a model under --omni. ``args.model`` always carries vLLM's + # ModelConfig default (``Qwen/Qwen3-0.6B``), so an omit is silent: the + # default text LLM is routed into the diffusion stage and startup + # crashes deep in the diffusion worker with a confusing registry error. + # Fail fast instead. A model can be supplied positionally + # (``vllm serve --omni``), via ``--model``, or per-stage through + # ``--deploy-config`` / ``--stage-configs-path``; only error when none + # of these is present. See + # https://github.com/vllm-project/vllm-omni/issues/4158. + if getattr(args, "omni", False): + explicit_keys = getattr(args, "explicit_keys", None) or frozenset() + model_provided = getattr(args, "model_tag", None) is not None or "model" in explicit_keys + stage_yaml_provided = ( + getattr(args, "stage_configs_path", None) is not None + or getattr(args, "deploy_config", None) is not None + ) + if not model_provided and not stage_yaml_provided: + raise ValueError( + "`vllm serve --omni` requires a model. Pass it positionally " + "(`vllm serve --omni`), via `--model`, or supply " + "per-stage models through `--deploy-config` / " + "`--stage-configs-path`. Without one, vLLM's default model " + "(Qwen/Qwen3-0.6B) is selected and routed into the diffusion " + "stage, which fails with a confusing diffusion-registry error." + ) + # --omni-replica-address is only consulted in run_headless(); reject it # on the head so a misconfigured launch fails loudly instead of being # silently ignored. From 045ea71421a41605bcd9d566fced57592f63aeb3 Mon Sep 17 00:00:00 2001 From: abinggo <107740309+abinggo@users.noreply.github.com> Date: Sat, 1 Aug 2026 19:26:41 +0800 Subject: [PATCH 2/2] Fix --omni model guard: deploy/stage YAML is not a model source Address review feedback (#4167): the guard treated the presence of --deploy-config / --stage-configs-path as proof a model was provided, but those YAMLs carry per-stage engine args only -- the checkpoint is always threaded in from args.model (load_and_resolve_stage_configs takes model as its first arg; no deploy/stage YAML in-tree defines a model). So `vllm serve --omni --deploy-config ` bypassed the guard while args.model stayed at vLLM's default and reproduced the same diffusion-registry crash. Require an explicit, non-empty model from the CLI (positional or --model). An empty/whitespace value (e.g. --model "$MODEL" with the var unset) is also rejected so it fails fast instead of crashing downstream. Add negative tests for deploy-config/stage-configs-path without a model and for empty/whitespace model values; positive tests pair a model with the YAML. Signed-off-by: abinggo <107740309+abinggo@users.noreply.github.com> --- tests/entrypoints/test_serve.py | 48 ++++++++++++++++++++----- vllm_omni/entrypoints/cli/serve.py | 56 ++++++++++++++++++++---------- 2 files changed, 76 insertions(+), 28 deletions(-) diff --git a/tests/entrypoints/test_serve.py b/tests/entrypoints/test_serve.py index 09f5e39a8e6..a4da832efdb 100644 --- a/tests/entrypoints/test_serve.py +++ b/tests/entrypoints/test_serve.py @@ -47,7 +47,37 @@ def test_omni_serve_requires_model_when_none_provided() -> None: """ args = _parse_serve_args(["serve", "--omni"]) cmd = OmniServeCommand() - with pytest.raises(ValueError, match="requires a model"): + with pytest.raises(ValueError, match="requires an explicit model"): + cmd.validate(args) + + +@pytest.mark.parametrize( + "argv", + [ + # A deploy/stage YAML supplied *without* an explicit model must still + # trip the guard. These YAMLs carry per-stage engine args only -- the + # checkpoint is always threaded in from ``args.model`` -- so their mere + # presence does not establish that a model was provided. Without an + # explicit model ``args.model`` stays at vLLM's default and the launch + # reproduces the issue-4158 diffusion-registry crash. + ["serve", "--omni", "--deploy-config", "deploy.yaml"], + ["serve", "--omni", "--stage-configs-path", "stages.yaml"], + # An empty/whitespace model must not satisfy the guard either -- this is + # the ``--model "$MODEL"``-with-unset-var footgun. Otherwise the empty + # value flows downstream and crashes with the same confusing error. + ["serve", "", "--omni"], # empty positional + ["serve", "--omni", "--model", ""], # empty --model + ["serve", "--omni", "--model", " "], # whitespace-only --model + ], +) +def test_omni_serve_rejects_missing_or_empty_model(argv: list[str]) -> None: + """Regression for review feedback on PR #4167: a deploy/stage YAML is not a + model source, so ``--deploy-config`` / ``--stage-configs-path`` alone must + not bypass the require-a-model guard; neither may an empty/whitespace + model value.""" + args = _parse_serve_args(argv) + cmd = OmniServeCommand() + with pytest.raises(ValueError, match="requires an explicit model"): cmd.validate(args) @@ -56,16 +86,16 @@ def test_omni_serve_requires_model_when_none_provided() -> None: [ ["serve", "fake-model", "--omni"], # positional model ["serve", "--omni", "--model", "fake-model"], # --model flag - ["serve", "--omni", "--stage-configs-path", "stages.yaml"], # per-stage YAML - ["serve", "--omni", "--deploy-config", "deploy.yaml"], # deploy YAML + # An explicit model *with* a deploy/stage YAML is the legitimate + # multi-stage path and must pass the guard. + ["serve", "fake-model", "--omni", "--deploy-config", "deploy.yaml"], + ["serve", "fake-model", "--omni", "--stage-configs-path", "stages.yaml"], ], ) -def test_omni_serve_accepts_model_or_stage_yaml(argv: list[str], mocker: MockerFixture) -> None: - """A model supplied positionally, via ``--model``, or per-stage through - ``--stage-configs-path`` / ``--deploy-config`` must NOT trip the - require-a-model guard (avoid false positives on the legitimate multi-stage - YAML path). Downstream validation is mocked so only the new guard is - exercised.""" +def test_omni_serve_accepts_explicit_model(argv: list[str], mocker: MockerFixture) -> None: + """A model supplied positionally or via ``--model`` -- with or without a + deploy/stage YAML -- must NOT trip the require-a-model guard. Downstream + validation is mocked so only the new guard is exercised.""" mocker.patch( "vllm_omni.diffusion.utils.hf_utils.is_diffusion_model", return_value=False, diff --git a/vllm_omni/entrypoints/cli/serve.py b/vllm_omni/entrypoints/cli/serve.py index 151f839b325..cd48141a192 100644 --- a/vllm_omni/entrypoints/cli/serve.py +++ b/vllm_omni/entrypoints/cli/serve.py @@ -113,30 +113,48 @@ def validate(self, args: argparse.Namespace) -> None: if args.stage_id is not None and (args.omni_master_address is None or args.omni_master_port is None): raise ValueError("--stage-id requires both --omni-master-address and --omni-master-port to be set") - # Require a model under --omni. ``args.model`` always carries vLLM's - # ModelConfig default (``Qwen/Qwen3-0.6B``), so an omit is silent: the - # default text LLM is routed into the diffusion stage and startup + # Require an explicit model under --omni. ``args.model`` always carries + # vLLM's ModelConfig default (``Qwen/Qwen3-0.6B``), so an omit is silent: + # the default text LLM is routed into the diffusion stage and startup # crashes deep in the diffusion worker with a confusing registry error. - # Fail fast instead. A model can be supplied positionally - # (``vllm serve --omni``), via ``--model``, or per-stage through - # ``--deploy-config`` / ``--stage-configs-path``; only error when none - # of these is present. See + # Fail fast instead. + # + # The model must come from the CLI -- positionally + # (``vllm serve --omni``) or via ``--model``. Deploy/stage YAMLs + # (``--deploy-config`` / ``--stage-configs-path``) are NOT a model + # source: they carry per-stage engine args only, and the checkpoint is + # always threaded in from ``args.model`` (see + # ``load_and_resolve_stage_configs``, which takes ``model`` as its first + # argument). Treating their mere presence as "model provided" let + # ``vllm serve --omni --deploy-config `` slip through with the + # default model and reproduce the very crash this guard prevents. See # https://github.com/vllm-project/vllm-omni/issues/4158. if getattr(args, "omni", False): explicit_keys = getattr(args, "explicit_keys", None) or frozenset() - model_provided = getattr(args, "model_tag", None) is not None or "model" in explicit_keys - stage_yaml_provided = ( - getattr(args, "stage_configs_path", None) is not None - or getattr(args, "deploy_config", None) is not None - ) - if not model_provided and not stage_yaml_provided: + # Resolve the model the user actually supplied on the CLI. A + # positional ``model_tag`` takes precedence (``cmd`` later copies it + # onto ``args.model``); otherwise ``--model`` counts only when it was + # explicitly passed. An empty/whitespace value (e.g. ``--model + # "$MODEL"`` with ``MODEL`` unset) is treated as "not provided" so it + # fails here with a clear message instead of the same confusing + # downstream crash. + model_tag = getattr(args, "model_tag", None) + if model_tag is not None: + explicit_model = model_tag + elif "model" in explicit_keys: + explicit_model = getattr(args, "model", None) + else: + explicit_model = None + model_provided = explicit_model is not None and str(explicit_model).strip() != "" + if not model_provided: raise ValueError( - "`vllm serve --omni` requires a model. Pass it positionally " - "(`vllm serve --omni`), via `--model`, or supply " - "per-stage models through `--deploy-config` / " - "`--stage-configs-path`. Without one, vLLM's default model " - "(Qwen/Qwen3-0.6B) is selected and routed into the diffusion " - "stage, which fails with a confusing diffusion-registry error." + "`vllm serve --omni` requires an explicit model. Pass it " + "positionally (`vllm serve --omni`) or via `--model`. " + "`--deploy-config` / `--stage-configs-path` carry per-stage " + "engine args only and do not supply a model; without an " + "explicit model, vLLM's default (Qwen/Qwen3-0.6B) is selected " + "and routed into the diffusion stage, which fails with a " + "confusing diffusion-registry error." ) # --omni-replica-address is only consulted in run_headless(); reject it