[Config Refactor] Validate Engine Args From CLI#3008
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71fea3ae18
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
fix conflicts |
Signed-off-by: Alex Brooks <albrooks@redhat.com> minor refactoring Signed-off-by: Alex Brooks <albrooks@redhat.com>
Signed-off-by: Alex Brooks <albrooks@redhat.com>
alex-jw-brooks
force-pushed
the
from_cli_overrides
branch
from
31d1f2e to
e2ca7a9
Compare
|
Thanks! Done |
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
Good type safety improvement. A few notes:
Potential Breaking Behavior: This PR raises ValidationError for invalid types that were previously silently filtered. If existing deployments were passing malformed types, this will surface them as errors. This is the correct behavior, but worth noting in the migration path.
Suggestions:
- Consider the severity of logging unknown keys — DEBUG is fine for now, but INFO might help users debug config issues.
- The strict=True in TypeAdapter.validate_python will reject type coercion (e.g., string "0.5" for float 0.5). This is intentional, but could surprise users coming from argparse defaults. Add a comment or test explaining the choice.
|
@alex-jw-brooks Hi, can you refractor the PR as #3144 will be integrated and from_cli_args() will be dropped to make the front-end API clean as before. |
|
@lishunyang12, sure, I'll rebase it onto @xiaohajiayou's PR so that we can merge #3144 before this one 😄 |
lishunyang12
left a comment
lishunyang12
left a comment
There was a problem hiding this comment.
Per #3144, from_cli_args is being removed in favor of direct Omni(**vars(args)). Suggest exposing get_validated_args_dict as a top-level helper so both this method and the new direct-construction path share validation; otherwise the two PRs land with diverging behavior.
| that are unknown, and validate the values of known keys, raising if we | ||
| get incorrect types.""" | ||
| validated_kwargs = {} | ||
| field_type_map = get_type_hints(cls) |
There was a problem hiding this comment.
get_type_hints(cls) evaluates string annotations at call time and walks the parent EngineArgs from upstream vllm. If any inherited field's annotation references a symbol not importable from this module's namespace, this raises NameError rather than the cleaner dataclasses.fields(cls) + per-field resolution. Has bitten upstream before across vllm version bumps.
| else: | ||
| field_type = field_type_map[key] | ||
| if field_type is not Any: | ||
| value = TypeAdapter(field_type).validate_python(value, strict=True) |
There was a problem hiding this comment.
strict=True rejects argparse None for fields typed as plain int (no | None). Argparse defaults frequently produce None for unprovided optional args. Add a positive-path test that runs against a realistic full argparse namespace, not just Namespace(model=...).
| OmniEngineArgs.from_cli_args(ns) | ||
|
|
||
|
|
||
| def test_invalid_cli_args_logs_unrecognized_kwargs(caplog): |
There was a problem hiding this comment.
Missing positive round-trip test — only the log-fires path is asserted. Add a case that constructs a valid Namespace, calls from_cli_args, and verifies values pass through TypeAdapter unchanged.
|
@xiaohajiayou @lishunyang12 for now I think I'm actually going to close this one, I think the validation on the type adapter doesn't make as much sense with the current state. Will work on some stuff related to this, if it makes sense to have the validation as part of what comes out if it, I'll integrate it there: #3162 (comment) |
3 participants
Purpose
Addresses #3 on #2887 and validates types for overrides using Pydantic TypeAdapters. also adds a debug log for the set of keys that are dropped.
Test Plan
Tests added ensuring that the log fires & that passing bad types now raises a ValidationError through pydantic.
Test Result
Tests pass.
CC @lishunyang12 @hsliuustc0106