fix(model): apply thinkingBudget to OpenAI-compatible API request#1727
fix(model): apply thinkingBudget to OpenAI-compatible API request#1727lizhihao688 wants to merge 1 commit into
Conversation
fc2b0af to
396e88d
Compare
GenerateOptions.thinkingBudget was stored in the options object but never mapped to OpenAIRequest, causing the thinking_budget parameter to be silently dropped from all OpenAI-compatible API calls. This breaks reasoning/thinking models (e.g. Qwen3, DeepSeek-R1) that use thinking_budget to limit internal reasoning tokens: without the cap, the model exhausts all max_completion_tokens on reasoning_content and returns an empty content field. Changes: - Add thinking_budget field to OpenAIRequest DTO with @JsonProperty - Map GenerateOptions.thinkingBudget in OpenAIChatFormatter.applyOptions() - Add unit tests covering direct options, default fallback, override, and absent cases Fixes agentscope-ai#1697
396e88d to
866a7c5
Compare
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This is a clean, well-scoped bug fix that correctly addresses the missing thinkingBudget → OpenAIRequest mapping in OpenAIChatFormatter.applyOptions(). The implementation follows the exact same pattern used by all other option fields (temperature, reasoningEffort, topP, etc.), ensuring consistency. The new thinking_budget JSON property on OpenAIRequest is properly guarded by the class-level @JsonInclude(NON_NULL), so it will not leak into requests for providers that don't support it. The four unit tests cover the essential cases (direct, default-fallback, override, absent) and mirror the test structure used for other options. The Javadoc on the new field is informative and cites a concrete provider example. No logic, concurrency, or serialization issues found.
| request.setReasoningEffort(reasoningEffort); | ||
| } | ||
|
|
||
| // Apply thinking budget |
There was a problem hiding this comment.
[praise] Clean, minimal addition that follows the established apply-options pattern exactly. The null-guard and getOptionOrDefault usage are consistent with every other field in this method — easy to review and low risk.
| * reasoning, leaving the remainder of {@code max_completion_tokens} for the | ||
| * visible response. | ||
| */ | ||
| @JsonProperty("thinking_budget") |
There was a problem hiding this comment.
[praise] Good Javadoc — citing a concrete provider (DashScope Qwen3) gives future maintainers the context they need to understand why this non-standard field exists on an OpenAI-compatible DTO.
|
|
||
| GenerateOptions options = GenerateOptions.builder().temperature(0.7).build(); | ||
|
|
||
| formatter.applyOptions(request, options, null); |
There was a problem hiding this comment.
[praise] Solid test coverage for a single-field addition. The four cases (direct set, default fallback, override, absent) mirror the pattern used by other option test groups and give confidence that getOptionOrDefault semantics are correct.
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This is a clean, well-scoped bug fix that correctly addresses the missing thinkingBudget → OpenAIRequest mapping in OpenAIChatFormatter.applyOptions(). The implementation follows the exact same pattern used by all other option fields (temperature, reasoningEffort, topP, etc.), ensuring consistency. The new thinking_budget JSON property on OpenAIRequest is properly guarded by the class-level @JsonInclude(NON_NULL), so it will not leak into requests for providers that don't support it. The four unit tests cover the essential cases (direct, default-fallback, override, absent) and mirror the test structure used for other options. The Javadoc on the new field is informative and cites a concrete provider example. No logic, concurrency, or serialization issues found.
| request.setReasoningEffort(reasoningEffort); | ||
| } | ||
|
|
||
| // Apply thinking budget |
There was a problem hiding this comment.
[praise] Clean, minimal addition that follows the established apply-options pattern exactly. The null-guard and getOptionOrDefault usage are consistent with every other field in this method — easy to review and low risk.
| * reasoning, leaving the remainder of {@code max_completion_tokens} for the | ||
| * visible response. | ||
| */ | ||
| @JsonProperty("thinking_budget") |
There was a problem hiding this comment.
[praise] Good Javadoc — citing a concrete provider (DashScope Qwen3) gives future maintainers the context they need to understand why this non-standard field exists on an OpenAI-compatible DTO.
|
|
||
| GenerateOptions options = GenerateOptions.builder().temperature(0.7).build(); | ||
|
|
||
| formatter.applyOptions(request, options, null); |
There was a problem hiding this comment.
[praise] Solid test coverage for a single-field addition. The four cases (direct set, default fallback, override, absent) mirror the pattern used by other option test groups and give confidence that getOptionOrDefault semantics are correct.
Summary
GenerateOptions.thinkingBudgetwas stored in the options object but never mapped toOpenAIRequest, causing thethinking_budgetparameter to be silently dropped from all OpenAI-compatible API calls.Root Cause
In
OpenAIChatFormatter.applyOptions(), all otherGenerateOptionsfields were correctly mapped toOpenAIRequestsetters — butthinkingBudgetwas missing. Additionally,OpenAIRequesthad nothinking_budgetfield at all.temperature→request.setTemperature()reasoningEffort→request.setReasoningEffort()maxCompletionTokens→request.setMaxCompletionTokens()thinkingBudget→ NOT HANDLED — silently droppedThis breaks reasoning/thinking models (e.g. Qwen3, DeepSeek-R1): without the cap, the model exhausts all
max_completion_tokensonreasoning_contentand returns an emptycontentfield.Changes
OpenAIRequest.java: Add@JsonProperty("thinking_budget")field with getter/setter and Builder methodOpenAIChatFormatter.java: MapGenerateOptions.thinkingBudgetinapplyOptions()OpenAIChatFormatterTest.java: AddThinkingBudgetTestswith 4 unit tests covering: direct options, default fallback, override, and absent casesTesting
All 948 existing unit tests pass.
spotless:checkpasses.Before / After
Before —
thinkingBudgetsilently ignored:After —
thinking_budgetcorrectly sent:Fixes #1697