[Feature] Add auto-detection for reasoning_config when only reasoning_parser is set#38214
Conversation
|
Documentation preview: https://vllm--38214.org.readthedocs.build/en/38214/ |
There was a problem hiding this comment.
Code Review
This pull request introduces automatic initialization of reasoning boundary tokens (think_start_str, think_end_str) based on a specified reasoning parser. The ReasoningConfig now supports an optional reasoning_parser_name and can derive boundary tokens from it if not explicitly provided. The VLLMConfig logs a warning if this auto-initialization fails. Default reasoning configuration arguments are now set in arg_utils.py, and the documentation has been updated to reflect this new behavior. Additionally, existing tests for thinking_token_budget have been parameterized to cover both default and auto-configured server setups, and an assertion message was improved for clarity. There is no feedback to provide from the review comments.
|
Hi @chaunceyjiang, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
llsj14
left a comment
There was a problem hiding this comment.
I think this is a nice solution. Should we consider setting start_token and end_token to None in BasicReasoningParsers as well?
Right now, these fields are part of the interface, but some reasoning parsers don’t actually define or update them. This can potentially lead to assertion errors or even NotImplementedError in certain cases.
vllm/vllm/reasoning/basic_parsers.py
Lines 30 to 40 in 2e225f7
f8cf822 to
96bdf52
Compare
|
/cc @njhill Ready for review. |
sfeng33
left a comment
There was a problem hiding this comment.
One thing I noticed: with this change, reasoning_config can now be non-None even when auto-detection fails (i.e., enabled=False). This means the validation in vllm/v1/engine/input_processor.py (line 103) would pass through a request with thinking_token_budget set, but the ThinkingTokenBudgetLogitsProcessor would silently ignore it since is_enabled is False.
Would it make sense to update that check to something like:
if params.thinking_token_budget is not None and (
self.vllm_config.reasoning_config is None
or not self.vllm_config.reasoning_config.enabled
):
|
@DarkLight1337 All tests have passed. |
|
@claude review |
|
This pull request has merge conflicts that must be resolved before it can be |
…_parser is set Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
1e02c70 to
ead0fcd
Compare
…_parser is set Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…_parser is set Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…_parser is set (vllm-project#38214) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…_parser is set (vllm-project#38214) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…_parser is set (vllm-project#38214) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…_parser is set (vllm-project#38214) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…_parser is set (vllm-project#38214) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…_parser is set (vllm-project#38214) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
…_parser is set (vllm-project#38214) Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com> Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Purpose
Add auto-detection for reasoning_config when only reasoning_parser is set
Test Plan
see e2e
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.