-
-
Notifications
You must be signed in to change notification settings - Fork 9.2k
Improve literal dataclass field conversion to argparse argument #17391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the conversion of Literal type hints for argparse by introducing a helper function (literal_to_kwargs) to centralize the logic. It also enforces that fields using Literals must merge nested literal values (e.g. Literal[Literal[1], Literal[2]] rather than Union[Literal[1], Literal[2]]), and adds tests to validate these behaviors.
- Consolidates Literal-to-kwargs conversion into a single helper function.
- Enforces stricter validation for Literal types in dataclass field definitions.
- Adds test cases to ensure proper handling of Literal and list[Literal] types.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
vllm/engine/arg_utils.py | Introduces literal_to_kwargs and updates get_kwargs accordingly. |
vllm/config.py | Updates field validation to mandate merged Literal types. |
tests/test_config.py | Adds tests to check proper configuration and error messages. |
tests/engine/test_arg_utils.py | Adds tests for literal_to_kwargs behavior. |
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
…-project#17391) Signed-off-by: Harry Mellor <[email protected]>
…-project#17391) Signed-off-by: Harry Mellor <[email protected]>
…-project#17391) Signed-off-by: Harry Mellor <[email protected]> Signed-off-by: Mu Huai <[email protected]>
…-project#17391) Signed-off-by: Harry Mellor <[email protected]> Signed-off-by: Yuqi Zhang <[email protected]>
…-project#17391) Signed-off-by: Harry Mellor <[email protected]> Signed-off-by: minpeter <[email protected]>
Literal
s are merged likeLiteral[Literal[1], Literal[2]]
instead ofUnion[Literal[1], Literal[2]]
(this stops us from needing complicatedLiteral
merging inget_kwargs
)list[Literal]
where we want the argument to accept multiple values from a set of choices