-
-
Notifications
You must be signed in to change notification settings - Fork 12.9k
[Frontend] Use init_app_state and FrontendArgs from api_server in run_batch #32967
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Pooya Davoodi <pooya.davoodi@parasail.io>
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
The pull request successfully integrates init_app_state and FrontendArgs from api_server.py into run_batch.py, significantly reducing code duplication and enabling support for new features like tool calling. The changes to argument parsing for metrics, including renaming --port and --url to --metrics-port and --metrics-url respectively, are well-handled with backward compatibility. The addition of a comprehensive test case for tool calling ensures the new functionality works as expected. Overall, the changes improve modularity, maintainability, and extend the capabilities of run_batch.
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Signed-off-by: Pooya Davoodi <pooya.davoodi@parasail.io>
| ) | ||
| parser.add_argument( | ||
| "--url", | ||
| "--metrics-url", |
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.
We can avoid this by adding a new base class of FrontendArgs, then each subclass can have different definitions of host and port
Purpose
init_app_stateandFrontendArgsfromvllm/entrypoints/openai/api_server.py.--portbetween FrontendArgs and the existing run_batch options, we improve the option names from--portand--urlto--metrics-portand--metrics-urland provide a backward compatibility guarantee.Test Plan
Adding a new test for tool calling.
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.