Repository navigation
[Core] Validate PipelineConfig in __post_init__ - #6829
Conversation
Signed-off-by: Alex Brooks <albrooks@redhat.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com>
|
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. Module owners: @lishunyang12 @alex-jw-brooks @alex-jw-brooks, 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. |
hsliuustc0106
left a comment
There was a problem hiding this comment.
Two findings, both inlined:
- P1 (stage_config.py:306): eager validation raises for three test files the PR does not update —
tests/config/test_pipeline_registry.py:33,44,tests/config/test_omni_config.py:1042,1072(stageless constructions) andtests/utils/test_tracking_parser.py:33(module-level pipeline with nofinal_output=Truestage → whole-module collection error). No test lane ran on this PR, so the cpu lanes go red on merge. Fix: give these constructions a minimal terminal stage likeSINGLE_STAGE_PIPE_CFG. - P3 (stage_config.py:342):
maybe_get_validation_errors_as_stris a single-caller helper — consider inlining into__post_init__.
Otherwise clean: verified there are no leftover .validate() callers and no in-tree production path constructs a PipelineConfig without stages, so the eager-fail design itself is safe in-tree.
Signed-off-by: Alex Brooks <albrooks@redhat.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com>
|
Thanks @hsliuustc0106! I agree with your points. I inlined the helper and added the missing stage to other |
main added revision=od_config.revision to the from_pretrained calls (PR vllm-project#6829); the CPU-offload branch reworked vae loading to conditional placement. Keep both: revision passthrough plus loader-managed placement under enable_cpu_offload / enable_layerwise_offload. Signed-off-by: VLa1111 <vyu112@foxmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com> Signed-off-by: ZhengWG <zwg0606@gmail.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com>
Purpose
Follow-up to #6291; rather than having a separate
validate()function that we can call to get a list of errors, we should actually validate & raise an error if aPipelineConfigis not correctly configured. This will ensure that we do not accidentally create invalidPipelineConfigobjects in vLLM Omni, and also is a better user experience when registering out of tree pipelines (i.e.,PipelineConfigobjects should explode as early as possible rather than deferring until we try to register it).Errors surface as being joined by
\n\tif there are more than one. E.g.,will raise
will raise
to try to keep it readable.
CC @lishunyang12 @m0g3r @hsliuustc0106 - PTAL, thanks!