-
Notifications
You must be signed in to change notification settings - Fork 651
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
Add provider-agnostic modelMaxThinkingTokens setting #1257
Conversation
|
CC @lupuletic |
@@ -1723,6 +1720,7 @@ export class ClineProvider implements vscode.WebviewViewProvider { | |||
this.updateGlobalState("requestyModelInfo", requestyModelInfo), | |||
this.updateGlobalState("modelTemperature", modelTemperature), | |||
this.updateGlobalState("modelMaxTokens", modelMaxTokens), | |||
this.updateGlobalState("anthropicThinking", modelMaxThinkingTokens), |
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.
Typographical Inconsistency: In updateApiConfiguration, the call to updateGlobalState uses the key "anthropicThinking" with the variable modelMaxThinkingTokens. Since the change intends to add a provider-agnostic 'modelMaxThinkingTokens' setting, consider renaming the key to "modelMaxThinkingTokens" for consistency.
this.updateGlobalState("anthropicThinking", modelMaxThinkingTokens), | |
this.updateGlobalState("modelMaxThinkingTokens", modelMaxThinkingTokens), |
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.
@ellipsis-dev - We don't want to change the way this setting is persisted or people will lose their settings.
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 current implementation correctly updates the global state key anthropicThinking
with the variable modelMaxThinkingTokens
. This approach ensures that user settings are preserved while implementing the new unified setting. Therefore, no changes are needed in response to the comment.
Responding to this comment by @cte. For more information about Ellipsis, check the documentation.
Description
I think it's going to be unwieldy if we split this setting for each provider, so let's unify it and give it a better name. Note that the storage key is not changed, so no one's settings will get wiped out.
Type of change
How Has This Been Tested?
Checklist:
Additional context
Related Issues
Reviewers
Important
Unified
modelMaxThinkingTokens
setting across providers, replacingvertexThinking
andanthropicThinking
.modelMaxThinkingTokens
setting replacesvertexThinking
andanthropicThinking
across the codebase.modelMaxThinkingTokens
is capped at 80% ofmodelMaxTokens
or a minimum of 1024 tokens.VertexHandler
,AnthropicHandler
, andOpenRouterHandler
to usemodelMaxThinkingTokens
.vertexThinking
andanthropicThinking
fromClineProvider.ts
andglobalState.ts
.ThinkingBudget.tsx
to usemodelMaxThinkingTokens
.vertex.test.ts
,checkExistApiConfig.test.ts
,ApiOptions.test.tsx
, andThinkingBudget.test.tsx
to reflect the new setting.vertexThinking
andanthropicThinking
.This description was created by
for 8cbce2d. It will automatically update as commits are pushed.