[1/N] Initial Implementation of Parser for ResponsesAPI#32712
[1/N] Initial Implementation of Parser for ResponsesAPI#32712chaunceyjiang merged 14 commits intovllm-project:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new abstraction layer for unifying reasoning and tool parsing, which is a significant architectural improvement. The Parser abstract class, DelegatingParser, and ParserManager provide a modular and extensible system for handling different parsing strategies. The integration into serving.py and responses/serving.py correctly utilizes this new abstraction. Type hints and docstrings are well-maintained, contributing to code clarity and maintainability.
Signed-off-by: Andrew Xia <axia@fb.com>
Signed-off-by: Andrew Xia <axia@fb.com>
Signed-off-by: Andrew Xia <axia@fb.com>
Signed-off-by: Andrew Xia <axia@fb.com>
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Andrew Xia <axia@fb.com>
Signed-off-by: Andrew Xia <axia@fb.com>
|
Hi @qandrew, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 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
| _WrappedParser.reasoning_parser_cls = reasoning_parser_cls | ||
| _WrappedParser.tool_parser_cls = tool_parser_cls | ||
|
|
||
| return _WrappedParser |
There was a problem hiding this comment.
Global class attribute mutation causes shared state bug
Medium Severity
The _get_parser method modifies class-level attributes on the globally-shared _WrappedParser class (reasoning_parser_cls and tool_parser_cls). Since _WrappedParser is imported from a module, these attributes are shared across all usages. If multiple OpenAIServingResponses instances are created with different parser configurations (e.g., in tests, multi-model deployments, or configuration changes), later instances will overwrite the class attributes, affecting all previous instances that reference _WrappedParser.
| ) | ||
| return parser | ||
| except KeyError: | ||
| pass |
There was a problem hiding this comment.
Strategy 2 silently overrides user's explicit parser choices
Medium Severity
Strategy 2 in _get_parser can silently override the user's explicit parser configuration. When a user specifies different parsers for tools and reasoning (e.g., --tool-call-parser hermes --reasoning-parser minimax_m2), Strategy 2 iterates through both names and returns the first unified parser found. If "minimax_m2" has a unified parser registered, the user's explicit "hermes" tool parser choice is silently ignored and MinimaxM2ToolParser is used instead. This contradicts the PR's stated intent of "No functional changes expected."
| reasoning_parser_cls=self.reasoning_parser, | ||
| reasoning_parser_cls=self.parser.reasoning_parser_cls | ||
| if self.parser | ||
| else None, |
There was a problem hiding this comment.
Missing null check causes crash with ParsableContext
Medium Severity
When VLLM_USE_EXPERIMENTAL_PARSER_CONTEXT=True, the code passes self.parser.reasoning_parser_cls to ParsableContext, but only checks if self.parser is not None. If a user configures only a tool parser (no reasoning parser), self.parser will be _WrappedParser with reasoning_parser_cls = None. This passes None to ParsableContext, which raises ValueError("reasoning_parser_cls must be provided.") at runtime. The check needs to verify that reasoning_parser_cls is also not None before using ParsableContext.
|
Hi @qandrew, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
|
Hi @qandrew, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
chaunceyjiang
left a comment
There was a problem hiding this comment.
Thank you. I will research this issue in the coming days.
Also worth mentioning is that over the past two days, while looking into #33215, I've noticed some limitations in the current design of the ReasoningParser.
It might require a refactoring.
vllm/parser/minimax_m2_parser.py
Outdated
| logger = init_logger(__name__) | ||
|
|
||
|
|
||
| @ParserManager.register_module("minimax_m2") |
There was a problem hiding this comment.
I don't really recommend adding another ParserManager.
Is it possible to have a self-contained mechanism to handle these tasks?
There was a problem hiding this comment.
i would ideally like to merge reasoning parser and tool calling parser managers into a single ParserManager, then we can deprecate ReasoningManager / ToolCallingManager. would you have thoughts on this?
i'd be curious on your thoughts for ReasoningParser refactor and seeing how we could put it inside Parser :) |
|
This pull request has merge conflicts that must be resolved before it can be |
I’m not quite sure why this needs to be placed in a parser. For some older models that don’t have reasoning, how are you planning to handle them? |
if there's no reasoning parser, there will be no parser, and the behavior will not be changed |
Signed-off-by: Andrew Xia <axia@fb.com>
|
Hi @qandrew, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
|
Hi @qandrew, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
There was a problem hiding this comment.
Thanks to @qandrew for the patience and for walking me through the design in detail.
I think it makes sense to unify the tool parser and reasoning.
For this PR, I’d suggest starting by introducing get_parser, class Parser, and class WrappedParser.
ParserManager and MiniMaxM2Parser can be introduced in a separate PR(ParserManager hasn’t been invoked yet.
).
| # SPDX-FileCopyrightText: Copyright contributors to the vLLM project | ||
|
|
||
| """ | ||
| MiniMax M2 Parser - A unified parser for MiniMax M2 models. |
There was a problem hiding this comment.
As we discussed offline, this is just an example. We can add it in a separate PR.
Signed-off-by: Andrew Xia <axia@fb.com>
|
Hi @qandrew, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
|
Hi @qandrew, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
Signed-off-by: Andrew Xia <axia@fb.com>
| except KeyError: | ||
| pass | ||
|
|
||
| # Strategy 3: Create a DelegatingParser with the individual parser classes |
There was a problem hiding this comment.
To be honest, personally I tend to keep only Strategy 3. I’d prefer to drop Strategy 1 and Strategy 2.
We can use a single Parser to unify the interface, but still keep the implementations of reasoning_parser and tool_parser separate. Basically, keep the current structure.
There was a problem hiding this comment.
This is also why I’d like you to remove vllm/parser/minimax_m2_parser.py.
There was a problem hiding this comment.
Given that this PR is fairly urgent for you right now, I’m okay with merging it first, and we can discuss this part afterward.
There was a problem hiding this comment.
sounds good, appreciate the help! let me follow up with cleaning up minimax_m2_parser. It might be more clear to motivate strategy 1 through GPTOSS harmony parser, or if we want to support models that do interleaved reasoning :)
i think i'd prefer to move reasoning_parser / tool_parser into parser, but happy to chat about that more
…#32712) Signed-off-by: Andrew Xia <axia@fb.com> Co-authored-by: Andrew Xia <axia@fb.com> Signed-off-by: felix01.yu <felix01.yu@vipshop.com>
…#32712) Signed-off-by: Andrew Xia <axia@fb.com> Co-authored-by: Andrew Xia <axia@fb.com>
…#32712) Signed-off-by: Andrew Xia <axia@fb.com> Co-authored-by: Andrew Xia <axia@fb.com>
Purpose
Initial Implementation of #32713. This PR introduces
ParserManager, with the first example being the MinimaxM2 parser.Future goals
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.