Skip to content

Conversation

@hustxiayang
Copy link
Contributor

@hustxiayang hustxiayang commented Oct 29, 2025

Description
thinking_config among anthropic and gemini models are similar, for example: thinking and thinking_config; budget_tokens and thinkingBudget. Our users do not need to set up different thinking configs for different models as we can provide a unified interface among different providers.

Related Issues/PRs (if applicable)

Related to #1463

@hustxiayang hustxiayang requested a review from a team as a code owner October 29, 2025 13:37
@hustxiayang hustxiayang marked this pull request as draft October 29, 2025 13:37
Signed-off-by: yxia216 <[email protected]>
@hustxiayang hustxiayang force-pushed the unified_thinking_config branch from 3968526 to 0ebf149 Compare October 29, 2025 14:04
@hustxiayang hustxiayang changed the title Unified thinking config among different providers feat: unified thinking config among different providers Oct 29, 2025
Signed-off-by: yxia216 <[email protected]>
@hustxiayang hustxiayang force-pushed the unified_thinking_config branch from 0ebf149 to cf3362d Compare October 29, 2025 14:22
@hustxiayang hustxiayang changed the title feat: unified thinking config among different providers feat: provide unified thinking config among different providers Oct 29, 2025
@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2025

Codecov Report

❌ Patch coverage is 50.72464% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.48%. Comparing base (f547bad) to head (3950be7).

Files with missing lines Patch % Lines
internal/apischema/openai/openai.go 0.00% 25 Missing ⚠️
internal/extproc/translator/openai_gcpvertexai.go 64.28% 3 Missing and 2 partials ⚠️
internal/extproc/translator/openai_awsbedrock.go 87.50% 1 Missing and 1 partial ⚠️
internal/extproc/translator/openai_gcpanthropic.go 85.71% 1 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (50.72%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1461      +/-   ##
==========================================
- Coverage   83.67%   83.48%   -0.20%     
==========================================
  Files         138      138              
  Lines       12268    12326      +58     
==========================================
+ Hits        10265    10290      +25     
- Misses       1395     1426      +31     
- Partials      608      610       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hustxiayang
Copy link
Contributor Author

/retest

Signed-off-by: yxia216 <[email protected]>
@hustxiayang hustxiayang marked this pull request as ready for review October 29, 2025 15:33
@mathetake
Copy link
Member

so if i understand correctly, this goes completely beyond the original @sukumargaonkar 's proposal https://github.com/envoyproxy/ai-gateway/blob/main/docs/proposals/004-vendor-specific-fields/proposal.md

This is trying to define "AI Gateway Schema", and the scope and impact is huge. Could you write up a design doc and have more comments from community here?

At the very least, I would request for very detailed documents and future planning on this direction as well as this kind of thing requires user facing documentation in the website, not just code. sg?

@mathetake
Copy link
Member

Please note that breaking change regarding this is extremely impactful than the control plane API. Once this is exposed, you cannot assume that clients will migrate properly, so we have to be very careful

@hustxiayang
Copy link
Contributor Author

@mathetake Sure, thanks a lot for the comments! One key motivation is that we do not want to break the existing codes internally. Would prepare a doc also. Thanks!

@yuzisun
Copy link
Contributor

yuzisun commented Oct 29, 2025

so if i understand correctly, this goes completely beyond the original @sukumargaonkar 's proposal https://github.com/envoyproxy/ai-gateway/blob/main/docs/proposals/004-vendor-specific-fields/proposal.md

This is trying to define "AI Gateway Schema", and the scope and impact is huge. Could you write up a design doc and have more comments from community here?

At the very least, I would request for very detailed documents and future planning on this direction as well as this kind of thing requires user facing documentation in the website, not just code. sg?

@mathetake It is not against @sukumargaonkar's original proposal for vendor specific fields. However for thinking we found out that different providers have very similar looking definitions, so it is worth unifying at the gateway level. @hustxiayang will provider a detailed writeup.

@yuzisun
Copy link
Contributor

yuzisun commented Oct 29, 2025

Please note that breaking change regarding this is extremely impactful than the control plane API. Once this is exposed, you cannot assume that clients will migrate properly, so we have to be very careful

agreed, I think we can still keep the GCP thinking vendor field but recommend the new unified thinking API shape.

@mathetake
Copy link
Member

ah sorry i should clarify that my comment about breaking change is about this new "unified api portion", not about vendor stuff already in place, and that's why I am saying we should not take this addition lightly without any very careful consideration.

@mathetake
Copy link
Member

mathetake commented Oct 29, 2025

basically API added here in this PRs will never be reverted without very long migration period (think about openai/anthropic, or any other public API's deprecation window)

@yuzisun
Copy link
Contributor

yuzisun commented Oct 29, 2025

basically API added here in this PRs will never be reverted without very long migration period (think about openai/anthropic, or any other public API's deprecation window)

Adding an agenda tomorrow to discuss the deprecation policy.

@nacx
Copy link
Contributor

nacx commented Oct 29, 2025

Premature abstractions can come at a significant maintenance cost, so we need proper, informed decisions before pushing for them.

thinking_config among different providers are similar, for example: thinking and thinking_config; budget_tokens and thinkingBudget. Our users do not need to set up different thinking configs for different models as we can provide a unified interface among different providers.

This PR description is quite vague and doesn't give a good understanding on what will be covered in reality and the real user benefits. Please edit the description and add a detailed breakdown of the providers that are supported today and how their APIs look like. Let's include all all the currently supported providers, so we can get a good understanding on what this change would really cover (because not all of them support the same features through the OpenAI comapt APIs. For example, Cohere does not support the reasoning_effort field).

That would help. lot understand what this really covers and what it's optimizing for.

@hustxiayang hustxiayang changed the title feat: provide unified thinking config among different providers feat: provide unified thinking config among anthropic and gemini models Oct 29, 2025
@mathetake
Copy link
Member

yeah so i won't be able to make it to the meeting tomorrow (sorry 🙏) but as long as the deprecation policy is agreed upon in a sane way as well as the API design is reviewed by multiple people given the prior art research (not limited to litellm), then all good to me.

of course the user facing documentation is a must but that's after the agreement we can do that

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.

6 participants