Repository navigation
[Refactor][OutputProcessor 2/3]: OmniRequestOutput should inherit RequestOutput and no nested wrap-up - #5146
Conversation
are all of these needed? the output should only be related to different modalities: text, audio, video, image, action... |
Yes, this should be refactored to align with #4922 definition for better readability and usablity. But this will inflence a lot of files because this is the front-end part. This PR I dealt with some structure problems and I will refactor this in latter PR. |
| encoder_prompt: str | None = None | ||
| encoder_prompt_token_ids: list[int] | None = None | ||
| num_cached_tokens: int | None = None | ||
| kv_transfer_params: dict[str, Any] | None = None |
There was a problem hiding this comment.
multi_modal_placeholders is part of RequestOutput.__init__, but it is not redeclared as a dataclass field here. Since OmniRequestOutput is now encoded by msgspec as a dataclass, dynamic attrs copied from the wrapped RequestOutput are dropped on connector serialization; I reproduced multi_modal_placeholders becoming None after OmniMsgpackEncoder/Decoder. Please make this a real field and copy/round-trip it like the other RequestOutput content attrs.
There was a problem hiding this comment.
I am not sure whether multi_modal_placeholders is still necessary. I found upstream VLLM already removed it in commit vllm-project/vllm@f9ac19204. But our vllm-omni serialization.py still has multi_modal_placeholders. This is the only file in our vllm-omni repo has multi_modal_placeholders. If multi_modal_placeholders not used in the future, should we remove it in serialization.py?
| request_output=request_output, | ||
| finished=True, | ||
| ) | ||
| out.finished = True |
There was a problem hiding this comment.
This now changes the inner RequestOutput.finished semantics. _copy_content_from() copies request_output.finished, but then this overwrites it with True; because request_output now returns self, callers that still check output.request_output.finished will see True for unfinished stage outputs. Please preserve the wrapped RequestOutput.finished value here, or only force the wrapper-level flag through a separate field that does not affect the RequestOutput view.
| _dataclass_init = OmniRequestOutput.__init__ | ||
|
|
||
|
|
||
| def _omni_request_output_init(self: OmniRequestOutput, *args: Any, request_output: Any = None, **kwargs: Any) -> None: | ||
| _dataclass_init(self, *args, **kwargs) | ||
| if request_output is not None: | ||
| self._copy_content_from(request_output) | ||
|
|
||
|
|
||
| OmniRequestOutput.__init__ = _omni_request_output_init # type: ignore[method-assign] |
There was a problem hiding this comment.
this snippet looks less elegant and a little bit weird.
There was a problem hiding this comment.
fixed. Remove property and logic of mutating __init__.
hsliuustc0106
left a comment
There was a problem hiding this comment.
Requesting changes for the two compatibility issues in the inline comments:
multi_modal_placeholdersis part of theRequestOutputcontract but is not a realOmniRequestOutputdataclass field, so it is dropped by msgspec/dataclass connector serialization.from_pipeline()overwrites the copiedRequestOutput.finishedvalue withTrue; sincerequest_outputnow aliasesself, callers checkingoutput.request_output.finishedcan see unfinished stage outputs as finished.
Please fix these before merging.
Signed-off-by: bowieshi <aoibosh@connect.hku.hk>
Signed-off-by: bowieshi <aoibosh@connect.hku.hk>
local test for L3, 7 failed jobs fixed and passed. 2 failed job can not be tested in local (possible GPU error). |
Signed-off-by: bowieshi <aoibosh@connect.hku.hk>
Signed-off-by: bowieshi <aoibosh@connect.hku.hk>
Signed-off-by: bowieshi <aoibosh@connect.hku.hk>
|
fix conflicts and let's get it merged |
Signed-off-by: bowieshi <aoibosh@connect.hku.hk>
…n weekly CI main's fcf1085 (vllm-project#5146) made OmniRequestOutput inherit RequestOutput rather than nest one, so `.request_output` no longer exists. The e2e file and the offline example both still read it and raised AttributeError before producing any audio; the branch rebased past the refactor without migrating. Add the weekly Gepard step the `TO CONFIRM` note in test-merge.yml was asking for, and drop the note. The CI image carries no NanoCodec, so the step installs nemo_toolkit[tts] with the README's overrides first; without a step anywhere these tests ran in no tier at all, which is how the stale accessor survived. Also document `VLLM_USE_FLASHINFER_SAMPLER=0` for hosts whose CUDA toolkit cannot build the sampler kernels, matching the Qwen3-TTS and JoyAI-VL notes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Mengjie Zhao <zmj0129@gmail.com>
Blocking fixes from the maintainer review on vllm-project#3422. 1. Stale output accessor (offline example + offline e2e suite) `OmniRequestOutput` has subclassed `RequestOutput` since vllm-project#5146, so the nested `.request_output` no longer exists and both files crashed with `AttributeError` *after* synthesizing correctly. They were the only two users left in the tree, and `tests/docs/test_example_output_accessors.py` -- a cpu/core_model guard -- was already failing on them, so this was red at L1, not only in the GPU lane. Each item yielded by `Omni.generate` is itself an `OmniRequestOutput`, so read `.outputs` off it directly. Note `OmniRequestOutput` is *not* iterable: the `moss_tts_nano` example cited in review iterates it and would raise `TypeError`, so this follows `gepard/end2end.py` instead, which has the same two-level indexing. 2. Online requests could not influence Vevo2's sampling, including `seed` Vevo2 samples inside Amphion's `inference_ar_and_fm`, reading its knobs from `additional_information`; the `SamplingParams` the dummy AR scheduler carries only drive the forced-EOS sampler. `Vevo2Adapter.build` never copied anything across, so `/v1/audio/speech` seeds were inert and `default_sampling_params.seed` was a dead setting. `build()` runs before the shared path writes `request.seed` onto `SamplingParams`, so the `moss_tts.py` precedent would have wired the deploy default rather than the request seed. Read `request.seed` directly and fall back to the deploy default; also forward top_k / top_p / temperature / flow_matching_steps from `extra_params`. 3. Seeded requests leaked global RNG state across a batch The save/restore `finally` sat after `yield chunk`, so it only ran when the generator was resumed on a later `forward()` step -- leaving the process RNG seeded while batch siblings ran, and never returning to the original state under nested save/restore. Seeding now happens in a `_seeded_rng` context manager scoped to the inference call alone, never spanning a yield. Also in this change: - `forward()` creates every generator for a step before advancing any, and `_create_stream_gen` validates eagerly, so one malformed row no longer aborts a step that already consumed its siblings' chunks. The remaining raise-on-inference-failure is documented against the missing per-row error channel in `OmniOutput`. - `init_vevo2_checkpoint.py` is idempotent and repairs in place, re-raises instead of warning on a failed placeholder write, fails loudly on a missing AR sub-checkpoint, and verifies every root artifact before reporting success. The CI lane gates the download on a sub-checkpoint the initialiser never writes and always runs the initialiser. - The `LlamaConfig` positional shim is a scoped context manager instead of a permanent process-wide patch, and rejects duplicate positional/keyword arguments. Dropped the `rope_scaling` backfill: it never fired, and on transformers>=5 `rope_scaling` is a write-through property over `rope_parameters`, so forcing it to None would clear the rope config. - Deploy YAML and recipe restate the memory budget subtractively, matching `worker/base.py`: the pipeline is subtracted from `gpu_memory_utilization`, not stacked on top of it. - Recipe verification numbers re-attributed to the paths that produced them, and the broken `#vevo2-license-considerations` anchor fixed. - `run_server.sh` and the `end2end.py` docstring document the one-time init step and use `hf download`. - Tests: streaming lifecycle (waveform -> sentinel -> EOS -> cleanup, and the runaway-generation guard), RNG hygiene, eager validation, adapter seed plumbing, shim scoping, online seed reproducibility, a 400-not-5xx assertion, output duration bounds, and an offline batch test that actually distinguishes isolation from reproducibility. 18 -> 33 CPU unit tests. Signed-off-by: Yash Jain <yash@metica.com>
Blocking fixes from the maintainer review on vllm-project#3422. 1. Stale output accessor (offline example + offline e2e suite) `OmniRequestOutput` has subclassed `RequestOutput` since vllm-project#5146, so the nested `.request_output` no longer exists and both files crashed with `AttributeError` *after* synthesizing correctly. They were the only two users left in the tree, and `tests/docs/test_example_output_accessors.py` -- a cpu/core_model guard -- was already failing on them, so this was red at L1, not only in the GPU lane. Each item yielded by `Omni.generate` is itself an `OmniRequestOutput`, so read `.outputs` off it directly. Note `OmniRequestOutput` is *not* iterable: the `moss_tts_nano` example cited in review iterates it and would raise `TypeError`, so this follows `gepard/end2end.py` instead, which has the same two-level indexing. 2. Online requests could not influence Vevo2's sampling, including `seed` Vevo2 samples inside Amphion's `inference_ar_and_fm`, reading its knobs from `additional_information`; the `SamplingParams` the dummy AR scheduler carries only drive the forced-EOS sampler. `Vevo2Adapter.build` never copied anything across, so `/v1/audio/speech` seeds were inert and `default_sampling_params.seed` was a dead setting. `build()` runs before the shared path writes `request.seed` onto `SamplingParams`, so the `moss_tts.py` precedent would have wired the deploy default rather than the request seed. Read `request.seed` directly and fall back to the deploy default; also forward top_k / top_p / temperature / flow_matching_steps from `extra_params`. 3. Seeded requests leaked global RNG state across a batch The save/restore `finally` sat after `yield chunk`, so it only ran when the generator was resumed on a later `forward()` step -- leaving the process RNG seeded while batch siblings ran, and never returning to the original state under nested save/restore. Seeding now happens in a `_seeded_rng` context manager scoped to the inference call alone, never spanning a yield. Also in this change: - `forward()` creates every generator for a step before advancing any, and `_create_stream_gen` validates eagerly, so one malformed row no longer aborts a step that already consumed its siblings' chunks. The remaining raise-on-inference-failure is documented against the missing per-row error channel in `OmniOutput`. - `init_vevo2_checkpoint.py` is idempotent and repairs in place, re-raises instead of warning on a failed placeholder write, fails loudly on a missing AR sub-checkpoint, and verifies every root artifact before reporting success. The CI lane gates the download on a sub-checkpoint the initialiser never writes and always runs the initialiser. - The `LlamaConfig` positional shim is a scoped context manager instead of a permanent process-wide patch, and rejects duplicate positional/keyword arguments. Dropped the `rope_scaling` backfill: it never fired, and on transformers>=5 `rope_scaling` is a write-through property over `rope_parameters`, so forcing it to None would clear the rope config. - Deploy YAML and recipe restate the memory budget subtractively, matching `worker/base.py`: the pipeline is subtracted from `gpu_memory_utilization`, not stacked on top of it. - Recipe verification numbers re-attributed to the paths that produced them, and the broken `#vevo2-license-considerations` anchor fixed. - `run_server.sh` and the `end2end.py` docstring document the one-time init step and use `hf download`. - Tests: streaming lifecycle (waveform -> sentinel -> EOS -> cleanup, and the runaway-generation guard), RNG hygiene, eager validation, adapter seed plumbing, shim scoping, online seed reproducibility, a 400-not-5xx assertion, output duration bounds, and an offline batch test that actually distinguishes isolation from reproducibility. 18 -> 33 CPU unit tests. Signed-off-by: Yash Jain <yash@metica.com>
Blocking fixes from the maintainer review on vllm-project#3422. 1. Stale output accessor (offline example + offline e2e suite) `OmniRequestOutput` has subclassed `RequestOutput` since vllm-project#5146, so the nested `.request_output` no longer exists and both files crashed with `AttributeError` *after* synthesizing correctly. They were the only two users left in the tree, and `tests/docs/test_example_output_accessors.py` -- a cpu/core_model guard -- was already failing on them, so this was red at L1, not only in the GPU lane. Each item yielded by `Omni.generate` is itself an `OmniRequestOutput`, so read `.outputs` off it directly. Note `OmniRequestOutput` is *not* iterable: the `moss_tts_nano` example cited in review iterates it and would raise `TypeError`, so this follows `gepard/end2end.py` instead, which has the same two-level indexing. 2. Online requests could not influence Vevo2's sampling, including `seed` Vevo2 samples inside Amphion's `inference_ar_and_fm`, reading its knobs from `additional_information`; the `SamplingParams` the dummy AR scheduler carries only drive the forced-EOS sampler. `Vevo2Adapter.build` never copied anything across, so `/v1/audio/speech` seeds were inert and `default_sampling_params.seed` was a dead setting. `build()` runs before the shared path writes `request.seed` onto `SamplingParams`, so the `moss_tts.py` precedent would have wired the deploy default rather than the request seed. Read `request.seed` directly and fall back to the deploy default; also forward top_k / top_p / temperature / flow_matching_steps from `extra_params`. 3. Seeded requests leaked global RNG state across a batch The save/restore `finally` sat after `yield chunk`, so it only ran when the generator was resumed on a later `forward()` step -- leaving the process RNG seeded while batch siblings ran, and never returning to the original state under nested save/restore. Seeding now happens in a `_seeded_rng` context manager scoped to the inference call alone, never spanning a yield. Also in this change: - `forward()` creates every generator for a step before advancing any, and `_create_stream_gen` validates eagerly, so one malformed row no longer aborts a step that already consumed its siblings' chunks. The remaining raise-on-inference-failure is documented against the missing per-row error channel in `OmniOutput`. - `init_vevo2_checkpoint.py` is idempotent and repairs in place, re-raises instead of warning on a failed placeholder write, fails loudly on a missing AR sub-checkpoint, and verifies every root artifact before reporting success. The CI lane gates the download on a sub-checkpoint the initialiser never writes and always runs the initialiser. - The `LlamaConfig` positional shim is a scoped context manager instead of a permanent process-wide patch, and rejects duplicate positional/keyword arguments. Dropped the `rope_scaling` backfill: it never fired, and on transformers>=5 `rope_scaling` is a write-through property over `rope_parameters`, so forcing it to None would clear the rope config. - Deploy YAML and recipe restate the memory budget subtractively, matching `worker/base.py`: the pipeline is subtracted from `gpu_memory_utilization`, not stacked on top of it. - Recipe verification numbers re-attributed to the paths that produced them, and the broken `#vevo2-license-considerations` anchor fixed. - `run_server.sh` and the `end2end.py` docstring document the one-time init step and use `hf download`. - Tests: streaming lifecycle (waveform -> sentinel -> EOS -> cleanup, and the runaway-generation guard), RNG hygiene, eager validation, adapter seed plumbing, shim scoping, online seed reproducibility, a 400-not-5xx assertion, output duration bounds, and an offline batch test that actually distinguishes isolation from reproducibility. 18 -> 33 CPU unit tests. Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
Blocking fixes from the maintainer review on vllm-project#3422. 1. Stale output accessor (offline example + offline e2e suite) `OmniRequestOutput` has subclassed `RequestOutput` since vllm-project#5146, so the nested `.request_output` no longer exists and both files crashed with `AttributeError` *after* synthesizing correctly. They were the only two users left in the tree, and `tests/docs/test_example_output_accessors.py` -- a cpu/core_model guard -- was already failing on them, so this was red at L1, not only in the GPU lane. Each item yielded by `Omni.generate` is itself an `OmniRequestOutput`, so read `.outputs` off it directly. Note `OmniRequestOutput` is *not* iterable: the `moss_tts_nano` example cited in review iterates it and would raise `TypeError`, so this follows `gepard/end2end.py` instead, which has the same two-level indexing. 2. Online requests could not influence Vevo2's sampling, including `seed` Vevo2 samples inside Amphion's `inference_ar_and_fm`, reading its knobs from `additional_information`; the `SamplingParams` the dummy AR scheduler carries only drive the forced-EOS sampler. `Vevo2Adapter.build` never copied anything across, so `/v1/audio/speech` seeds were inert and `default_sampling_params.seed` was a dead setting. `build()` runs before the shared path writes `request.seed` onto `SamplingParams`, so the `moss_tts.py` precedent would have wired the deploy default rather than the request seed. Read `request.seed` directly and fall back to the deploy default; also forward top_k / top_p / temperature / flow_matching_steps from `extra_params`. 3. Seeded requests leaked global RNG state across a batch The save/restore `finally` sat after `yield chunk`, so it only ran when the generator was resumed on a later `forward()` step -- leaving the process RNG seeded while batch siblings ran, and never returning to the original state under nested save/restore. Seeding now happens in a `_seeded_rng` context manager scoped to the inference call alone, never spanning a yield. Also in this change: - `forward()` creates every generator for a step before advancing any, and `_create_stream_gen` validates eagerly, so one malformed row no longer aborts a step that already consumed its siblings' chunks. The remaining raise-on-inference-failure is documented against the missing per-row error channel in `OmniOutput`. - `init_vevo2_checkpoint.py` is idempotent and repairs in place, re-raises instead of warning on a failed placeholder write, fails loudly on a missing AR sub-checkpoint, and verifies every root artifact before reporting success. The CI lane gates the download on a sub-checkpoint the initialiser never writes and always runs the initialiser. - The `LlamaConfig` positional shim is a scoped context manager instead of a permanent process-wide patch, and rejects duplicate positional/keyword arguments. Dropped the `rope_scaling` backfill: it never fired, and on transformers>=5 `rope_scaling` is a write-through property over `rope_parameters`, so forcing it to None would clear the rope config. - Deploy YAML and recipe restate the memory budget subtractively, matching `worker/base.py`: the pipeline is subtracted from `gpu_memory_utilization`, not stacked on top of it. - Recipe verification numbers re-attributed to the paths that produced them, and the broken `#vevo2-license-considerations` anchor fixed. - `run_server.sh` and the `end2end.py` docstring document the one-time init step and use `hf download`. - Tests: streaming lifecycle (waveform -> sentinel -> EOS -> cleanup, and the runaway-generation guard), RNG hygiene, eager validation, adapter seed plumbing, shim scoping, online seed reproducibility, a 400-not-5xx assertion, output duration bounds, and an offline batch test that actually distinguishes isolation from reproducibility. 18 -> 33 CPU unit tests. Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
…lm-project#5146 examples/offline_inference/text_to_speech/qwen3_tts/word_timestamps.py was deleted in vllm-project#4795 when word timestamps became a serving-path pipeline stage, re-added by vllm-project#5146 in a rebase, and later edited by vllm-project#6200. On main it fails at import: it imports `align` from vllm_omni.utils.forced_aligner, which no longer exists, and calls build_forced_aligner_config with the pre-vllm-project#4795 signature. Nothing else references the file; the README in the same directory already points to `vllm-omni serve --forced-aligner`. Co-authored-by: Claude Signed-off-by: Tianyao Wu <rayroy31@gmail.com>
Blocking fixes from the maintainer review on vllm-project#3422. 1. Stale output accessor (offline example + offline e2e suite) `OmniRequestOutput` has subclassed `RequestOutput` since vllm-project#5146, so the nested `.request_output` no longer exists and both files crashed with `AttributeError` *after* synthesizing correctly. They were the only two users left in the tree, and `tests/docs/test_example_output_accessors.py` -- a cpu/core_model guard -- was already failing on them, so this was red at L1, not only in the GPU lane. Each item yielded by `Omni.generate` is itself an `OmniRequestOutput`, so read `.outputs` off it directly. Note `OmniRequestOutput` is *not* iterable: the `moss_tts_nano` example cited in review iterates it and would raise `TypeError`, so this follows `gepard/end2end.py` instead, which has the same two-level indexing. 2. Online requests could not influence Vevo2's sampling, including `seed` Vevo2 samples inside Amphion's `inference_ar_and_fm`, reading its knobs from `additional_information`; the `SamplingParams` the dummy AR scheduler carries only drive the forced-EOS sampler. `Vevo2Adapter.build` never copied anything across, so `/v1/audio/speech` seeds were inert and `default_sampling_params.seed` was a dead setting. `build()` runs before the shared path writes `request.seed` onto `SamplingParams`, so the `moss_tts.py` precedent would have wired the deploy default rather than the request seed. Read `request.seed` directly and fall back to the deploy default; also forward top_k / top_p / temperature / flow_matching_steps from `extra_params`. 3. Seeded requests leaked global RNG state across a batch The save/restore `finally` sat after `yield chunk`, so it only ran when the generator was resumed on a later `forward()` step -- leaving the process RNG seeded while batch siblings ran, and never returning to the original state under nested save/restore. Seeding now happens in a `_seeded_rng` context manager scoped to the inference call alone, never spanning a yield. Also in this change: - `forward()` creates every generator for a step before advancing any, and `_create_stream_gen` validates eagerly, so one malformed row no longer aborts a step that already consumed its siblings' chunks. The remaining raise-on-inference-failure is documented against the missing per-row error channel in `OmniOutput`. - `init_vevo2_checkpoint.py` is idempotent and repairs in place, re-raises instead of warning on a failed placeholder write, fails loudly on a missing AR sub-checkpoint, and verifies every root artifact before reporting success. The CI lane gates the download on a sub-checkpoint the initialiser never writes and always runs the initialiser. - The `LlamaConfig` positional shim is a scoped context manager instead of a permanent process-wide patch, and rejects duplicate positional/keyword arguments. Dropped the `rope_scaling` backfill: it never fired, and on transformers>=5 `rope_scaling` is a write-through property over `rope_parameters`, so forcing it to None would clear the rope config. - Deploy YAML and recipe restate the memory budget subtractively, matching `worker/base.py`: the pipeline is subtracted from `gpu_memory_utilization`, not stacked on top of it. - Recipe verification numbers re-attributed to the paths that produced them, and the broken `#vevo2-license-considerations` anchor fixed. - `run_server.sh` and the `end2end.py` docstring document the one-time init step and use `hf download`. - Tests: streaming lifecycle (waveform -> sentinel -> EOS -> cleanup, and the runaway-generation guard), RNG hygiene, eager validation, adapter seed plumbing, shim scoping, online seed reproducibility, a 400-not-5xx assertion, output duration bounds, and an offline batch test that actually distinguishes isolation from reproducibility. 18 -> 33 CPU unit tests. Signed-off-by: Yash Jain <yashjainjain1704@gmail.com>
…uestOutput and no nested wrap-up (vllm-project#5146) Signed-off-by: Boao Shi <aoibosh@connect.hku.hk> Signed-off-by: bowieshi <aoibosh@connect.hku.hk> Signed-off-by: princepride <wangzhipeng628@gmail.com> Signed-off-by: R2-Y <ruiruyang2@gmail.com> Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Signed-off-by: Shi Boao Bowie <aoibosh@connect.hku.hk> Co-authored-by: 汪志鹏 <wangzhipeng628@gmail.com> Co-authored-by: ruirui(rein) yang <73573651+R2-Y@users.noreply.github.com> Co-authored-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Gao Han <hgaoaf@connect.ust.hk> Co-authored-by: Hongsheng Liu <liuhongsheng4@huawei.com>
(#7233) Signed-off-by: Tianyao Wu <rayroy31@gmail.com>

PLEASE FILL IN THE PR DESCRIPTION HERE.
Purpose
OmniRequestOutput taks RequestOutput as a field. However, RequestOutput is a class that contains the output of a LLM output. Our OmniRequestOutput is designed to be a unified output class for both LLM and diffusion models, which should inherit from RequestOutput instead of containing it as a field. This will allow us to use polymorphism and treat OmniRequestOutput as a RequestOutput when needed, while still having the additional fields for diffusion models.
Related code snippets:
vllm_omni/outputs.py: line 62 ~ line 111before modification:
After modification:
Test Plan
vLLM Version: 0.25.0
vLLM-Omni Commit: 4c40955
Test Result
pytest -s -v -m "core_model and cpu"BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.
(anything written below this line will be removed by GitHub Actions)