-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
Add think chunk #21333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add think chunk #21333
Conversation
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces support for a 'think' content chunk, which seems to be used for representing reasoning steps from models like Mistral. The changes involve updating chat utilities to recognize and parse this new content type, and modifying the DeepSeekR1ReasoningParser
to handle special tokens from Mistral's tokenizer.
I have raised a high-severity concern regarding the modification of DeepSeekR1ReasoningParser
. Adding Mistral-specific logic to a class named and registered for DeepSeek models is a design flaw that will impact maintainability and clarity. I've recommended creating a separate parser for Mistral to keep the components clean and decoupled.
b24960a
to
d284a34
Compare
1ad0d70
to
df19b71
Compare
tests/reasoning/utils.py
Outdated
@@ -56,29 +57,44 @@ def run_reasoning_extraction( | |||
|
|||
def run_reasoning_extraction_nonstreaming( | |||
reasoning_parser: ReasoningParser, | |||
model_output: list[str], | |||
model_output: Union[list[str], list[int]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added utils just for mistral so that it removes the Union
, @aarnphm might have another idea but at least this avoids changing previous code.
7150b0c
to
035817d
Compare
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
3d2af6c
to
e15343c
Compare
Signed-off-by: Julien Denize <[email protected]> Signed-off-by: 董巍 <[email protected]>
Signed-off-by: Julien Denize <[email protected]> Signed-off-by: avigny <[email protected]>
Signed-off-by: Julien Denize <[email protected]> Signed-off-by: shuw <[email protected]>
Signed-off-by: Julien Denize <[email protected]> Signed-off-by: x22x22 <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]>
Signed-off-by: Julien Denize <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
needs mistralai/mistral-common#122