[responsesAPI] parser.extract_response_outputs can take in token IDs#37130
[responsesAPI] parser.extract_response_outputs can take in token IDs#37130chaunceyjiang merged 2 commits intovllm-project:mainfrom
Conversation
Signed-off-by: Andrew Xia <axia@meta.com>
![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 updates the extract_response_outputs method in the parser to accept token IDs in addition to the model output string. However, the implementation in DelegatingParser does not yet utilize these token IDs for parsing, which is a critical oversight. My review includes a comment to address this.
vllm/parser/abstract_parser.py
Outdated
| self, | ||
| *, | ||
| model_output: str, | ||
| model_output_token_ids: list[int], |
There was a problem hiding this comment.
The model_output_token_ids parameter is introduced in the method signature but remains unused within the implementation. The parsing logic, including self.extract_reasoning and self._parse_tool_calls, continues to rely solely on the string-based model_output. To enable more robust token-based parsing, this parameter should be utilized by the underlying parsing methods.
qandrew
requested review from
DarkLight1337,
aarnphm,
chaunceyjiang and
russellb
as code owners
|
cc @chaunceyjiang @sfeng33 please take a look :) |
|
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
|
qandrew
changed the title
chaunceyjiang
left a comment
chaunceyjiang
left a comment
There was a problem hiding this comment.
LGTM
/cc @sfeng33
|
thanks, @chaunceyjiang would you mind approving / applying "ready" tag? |
chaunceyjiang
added
the
ready
…llm-project#37130) Signed-off-by: Andrew Xia <axia@meta.com>
…llm-project#37130) Signed-off-by: Andrew Xia <axia@meta.com>
…llm-project#37130) Signed-off-by: Andrew Xia <axia@meta.com>
…llm-project#37130) Signed-off-by: Andrew Xia <axia@meta.com>
…llm-project#37130) Signed-off-by: Andrew Xia <axia@meta.com> Signed-off-by: Monishver Chandrasekaran <monishverchandrasekaran@gmail.com>
…llm-project#37130) Signed-off-by: Andrew Xia <axia@meta.com>
…llm-project#37130) Signed-off-by: Andrew Xia <axia@meta.com> Signed-off-by: Vinay Damodaran <vrdn@hey.com>
…llm-project#37130) Signed-off-by: Andrew Xia <axia@meta.com> Signed-off-by: EricccYang <yangyang4991@gmail.com>
3 participants
Purpose
This will be needed when we move harmony into parser (https://github.com/vllm-project/vllm/blob/main/vllm/entrypoints/openai/responses/context.py#L865), or any other model that wishes to parse based on token_id instead of str.
See #32713 for more context
Test Plan
Test Result
no functional changes in this PR