-
Notifications
You must be signed in to change notification settings - Fork 256
feat: reasoning model controller #56
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
feat: reasoning model controller #56
Conversation
👥 vLLM Semantic Team NotificationThe following members have been identified for the changed files in this PR and have been automatically assigned: 📁
|
✅ Deploy Preview for vllm-semantic-router ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@tao12345666333 thanks for starting up this! I am also working on the benchmark script to make it easier to use. So when you are testing the reasoning controller, please verify it with a reasoning model. We are also going to add an e2e CI to test this out. |
|
@rootfs Glad to hear that, I'll verify it this way. Thank you! |
|
Cool, welcome @tao12345666333 :) |
|
@tao12345666333 thanks for the prompt action! Would you please rebase? |
| } | ||
|
|
||
| // getModelFamilyAndTemplateParam returns a normalized model family name and the template param to be used (if any) | ||
| func getModelFamilyAndTemplateParam(model string) (string, string) { |
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.
can you add a unit test for this? you can do it in a follow up PR.
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.
Sure
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.
I think we should add an UT in this PR, let us make sure it does not break the strategy for modifying the body.
| ) | ||
|
|
||
| // ReasoningDecisions tracks the reasoning mode decision outcome by category, model, and effort | ||
| ReasoningDecisions = promauto.NewCounterVec( |
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.
please come up with a follow up PR to add them to the doc
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 an issue for tracking #62
| if strings.Contains(lower, "qwen3") { | ||
| return "qwen3", "enable_thinking" | ||
| } | ||
| if strings.Contains(lower, "deepseek") || strings.Contains(lower, "ds") { |
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.
ds is too broad to me. Maybe we should also add a config validation to detect if there are conflict or duplicate model names with ds in them but not actually deepseek..
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.
would you add an issue and follow up with this in the next PR for more robust model name filter?
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.
I write "ds" here because many people (including on social media) refer to DeepSeek as "ds" and Claude Code as "cc." 🤣
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.
I created an issue for tracking #61
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.
my setup modelname for deepseek-v3 is also ds-v3 😄
Signed-off-by: Jintao Zhang <[email protected]>
7496067 to
5f6fd67
Compare
Signed-off-by: Jintao Zhang <[email protected]>
5f6fd67 to
467f36a
Compare
|
@tao12345666333 for the reasoning e2e test on CI github action, do you like to give it a try? You can start by creating the script to setup the env and verification process and we'll use your work in github action. Thanks |
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.
plz add UT in a follow-up
| } | ||
|
|
||
| // getModelFamilyAndTemplateParam returns a normalized model family name and the template param to be used (if any) | ||
| func getModelFamilyAndTemplateParam(model string) (string, string) { |
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.
I think we should add an UT in this PR, let us make sure it does not break the strategy for modifying the body.
| if strings.Contains(lower, "qwen3") { | ||
| return "qwen3", "enable_thinking" | ||
| } | ||
| if strings.Contains(lower, "deepseek") || strings.Contains(lower, "ds") { |
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.
my setup modelname for deepseek-v3 is also ds-v3 😄
@rootfs Sure. Please assign an issue to me. I'm happy to give it a try |
Sure. #56 (comment) I will submit the follow-up PR soon. |
Great! please create an issue and we'll assign it to you |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #33
Release Notes: Yes/No