Reduce validation to a warning #28749
Conversation
alecsolder
requested review from
ProExpertProg,
WoosukKwon,
hmellor,
houseroad,
mgoin,
robertgshaw2-redhat,
tlrmchlsmth,
yewentao256 and
youkaichao
as code owners
![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 correctly relaxes a validation check for reasoning_parser by changing a ValueError to a logger.warning. This allows for programmatic registration of reasoning parsers after the initial configuration, which was the intended goal. The change is well-contained and addresses the issue described. I have one suggestion to improve the clarity of the new warning message to avoid potential confusion for users.
vllm/config/structured_outputs.py
Outdated
| "Reasoning parser %s not defined in reasoning_parser_plugin " | ||
| "argument. Assuming it is registered to the " | ||
| "ReasoningParserManager programmatically.", |
There was a problem hiding this comment.
The current warning message is slightly misleading. It suggests the issue is with reasoning_parser_plugin, but the validation checks against all registered parsers, including built-in ones. This could confuse users who are not using a plugin but have, for example, misspelled a built-in parser name. A more general warning message would be clearer and help prevent user confusion during debugging.
| "Reasoning parser %s not defined in reasoning_parser_plugin " | |
| "argument. Assuming it is registered to the " | |
| "ReasoningParserManager programmatically.", | |
| "Reasoning parser %s not found among registered parsers. " | |
| "Assuming it will be registered programmatically later.", |
There was a problem hiding this comment.
I can make it more clear
Instead of reduce the validation from error to warning, should we delay the validation logic instead? |
vllm/config/structured_outputs.py
Outdated
| f"invalid reasoning parser: {self.reasoning_parser} " | ||
| f"(chose from {{ {','.join(valid_reasoning_parsers)} }})" | ||
| logger.warning( | ||
| "Reasoning parser %s not defined in reasoning_parser_plugin " |
There was a problem hiding this comment.
i don't think we should directly assume it exists? should we explicitly skip the check for reasoning parser plugin? any way for us to directly make valid_reasoning_parsers aware of plugin registration?
|
@Jialin @yeqcharlotte There is already a check when creating the api server, the early check here is an additional one. There is an additional problem that #28075 causes reasoning parsers to be registered twice, so this PR is just a fast-fix to un-break directly registered reasoning parsers. |
IC. If that's case, how about just get rid of the validation here? |
I think a warning is reasonable to help the vast majority of use cases, getting an early warning signal about a reasoning parser needing to be directly registered could definitely be useful IMO |
why would the reasoning parsers get registered twice? can that be fixed? |
|
@walterbm Does it make sense to remove the registration + validation from here and instead have it be in StructuredOutputManager? It seems we're doing it in quite a few places now |
yeah that makes sense to me. helps reduce some of the duplication around validation |
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Alec Solder <alecs@fb.com>
Signed-off-by: Alec Solder <alecs@fb.com>
…e as it occurs elsewhere Signed-off-by: Alec Solder <alecs@fb.com>
Signed-off-by: Alec Solder <alecs@fb.com>
alecsolder
force-pushed
the
remove-early-reasoning-parser-validation
branch
from
0a6ed23 to
0c744dd
Compare
4 participants
Purpose
Fix for issue introduced in 28075
The reasoning parser validation introduced on the structured_outputs_config argument is done very early and does not allow for other programmatic methods for registering reasoning parsers.
By only allowing registration of external reasoning parsers via
reasoning-parser-pluginit would require bundling of extra python files when deploying vLLM, which is not an ideal experience.Test Plan
External registration of plugins with ReasoningParserManager
Test Result
External registration of plugins with ReasoningParserManager works like before