[Config Refactor] sentinel default precedence (#3078)

Signed-off-by: lishunyang <lishunyang12@163.com>
Signed-off-by: xiaohajiayou <923390377@qq.com>
Co-authored-by: lishunyang <lishunyang12@163.com>

tests/engine/test_single_stage_mode.py

@@ -422,6 +422,39 @@ class TestSingleStageModeDetection:
         )
         assert engine._single_stage_id_filter is None
 
+    def test_engine_args_create_only_forwards_explicit_fields(self, mocker: MockerFixture):
+        from vllm_omni.engine.arg_utils import OmniEngineArgs
+
+        captured: dict[str, Any] = {}
+
+        def fake_resolve(self, model: str, kwargs: dict[str, Any]):
+            captured.update(kwargs)
+            return "/fake/path", [_make_stage_cfg(0)]
+
+        mocker.patch.object(AsyncOmniEngine, "_resolve_stage_configs", fake_resolve)
+        mocker.patch.object(AsyncOmniEngine, "_bootstrap_orchestrator")
+        mock_thread_cls = mocker.patch("threading.Thread")
+        mock_future_cls = mocker.patch("concurrent.futures.Future")
+        mock_future = mocker.Mock()
+        mock_future.result.return_value = mocker.Mock()
+        mock_future_cls.return_value = mock_future
+        mock_thread = mocker.Mock()
+        mock_thread.is_alive.return_value = False
+        mock_thread_cls.return_value = mock_thread
+
+        ea = OmniEngineArgs.create(model="ignored", gpu_memory_utilization=0.5)
+        AsyncOmniEngine(model="fake-model", engine_args=ea)
+
+        assert captured["gpu_memory_utilization"] == 0.5
+        assert "model" not in captured
+        assert "max_num_seqs" not in captured
+
+    def test_bare_engine_args_rejected(self, mocker: MockerFixture):
+        from vllm_omni.engine.arg_utils import OmniEngineArgs
+
+        with pytest.raises(TypeError, match="OmniEngineArgs.create"):
+            self._make_engine_no_thread(mocker, engine_args=OmniEngineArgs(model="fake-model"))
+
     def test_master_address_and_port_stored(self, mocker: MockerFixture):
         engine = self._make_engine_no_thread(
             mocker,

tests/entrypoints/test_omni_entrypoints.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import argparse
 import queue
 from collections.abc import Callable
 from types import SimpleNamespace
@@ -166,6 +167,31 @@ def _patch_engine(monkeypatch: pytest.MonkeyPatch, engine: FakeAsyncOmniEngine)
     monkeypatch.setattr("vllm_omni.entrypoints.omni_base.omni_snapshot_download", lambda model: model)
 
 
+def test_from_cli_args_only_nulls_untyped_override_fields(monkeypatch: pytest.MonkeyPatch):
+    from vllm_omni.entrypoints.omni import Omni
+
+    captured: dict[str, Any] = {}
+
+    def fake_engine(*args: Any, **kwargs: Any) -> FakeAsyncOmniEngine:
+        captured.update(kwargs)
+        return FakeAsyncOmniEngine()
+
+    monkeypatch.setattr("vllm_omni.entrypoints.omni_base.AsyncOmniEngine", fake_engine)
+    monkeypatch.setattr("vllm_omni.entrypoints.omni_base.omni_snapshot_download", lambda model: model)
+    monkeypatch.setattr("sys.argv", ["prog"])
+
+    parser = argparse.ArgumentParser()
+    parser.add_argument("--gpu-memory-utilization", type=float, default=0.9)
+    parser.add_argument("--hsdp-shard-size", type=int, default=-1)
+    args = parser.parse_args([])
+    args.model = "fake-model"
+
+    Omni.from_cli_args(args, parser=parser)
+
+    assert captured["gpu_memory_utilization"] is None
+    assert captured["hsdp_shard_size"] == -1
+
+
 def _make_base():
     from vllm_omni.entrypoints.omni_base import OmniBase
 

tests/test_arg_utils.py

@@ -351,3 +351,138 @@ def test_ambiguous_field_non_strict_routes_to_orchestrator(caplog):
     assert orch.deploy_config == "x"
     assert "deploy_config" not in engine
     assert any("both OrchestratorArgs" in r.message for r in caplog.records)
+
+
+# Sentinel-default precedence invariants (#3035)
+
+
+def _build_full_serve_parser():
+    from vllm.utils.argparse_utils import FlexibleArgumentParser
+
+    try:
+        from vllm.entrypoints.openai.cli_args import make_arg_parser
+    except ImportError:
+        pytest.skip("vllm parser not importable")
+    return make_arg_parser(FlexibleArgumentParser())
+
+
+def test_nullify_stage_engine_defaults_resets_inherited_defaults():
+    import argparse
+
+    from vllm_omni.engine.arg_utils import (
+        deploy_override_field_names,
+        nullify_stage_engine_defaults,
+    )
+
+    parser = _build_full_serve_parser()
+    nullify_stage_engine_defaults(parser)
+
+    override_dests = deploy_override_field_names()
+    offenders = [
+        (a.dest, a.default)
+        for a in parser._actions
+        if a.dest not in ("help", "version")
+        and a.option_strings
+        and a.dest in override_dests
+        and a.default is not None
+        and a.default is not argparse.SUPPRESS
+    ]
+    assert not offenders, f"Stage flags with non-None defaults after nullify: {offenders}"
+
+
+def test_non_override_flags_keep_real_defaults_after_nullify():
+    import argparse
+
+    from vllm_omni.engine.arg_utils import nullify_stage_engine_defaults
+
+    parser = argparse.ArgumentParser()
+    parser.add_argument("--hsdp-shard-size", type=int, default=-1, help="HSDP shard size.")
+    parser.add_argument("--max-num-seqs", type=int, default=64, help="Max num seqs.")
+    nullify_stage_engine_defaults(parser)
+
+    hsdp = next(a for a in parser._actions if a.dest == "hsdp_shard_size")
+    max_num_seqs = next(a for a in parser._actions if a.dest == "max_num_seqs")
+    assert hsdp.default == -1
+    assert max_num_seqs.default is None
+
+
+def test_help_text_preserves_default_after_nullify():
+    # Real defaults must stay visible in --help even though parser stores None.
+    import argparse
+
+    from vllm_omni.engine.arg_utils import nullify_stage_engine_defaults
+
+    parser = argparse.ArgumentParser()
+    parser.add_argument("--max-num-seqs", type=int, default=42, help="Example knob.")
+    nullify_stage_engine_defaults(parser)
+
+    action = next(a for a in parser._actions if a.dest == "max_num_seqs")
+    assert action.default is None
+    assert "(default: 42)" in action.help
+
+
+_OMNIENGINEARGS_USER_INPUT_FIELDS = frozenset(
+    {
+        "model_stage",
+        "model_arch",
+        "engine_output_type",
+        "hf_config_name",
+        "custom_process_next_stage_input_func",
+        "subtalker_sampling_params",
+        "async_chunk",
+        "omni_kv_config",
+        "quantization_config",
+        "worker_type",
+        "task_type",
+        "worker_cls",
+        "enable_sleep_mode",
+        "omni_master_address",
+        "omni_master_port",
+        "stage_configs_path",
+        "output_modalities",
+        "log_stats",
+        "custom_pipeline_args",
+    }
+)
+
+
+def test_omniengineargs_user_input_fields_default_to_none():
+    try:
+        from vllm_omni.engine.arg_utils import OmniEngineArgs
+    except Exception as exc:
+        pytest.skip(f"OmniEngineArgs not importable: {exc}")
+
+    offenders = [
+        (f.name, f.default)
+        for f in fields(OmniEngineArgs)
+        if f.name in _OMNIENGINEARGS_USER_INPUT_FIELDS
+        and f.default is not dataclasses.MISSING
+        and f.default is not None
+    ]
+    assert not offenders, f"User-input fields with non-None defaults: {offenders}"
+
+
+def test_omniengineargs_create_tracks_explicit_fields():
+    try:
+        from vllm_omni.engine.arg_utils import OmniEngineArgs
+    except Exception as exc:
+        pytest.skip(f"OmniEngineArgs not importable: {exc}")
+
+    ea = OmniEngineArgs.create(model="x", gpu_memory_utilization=0.5)
+    assert ea._explicit_fields == frozenset({"model", "gpu_memory_utilization"})
+    assert ea.explicit_kwargs() == {"model": "x", "gpu_memory_utilization": 0.5}
+
+
+def test_omniengineargs_bare_constructor_has_no_explicit_tracking():
+    try:
+        from vllm_omni.engine.arg_utils import OmniEngineArgs
+    except Exception as exc:
+        pytest.skip(f"OmniEngineArgs not importable: {exc}")
+
+    ea = OmniEngineArgs(model="x")
+    assert not hasattr(ea, "_explicit_fields")
+    assert "model" in ea.explicit_kwargs()
+
+
+# dataclasses already imported via ``from dataclasses import dataclass, fields``
+import dataclasses  # noqa: E402  -- needed for MISSING sentinel above

tests/test_config_factory.py

@@ -4,6 +4,7 @@
 Unit tests for StageConfigFactory and related classes.
 """
 
+import warnings
 from dataclasses import dataclass
 from pathlib import Path
 
@@ -1196,99 +1197,89 @@ class TestCLIOverrideFlow:
             assert s.runtime_overrides["enforce_eager"] is True
 
 
-class TestCLIExplicitPrecedence:
-    """Verify YAML > argparse defaults; explicit CLI args > YAML."""
+class TestSentinelDefaultPrecedence:
+    """Caller-typed (non-None) values win over YAML; None values fall through
+    to YAML / dataclass defaults (#3035)."""
 
-    def _stages(self, cli_overrides, cli_explicit_keys):
+    def _stages(self, cli_overrides):
         import vllm_omni.model_executor.models.qwen3_omni.pipeline  # noqa: F401
 
         return StageConfigFactory._create_from_registry(
             "qwen3_omni_moe",
             cli_overrides=cli_overrides,
-            cli_explicit_keys=cli_explicit_keys,
         )
 
-    def test_explicit_cli_overrides_yaml(self):
-        """User-typed --max-num-seqs wins over the deploy YAML value."""
-        stages = self._stages(
-            cli_overrides={"max_num_seqs": 999},
-            cli_explicit_keys={"max_num_seqs"},
-        )
-        # Stage 2 yaml has max_num_seqs=1; explicit CLI must beat it.
+    def test_typed_kwarg_overrides_yaml(self):
+        stages = self._stages({"max_num_seqs": 999})
         assert stages[2].runtime_overrides.get("max_num_seqs") == 999
 
-    def test_default_cli_does_not_override_yaml(self):
-        """Argparse defaults must NOT clobber values that are present in YAML."""
-        stages = self._stages(
-            cli_overrides={"max_num_seqs": 256},
-            cli_explicit_keys=set(),  # user typed nothing
-        )
-        # Stage 2's YAML value (1) should win because the user didn't type --max-num-seqs.
-        assert stages[2].runtime_overrides.get("max_num_seqs") != 256
+    def test_none_value_skipped_yaml_wins(self):
+        stages = self._stages({"max_num_seqs": None})
+        assert stages[2].runtime_overrides.get("max_num_seqs") is None
+        assert stages[2].yaml_engine_args.get("max_num_seqs") == 1
 
-    def test_default_cli_fills_missing_yaml_field(self):
-        """Argparse defaults still fill fields the YAML doesn't set."""
-        stages = self._stages(
-            cli_overrides={"some_unrelated_knob": "fallback"},
-            cli_explicit_keys=set(),
-        )
-        # Field absent from YAML → CLI default flows through as a fallback.
-        assert stages[0].runtime_overrides.get("some_unrelated_knob") == "fallback"
+    def test_empty_kwargs_yaml_only(self):
+        stages = self._stages({})
+        for stage in stages:
+            assert stage.runtime_overrides == {}
 
-    def test_per_stage_overrides_always_explicit(self):
-        """``stage_<id>_*`` keys are always treated as explicit."""
-        stages = self._stages(
-            cli_overrides={"stage_0_gpu_memory_utilization": 0.42},
-            cli_explicit_keys=set(),  # not in the explicit set, but per-stage
-        )
+    def test_typed_kwarg_equal_to_dataclass_default_still_overrides(self):
+        # Caller intent honored regardless of value coincidence (no heuristic).
+        stages = self._stages({"gpu_memory_utilization": 0.9})
+        assert stages[2].runtime_overrides.get("gpu_memory_utilization") == 0.9
+
+    def test_per_stage_kwarg_routed_to_correct_stage(self):
+        stages = self._stages({"stage_0_gpu_memory_utilization": 0.42})
         assert stages[0].runtime_overrides.get("gpu_memory_utilization") == 0.42
+        assert stages[2].runtime_overrides.get("gpu_memory_utilization") is None
 
-    def test_none_explicit_set_treats_all_as_explicit(self):
-        """Programmatic Omni() callers (cli_explicit_keys=None) keep current behavior."""
-        stages = self._stages(
-            cli_overrides={"max_num_seqs": 999},
-            cli_explicit_keys=None,
-        )
-        assert stages[2].runtime_overrides.get("max_num_seqs") == 999
-
-    def test_explicit_async_chunk_false_overrides_yaml(self):
-        """``--no-async-chunk`` flips the deploy-level async_chunk to False even
-        when the YAML sets it to True. Verifies that the per-stage
-        ``async_chunk: True`` injection in ``merge_pipeline_deploy`` is skipped
-        and that ``async_chunk`` does not leak through ``_merge_cli_overrides``.
-        """
-        stages = self._stages(
-            cli_overrides={"async_chunk": False},
-            cli_explicit_keys={"async_chunk"},
-        )
-        # qwen3_omni_moe.yaml has `async_chunk: true`, so by default every
-        # stage's engine_args would carry it. With the explicit override, it
-        # must NOT show up.
+    def test_async_chunk_false_overrides_yaml_true(self):
+        stages = self._stages({"async_chunk": False})
         for stage in stages:
             assert stage.yaml_engine_args.get("async_chunk") is not True
-            assert stage.runtime_overrides.get("async_chunk") is None
 
-    def test_default_async_chunk_leaves_yaml_alone(self):
-        """An unset ``--async-chunk`` (default None) must leave the YAML's True
-        in force on every stage."""
-        stages = self._stages(
-            cli_overrides={"async_chunk": None},
-            cli_explicit_keys=set(),
-        )
-        # qwen3_omni_moe.yaml: `async_chunk: true` → injected on every stage.
+    def test_async_chunk_none_keeps_yaml_true(self):
+        stages = self._stages({"async_chunk": None})
         for stage in stages:
             assert stage.yaml_engine_args.get("async_chunk") is True
 
-    def test_explicit_enable_prefix_caching_overrides_yaml(self):
-        """``--enable-prefix-caching`` (global) flips every stage's
-        ``enable_prefix_caching`` to True regardless of the YAML default."""
-        stages = self._stages(
-            cli_overrides={"enable_prefix_caching": True},
-            cli_explicit_keys={"enable_prefix_caching"},
-        )
+    def test_enable_prefix_caching_typed_overrides_yaml(self):
+        stages = self._stages({"enable_prefix_caching": True})
         for stage in stages:
             assert stage.runtime_overrides.get("enable_prefix_caching") is True
 
+    def test_omni_with_vars_args_anti_pattern_is_safe(self):
+        # Omni(**vars(args)) with mostly-None namespace must not clobber YAML.
+        simulated_vars_args = {
+            "gpu_memory_utilization": None,
+            "max_num_seqs": None,
+            "async_chunk": None,
+            "enable_prefix_caching": None,
+            "dtype": None,
+        }
+        stages = self._stages(simulated_vars_args)
+        for stage in stages:
+            assert stage.runtime_overrides == {}
+
+    def test_create_from_registry_no_cli_explicit_keys_param(self):
+        import inspect
+
+        sig = inspect.signature(StageConfigFactory._create_from_registry)
+        named = [p for p in sig.parameters.values() if p.kind != p.VAR_KEYWORD]
+        assert "cli_explicit_keys" not in {p.name for p in named}
+
+    def test_cli_explicit_keys_kwarg_emits_deprecation(self):
+        import vllm_omni.model_executor.models.qwen3_omni.pipeline  # noqa: F401
+
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter("always")
+            StageConfigFactory._create_from_registry(
+                "qwen3_omni_moe",
+                cli_overrides={},
+                cli_explicit_keys={"max_num_seqs"},
+            )
+            assert any(issubclass(x.category, DeprecationWarning) for x in w)
+
     def test_async_chunk_dispatches_processors(self):
         """A single ``qwen3_tts`` pipeline picks per-chunk vs end-to-end
         processors based on ``deploy.async_chunk``, without needing a