-
Notifications
You must be signed in to change notification settings - Fork 6k
fix: use max_completion_tokens for Azure reasoning models #5541
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
Conversation
|
@rekram1-node Could you help to review this PR for #5421 |
|
/review |
|
|
||
| return fetchFn(input, { | ||
| ...opts, | ||
| // @ts-ignore see here: https://github.com/oven-sh/bun/issues/16682 |
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.
Suggestion: The nesting here could be reduced by using early returns/guards. This is a stylistic suggestion - the current code is functionally correct.
| // @ts-ignore see here: https://github.com/oven-sh/bun/issues/16682 | |
| // Reasoning models require max_completion_tokens instead of max_tokens | |
| // when using openai-compatible provider | |
| // Check if model has reasoning capability OR has reasoningEffort option | |
| const isReasoningModel = | |
| model.capabilities.reasoning || (model.options && "reasoningEffort" in model.options) | |
| const shouldTransformMaxTokens = | |
| model.api.npm === "@ai-sdk/azure" && | |
| isReasoningModel && | |
| opts.body && | |
| typeof opts.body === "string" | |
| if (shouldTransformMaxTokens) { | |
| try { | |
| const body = JSON.parse(opts.body) | |
| if (body.max_tokens !== undefined) { | |
| body.max_completion_tokens = body.max_tokens | |
| delete body.max_tokens | |
| opts.body = JSON.stringify(body) | |
| } | |
| } catch { | |
| // Ignore JSON parse errors | |
| } | |
| } |
This flattens one level of nesting by combining the conditions into a single check. Also note: using catch without a parameter (instead of catch (e)) is cleaner when you do not use the error.
| // Reasoning models require max_completion_tokens instead of max_tokens | ||
| // when using openai-compatible provider | ||
| // Check if model has reasoning capability OR has reasoningEffort option | ||
| const isReasoningModel = |
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.
only condition here should be:
model.capabilities.reasoning
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 have updated it, please help to review it 7b5a00e
|
@rekram1-node is anything wrong with the new patch 7b5a00e |
|
@junmediatek feel free to resolve any bot comments that you addressed or want to ignore |
|
Also, im not entirely sure if this is the perfect fix and I need to test something w/ it so in the meantime (without requiring code changes from us, you should be able to add this: import { Plugin } from "@opencode-ai/plugin"
export const AzurePatch: Plugin = async (ctx) => {
return {
auth: {
provider: "gaisf-azure",
loader: async (getAuth, provider) => {
return {
async fetch(input, init) {
const opts = init ?? {}
if (opts.body && typeof opts.body === "string") {
try {
const body = JSON.parse(opts.body)
if (body.max_tokens !== undefined) {
body.max_completion_tokens = body.max_tokens
delete body.max_tokens
opts.body = JSON.stringify(body)
}
} catch (e) {}
}
return fetch(input, {
...opts,
timeout: false,
})
},
}
},
},
}
}to And assuming your azure proxy provider is still called |
|
You can completely override fetch implementations for any provider via plugins and I just wanted to demonstrate that for you |
That is a good idea, I will try it. However, the standard solution is to modify the code, |
|
This issue can be resolved by the plugin, However, the standard solution is to modify the code, because the API reference in the openai API doc. |
|
This patch can be merged? |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Fixes Azure reasoning models (GPT-5, o1) by using
max_completion_tokensinstead ofmax_tokens.