-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add provider-agnostic modelMaxThinkingTokens setting #1257
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
|
|
CC @lupuletic |
| 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
modelMaxThinkingTokenssetting across providers, replacingvertexThinkingandanthropicThinking.modelMaxThinkingTokenssetting replacesvertexThinkingandanthropicThinkingacross the codebase.modelMaxThinkingTokensis capped at 80% ofmodelMaxTokensor a minimum of 1024 tokens.VertexHandler,AnthropicHandler, andOpenRouterHandlerto usemodelMaxThinkingTokens.vertexThinkingandanthropicThinkingfromClineProvider.tsandglobalState.ts.ThinkingBudget.tsxto usemodelMaxThinkingTokens.vertex.test.ts,checkExistApiConfig.test.ts,ApiOptions.test.tsx, andThinkingBudget.test.tsxto reflect the new setting.vertexThinkingandanthropicThinking.This description was created by
for 8cbce2d. It will automatically update as commits are pushed.