-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
[Bugfix] Fix --config arg expansion called from api_server.py #23944
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
|
👋 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 You ask your reviewers to trigger select CI tests on top of 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 If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
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.
Code Review
This pull request correctly fixes a bug where the --config command-line argument was not properly handled when using the api_server.py entrypoint directly. The root cause was correctly identified: the argument parser expected a subcommand which is not always present. The fix introduces a new conditional branch to handle this specific case by checking if the first argument is an option. This change is well-contained and effective. Additionally, new unit tests have been added to cover the fixed behavior, including argument overrides, which strengthens the codebase. The overall change is solid and well-implemented.
DarkLight1337
left a comment
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 for fixing!
Signed-off-by: Jean-Francois Dube <dubejf+gh@gmail.com>
Signed-off-by: Jean-Francois Dube <dubejf+gh@gmail.com>
Head branch was pushed to by a user without write access
11916bf to
2358195
Compare
…roject#23944) Signed-off-by: Jean-Francois Dube <dubejf+gh@gmail.com> Co-authored-by: Jean-Francois Dube <dubejf+gh@gmail.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk>
…roject#23944) Signed-off-by: Jean-Francois Dube <dubejf+gh@gmail.com> Co-authored-by: Jean-Francois Dube <dubejf+gh@gmail.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk>
Purpose
Per
--help, the argument parser for thevllmCLI and theapi_serverentry point both accept the--configcommand-line argument, but only the CLI handles it correctly. This PR ensures consistent config file handling for all entry points.Examples:
vllm serve --config config.yaml✅ Works as expected
docker run <docker_arguments> vllm/vllm-openai --config config.yaml❌
api_server.py: error: argument --config: expected one argumentpython3 -m vllm.entrypoints.openai.api_server --config config.yaml❌
api_server.py: error: argument --config: expected one argumentCause:
_pull_args_from_configexpects the first argument to always be avllmCLI subcommand (serve,chat,complete, etc.), which is not the case forapi_server.This leads to incorrect expansion:
is mistakenly expanded to:
instead of:
Test Plan
--configargument parsing intests/entrypoints/openai/test_cli_args.py.Test Result
tests/utils_/test_utils.py) already cover configuration file expansion in thevllm servecontext; no changes required.Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.