Repository navigation
[Core] Flag pipelines with no terminal output stage in PipelineConfig.validate() - #6291
Conversation
|
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/vllm_omni_config.md, docs/design/module/stage_runtime.md, docs/design/module/entrypoints.md. Module owners: @lishunyang12 @alex-jw-brooks @tzhouam @fake0fan @linyueqian @NickCao @m0g3r, 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. |
….validate() `PipelineConfig.validate()` already rejects an empty pipeline, duplicate stage IDs, dangling and self-referential `input_sources`, and a missing entry point, but it accepts a pipeline in which no stage declares `final_output=True`. Request completion and client-visible output are both driven by stages that declare `final_output` (`Orchestrator._route_output`), so such a pipeline can never emit a result and every request hangs. Today `validate()` returns `[]` for it, and `register_pipeline` logs nothing. Add the missing check. It is cycle-agnostic: it makes no acyclicity assumption and does not detect cycles, so it stays compatible with the circular-stage work in vllm-project#831. Any stage may carry `final_output`, not only the last one. Also add an L1 CPU guard that every statically registered in-tree pipeline passes `validate()`. `validate()` runs only on `register_pipeline`, which in-tree pipelines never go through, so a shipped topology error would otherwise stay invisible until serving hangs. `TestPipelineConfigNew.test_validate_valid` described a two-stage pipeline with no terminal stage; its last stage is now marked `final_output=True` so the fixture is an actually-servable topology. Partially addresses vllm-project#4560 (the validation gap). The read-only topology-dump helper in that issue is deliberately not included: it is new public API and a separate design decision. Signed-off-by: m0g3r <87276771+m0g3r@users.noreply.github.com>
a894328 to
e1dced3
Compare
alex-jw-brooks
left a comment
There was a problem hiding this comment.
Thanks for the fix, looks good to me!
….validate() (vllm-project#6291) Signed-off-by: m0g3r <87276771+m0g3r@users.noreply.github.com>
….validate() (vllm-project#6291) Signed-off-by: m0g3r <87276771+m0g3r@users.noreply.github.com>
Purpose
Partially addresses #4560 — the first of the two gaps listed there.
PipelineConfig.validate()(vllm_omni/config/stage_config.py) already reports an empty pipeline, duplicate stage IDs, dangling and self-referentialinput_sources, and a missing entry point. It accepts a pipeline in which no stage declaresfinal_output=True.That topology cannot work. Both request completion and client-visible output are driven by stages that declare
final_output, inOrchestrator._route_output:With no
final_outputstage anywhere, neither branch is ever taken: noOutputMessageis emitted andrequest_finishednever becomes true, so every request hangs.validate()returns[]for it andregister_pipelinelogs nothing.Repro (the one from the issue, unchanged, against current
main):What this changes
PipelineConfig.validate()gains the missing check (5 lines). It is cycle-agnostic, per the issue's explicit non-goal: it asserts no acyclicity, does not detect cycles, and does not require the terminal stage to be last — so it stays compatible with the circular-stage work in [RFC]: Support circular dependencies between stages #831. Any stage may carryfinal_output.An L1 CPU guard that every statically registered in-tree pipeline passes
validate().validate()runs only fromregister_pipeline, which in-tree pipelines never go through, so today a shipped topology error stays invisible until serving hangs. Resolver entries are skipped — they need anhf_configto produce aPipelineConfig. Happy to drop this test if you'd rather keep the diff to the validator alone.TestPipelineConfigNew.test_validate_validis corrected. Its fixture was a two-stage pipeline with no terminal stage, assertingvalidate() == []. Its last stage is nowfinal_output=True, so the "valid pipeline" fixture describes an actually-servable topology. That the repo's own valid-pipeline fixture was not servable is the gap this PR closes.Not included, deliberately: the read-only topology-dump helper (entry stages / terminal stages / adjacency) that #4560 also proposes. That is new public API and a separate design decision, especially against the config refactors in #2072 / #4021 / #6200. Say the word and I'll add it here or in a follow-up.
Blast radius: the only production behaviour change is one extra string in the list
register_pipelinelogs for an out-of-tree pipeline that cannot emit output. No in-tree pipeline is affected — see the audit below.Test Plan
L1 unit / CPU, per the
vllm-omni-testskill. Both target modules already carrypytestmark = [pytest.mark.core_model, pytest.mark.cpu], so they are already collected by the Simple Unit Test step intest-ready.yml/test-merge.yml. No Buildkite YAML change is needed.tests/config/test_config_factory.py::TestPipelineConfigNewtest_validate_no_terminal_stage— new; the regression test, fails without the fixtest_validate_terminal_stage_need_not_be_last— new; pins the cycle-agnostic contracttest_validate_valid— fixture correctedtests/config/test_pipeline_registry.py::test_in_tree_pipelines_pass_topology_validation— new guardvLLM Version: 0.27.0 (as pinned by this branch's base; not installed on the machine used here — see below)
vLLM-Omni Commit:
2fdbf2234aeb76715618a5b236f0016f115e3e64Test Result
Checks that were run
validate()against the issue's repro[]before →['No terminal stage (stage with final_output=True)']afterPipelineConfigre-validated with the patched checkerpre-commit run --files <the 3 changed files>check-test-ci-coverage,check-pickle-imports)python -m py_compileon the 3 changed files (3.12.13)precheck-prcode-quality sweep (5 diff-scoped patterns)precheck-prexamples-policy sweepexamples/**paths touchedmocker/monkeypatchonly)Red/green was measured by lifting the committed test methods out of
tests/config/test_config_factory.pywithastand running them against two sources:git show HEAD:vllm_omni/config/stage_config.py(pre-fix) and the patched file. So the failure is on the code that ships, not on a paraphrase of it.For the in-tree guard, every literal
PipelineConfig(...)invllm_omni/was rebuilt and re-validated with the patched checker (includingming_tts_moe, which reusesMING_TTS_PIPELINE.stages):That is a superset of what
test_in_tree_pipelines_pass_topology_validationasserts, which covers only thePipelineConfiginstances actually inOMNI_PIPELINES. So the new guard should be green in CI, and no shipped pipeline regresses.Checks that could NOT be run locally, and why
pytestwas not run on this machine. vLLM 0.27 has no usable macOS arm64 wheel, sotests/config/test_config_factory.pyandtests/config/test_pipeline_registry.pycannot even be imported here (vllm_omnipulls invllm). The red/green above comes from a dependency-isolated harness that stubs only this module's external imports (transformers,vllm.logger,vllm.v1.core.sched.scheduler,vllm_omni.config.endpoint_policy,vllm_omni.config.yaml_util, the two omni schedulers) and then executes the realvllm_omni/config/stage_config.pysource, soPipelineConfig.validate()under test is the genuine implementation. It is not a substitute for a CI pytest run, andpy_compileis not a test pass.astrather than importingOMNI_PIPELINES, for the same dependency reason. Resolver-produced pipelines are outside both that audit and the new test.Please re-run the L1 CPU suite in CI; if the guard test surfaces anything I could not see from static reconstruction, I'll fix it here.
Disclosure: this change was implemented with AI coding-agent assistance, using the repository's own
vllm-omni-testandprecheck-prskills as instructed indocs/contributing/README.md. Every command and result reported above was actually executed; the one check I could not run locally (pytest) is named explicitly rather than implied to have passed.