add default stop string #29718
add default stop string #29718
Conversation
add default stop string
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to specify default stop tokens for all requests via a command-line argument. My review identifies a critical bug in the implementation that prevents the command-line argument from being applied due to incorrect variable scoping. I've also found a high-severity issue in how the default stop tokens are applied to requests, which would cause the feature to not work as intended in most cases and could lead to unintended side effects due to mutation of global state. I've provided suggestions to fix both issues.
| ) | ||
| args = parser.parse_args() | ||
|
|
||
| FORCED_STOP_TOKENS = args.default_stop if 'args' in locals() else [] |
There was a problem hiding this comment.
The assignment to FORCED_STOP_TOKENS creates a new local variable within the run_server_worker function, shadowing the global variable of the same name. This means the default stop tokens provided via the command-line will never be applied. To fix this, you need to use the global keyword to modify the module-level variable.
Additionally, the check if 'args' in locals() is redundant because args is defined on the preceding line and will always be in the local scope.
| FORCED_STOP_TOKENS = args.default_stop if 'args' in locals() else [] | |
| global FORCED_STOP_TOKENS | |
| FORCED_STOP_TOKENS = args.default_stop |
| if request.stop is None and len(FORCED_STOP_TOKENS) > 0: | ||
| request.stop = FORCED_STOP_TOKENS |
There was a problem hiding this comment.
There are two issues with this logic.
First, the condition request.stop is None will only be true if the client explicitly sends "stop": null. If the stop parameter is omitted in the request, request.stop will default to [] (an empty list), and this condition will not be met, causing the feature to not work for the most common use case. To apply the default stop tokens when none are provided by the user, you should check for a falsy value (which covers both None and []).
Second, you are assigning FORCED_STOP_TOKENS directly to request.stop. Since FORCED_STOP_TOKENS is a mutable list, any subsequent modification to request.stop elsewhere in the code could unintentionally alter the global FORCED_STOP_TOKENS list, leading to unpredictable behavior across different requests. You should assign a copy of the list instead.
| if request.stop is None and len(FORCED_STOP_TOKENS) > 0: | |
| request.stop = FORCED_STOP_TOKENS | |
| if not request.stop and FORCED_STOP_TOKENS: | |
| request.stop = FORCED_STOP_TOKENS.copy() |
|
👋 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. 🚀 |
|
@mhm0902 Can you elaborate more about why this PR is needed? Thanks! In the mean time, cc @aarnphm @chaunceyjiang |
2 participants
add default stop string
Purpose
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.