Skip to content

Conversation

@rootfs
Copy link
Collaborator

@rootfs rootfs commented Sep 8, 2025

What type of PR is this?

If the model doesn't support reasoning, don't set the reasoning effort in API call.

What this PR does / why we need it:
This is a regression when reasoning mode support is added
Which issue(s) this PR fixes:

Fixes #

Release Notes: Yes/No

@github-actions
Copy link

github-actions bot commented Sep 8, 2025

👥 vLLM Semantic Team Notification

The following members have been identified for the changed files in this PR and have been automatically assigned:

📁 config

Owners: @rootfs
Files changed:

  • config/config.yaml

📁 src

Owners: @rootfs, @Xunzhuo, @wangchen615
Files changed:

  • src/semantic-router/pkg/config/config.go
  • src/semantic-router/pkg/extproc/reason_mode_config_test.go
  • src/semantic-router/pkg/extproc/reason_mode_selector.go
  • src/semantic-router/pkg/extproc/reason_mode_selector_test.go
  • src/semantic-router/pkg/extproc/reasoning_integration_test.go

📁 website

Owners: @Xunzhuo
Files changed:

  • website/docs/getting-started/configuration.md

vLLM

🎉 Thanks for your contributions!

This comment was automatically generated based on the OWNER files in the repository.

@netlify
Copy link

netlify bot commented Sep 8, 2025

Deploy Preview for vllm-semantic-router ready!

Name Link
🔨 Latest commit 74af935
🔍 Latest deploy log https://app.netlify.com/projects/vllm-semantic-router/deploys/68c023440638380008a96dac
😎 Deploy Preview https://deploy-preview-97--vllm-semantic-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@rootfs
Copy link
Collaborator Author

rootfs commented Sep 8, 2025

cc @tao12345666333

@rootfs rootfs requested a review from Copilot September 8, 2025 22:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a regression introduced when reasoning mode support was added, specifically ensuring that reasoning effort is not set for models that don't support reasoning features.

  • Refactored the reasoning mode implementation from hardcoded model family detection to a configuration-driven approach
  • Added model reasoning configurations to define which models support reasoning and their specific syntax requirements
  • Updated test coverage to verify that unknown/unsupported models don't receive reasoning fields

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
config/config.yaml Adds model reasoning configurations defining supported models and their reasoning syntax
src/semantic-router/pkg/config/config.go Implements config types and pattern matching logic for model reasoning configurations
src/semantic-router/pkg/extproc/reason_mode_selector.go Refactors reasoning logic to use config-driven approach instead of hardcoded model detection
src/semantic-router/pkg/extproc/reason_mode_selector_test.go Updates tests to use new config-driven approach and adds comprehensive test coverage
src/semantic-router/pkg/extproc/reasoning_integration_test.go Updates integration tests to verify proper behavior for unknown models and config-driven reasoning

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Signed-off-by: Huamin Chen <[email protected]>
Signed-off-by: Huamin Chen <[email protected]>
@Xunzhuo
Copy link
Member

Xunzhuo commented Sep 9, 2025

let us stop the cycles round reasoning string match、pattern etc works for a better choice. I think we should have better approach to take care of the auto reasoning logics, the modelname can be very dynamic since that is set in vllm startup options, like i can set dsv31 as the modelname for deepseek-v3.1.

we should have the fixed groups/family names for models like we have supported:

  1. deepseek
  2. qwen
  3. gpt-oss
    theses three vars should be fixed and the model name just is an alias around it. so maybe we should have an extra params for modelconfig like reasoning-familiy:

deepseek:

model_config:
  ds-v31-balabala:
    reasoning-family: deepseek
......
    preferred_endpoints:
    - endpoint1

gpt-oss:

model_config:
  gptoss-balabala:
    reasoning-family: gpt-oss
......
    preferred_endpoints:
    - endpoint1

qwen

model_config:
  qwen3-balabala:
    reasoning-family: qwen3
......
    preferred_endpoints:
    - endpoint1

the modelname can be very flexible, like for deepseek family, we can set (ds-v3,self-hosted/ds,official/deepseek, anything we want, since that is used in modelname in request body) we just care about what the model reasoning family is. if it is empty, we just think it is not supported hybrid reasoning.

@rootfs rootfs requested a review from liangyuanpeng September 9, 2025 12:06
Signed-off-by: Huamin Chen <[email protected]>
@rootfs
Copy link
Collaborator Author

rootfs commented Sep 9, 2025

@Xunzhuo model family is a good idea. It is added to the new config now.

@rootfs
Copy link
Collaborator Author

rootfs commented Sep 9, 2025

review comments addressed, merging it so I can add other PRs using this new config.

@rootfs rootfs merged commit 446dfe6 into vllm-project:main Sep 9, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants