[Reasoning] Add glm47 reasoning parser for GLM-4.7 models#33349
[Reasoning] Add glm47 reasoning parser for GLM-4.7 models#33349QwertyJack wants to merge 1 commit intovllm-project:mainfrom
Conversation
GLM-4.7 models have a different chat template than GLM-4.5/4.6 models: - GLM-4.5/4.6: <think> is NOT in the generation prompt - GLM-4.7: <think> IS included in the generation prompt This means GLM-4.7 should use DeepSeekR1ReasoningParser instead of DeepSeekV3ReasoningWithThinkingParser used by GLM-4.5/4.6. Changes: - Add glm47 reasoning parser registration - Add tests for glm47 reasoning parser - Update documentation Fixes vllm-project#33348 Signed-off-by: QwertyJack <7554089+QwertyJack@users.noreply.github.com>
|
Documentation preview: https://vllm--33349.org.readthedocs.build/en/33349/ |
There was a problem hiding this comment.
Code Review
This pull request adds support for GLM-4.7 reasoning models by aliasing the glm47 parser to the existing DeepSeekR1ReasoningParser. The changes are well-structured, including updates to documentation, registration of the new parser, and a comprehensive new test suite. The approach of reusing an existing parser is sound given the model's behavior.
My main feedback is regarding the new test file, which introduces significant code duplication. While the tests themselves are thorough, refactoring them to be more reusable would greatly improve the long-term maintainability of the test suite. I've left a specific comment with a suggestion on how to achieve this.
|
@zRzRzRzRzRzRzR PTAL |
chaunceyjiang
left a comment
There was a problem hiding this comment.
Have you tested --reasoning-parser=glm45?
|
After further investigation, I found that:
Given that the recent refactoring has unified the parser, I can see the argument that a separate However, having
@chaunceyjiang What do you think? Should we keep the |
+1 |
|
Closing this PR based on feedback from @chaunceyjiang. After investigation, we found that:
A separate Thanks for the review! |
Summary
GLM-4.7 models have a different chat template than GLM-4.5/4.6 models:
<think>is NOT included in the generation prompt<think>IS included in the generation promptThis means GLM-4.7 should use
DeepSeekR1ReasoningParserinstead ofDeepSeekV3ReasoningWithThinkingParserused by GLM-4.5/4.6.Changes
glm47reasoning parser registrationglm47reasoning parserTest plan
glm47reasoning parserglm45tests still passFixes #33348