-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Updating OpenAI reasoning parameters for gpt-5.2 #2637
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
base: main
Are you sure you want to change the base?
Changes from all commits
7156b94
7d846a4
fcb9541
12bdf42
982f5f4
f88162b
90eda73
5df45ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -23,7 +23,7 @@ import { ChatCompletion, FinishedCompletionReason, TokenLogProb } from '../../ne | |||||
| import { IExperimentationService } from '../../telemetry/common/nullExperimentationService'; | ||||||
| import { ITelemetryService } from '../../telemetry/common/telemetry'; | ||||||
| import { TelemetryData } from '../../telemetry/common/telemetryData'; | ||||||
| import { getVerbosityForModelSync } from '../common/chatModelCapabilities'; | ||||||
| import { getVerbosityForModelSync, isHiddenModelB } from '../common/chatModelCapabilities'; | ||||||
| import { getStatefulMarkerAndIndex } from '../common/statefulMarkerContainer'; | ||||||
| import { rawPartAsThinkingData } from '../common/thinkingDataContainer'; | ||||||
|
|
||||||
|
|
@@ -57,8 +57,12 @@ export function createResponsesRequestBody(accessor: ServicesAccessor, options: | |||||
| 'disabled'; | ||||||
| const effortConfig = configService.getExperimentBasedConfig(ConfigKey.ResponsesApiReasoningEffort, expService); | ||||||
| const summaryConfig = configService.getExperimentBasedConfig(ConfigKey.ResponsesApiReasoningSummary, expService); | ||||||
| const effort = effortConfig === 'default' ? 'medium' : effortConfig; | ||||||
| const summary = summaryConfig === 'off' ? undefined : summaryConfig; | ||||||
| let effort = effortConfig === 'default' ? 'medium' : effortConfig; | ||||||
| let summary = summaryConfig === 'off' ? 'detailed' : summaryConfig; | ||||||
| const reasoningParams = reasoningParameterValuesBasedOnModel(endpoint.family, effort, summary); | ||||||
| effort = reasoningParams?.effort || effort; | ||||||
| summary = reasoningParams?.summary || summary; | ||||||
|
|
||||||
| if (effort || summary) { | ||||||
| body.reasoning = { | ||||||
| ...(effort ? { effort } : {}), | ||||||
|
|
@@ -71,6 +75,20 @@ export function createResponsesRequestBody(accessor: ServicesAccessor, options: | |||||
| return body; | ||||||
| } | ||||||
|
|
||||||
| // for gpt-5.2 + models, changing the default reasoning parameters | ||||||
|
||||||
| // for gpt-5.2 + models, changing the default reasoning parameters | |
| // Adjust reasoning parameter defaults for specific model families (hidden model B) |
Copilot
AI
Dec 18, 2025
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.
The type definition for ResponsesReasoningSummary is missing the 'off' option that is present in the configuration type ConfigKey.ResponsesApiReasoningSummary. The config type is 'off' | 'concise' | 'detailed' but the local type only includes 'concise' | 'detailed'.
While 'off' is handled specially in the code (converting to 'detailed'), the type should accurately reflect all possible config values to maintain type safety and code clarity. Consider adding 'off' to the type definition or creating a separate type that represents the normalized values after processing the config.
| type ResponsesReasoningSummary = 'concise' | 'detailed'; | |
| type ResponsesReasoningSummary = 'off' | 'concise' | 'detailed'; |
Copilot
AI
Dec 18, 2025
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.
The function reasoningParameterValuesBasedOnModel always overrides the summary parameter to 'concise' for hidden model B, regardless of the input summary value. However, it returns the effort parameter unchanged. This seems inconsistent - if the function is meant to override defaults for gpt-5.2+, why is only summary being overridden?
Additionally, the function accepts a summary parameter but ignores it when returning, making the parameter misleading. Consider either:
- Removing the
summaryparameter if it's not used - Or using it in some conditional logic if there's a specific reason to accept it
Copilot
AI
Dec 18, 2025
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.
There are two consecutive blank lines (lines 90-91) which is inconsistent with the rest of the codebase style. According to the coding standards, there should typically be only one blank line between functions.
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.
The logic change on line 61 modifies the behavior when
summaryConfig === 'off'. Previously, this would result insummary = undefined, but now it defaults to'detailed'. This changes the existing behavior: when users explicitly set the config to 'off', they likely expect reasoning summaries to be disabled, not to use 'detailed'.Consider either: