-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: exclude undefined max token fields from settings export #7946
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
- Modified ProviderSettingsManager export method to exclude modelMaxTokens and modelMaxThinkingTokens when undefined - Prevents these fields from appearing in exported settings for models that do not support reasoning budgets - Added test to verify undefined fields are properly excluded from export Fixes #7944
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.
Reviewing my own code because apparently I trust no one, not even myself.
| // These fields should only be included for models that support reasoning budgets | ||
| // and when the user has explicitly set values | ||
| // Use type assertion to access these optional fields | ||
| const configAny = config as any |
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 use of as any here works, but have you considered a more type-safe approach? We could check if these properties exist in the discriminated union type or use a type guard. Something like:
| const configAny = config as any | |
| // Remove max token fields if they are undefined | |
| // These fields should only be included for models that support reasoning budgets | |
| // and when the user has explicitly set values | |
| if ('modelMaxTokens' in config && config.modelMaxTokens === undefined) { | |
| delete config.modelMaxTokens | |
| } | |
| if ('modelMaxThinkingTokens' in config && config.modelMaxThinkingTokens === undefined) { | |
| delete config.modelMaxThinkingTokens | |
| } |
| delete configAny.modelMaxTokens | ||
| } | ||
| if (configAny.modelMaxThinkingTokens === undefined) { | ||
| delete configAny.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.
Could we extract this logic into a helper function to avoid duplication and improve maintainability? For example:
function removeUndefinedTokenFields(config: any): void {
if (config.modelMaxTokens === undefined) {
delete config.modelMaxTokens
}
if (config.modelMaxThinkingTokens === undefined) {
delete config.modelMaxThinkingTokens
}
}Then just call removeUndefinedTokenFields(config) here. This would make it easier to add more fields in the future if needed.
| const reasoningProvider = exportedData.providerProfiles.apiConfigs["reasoning-provider"] | ||
| expect(reasoningProvider.modelMaxTokens).toBe(8192) // Should be present with value | ||
| expect(reasoningProvider.modelMaxThinkingTokens).toBe(4096) // Should be present with value | ||
| }) |
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.
Excellent test coverage! The test thoroughly covers all scenarios: completely undefined fields, partially defined fields, and fully defined fields. This gives confidence that the fix works correctly.
|
Maybe we should check for Closing for now. |
Description
This PR fixes an issue where exported settings incorrectly included
modelMaxTokensandmodelMaxThinkingTokensfields for models that don't support reasoning budgets.Problem
undefinedin the exported JSON, creating misleading/invalid configurationSolution
ProviderSettingsManager.export()method to excludemodelMaxTokensandmodelMaxThinkingTokensfields when they are undefinedTesting
Fixes #7944
Important
Fixes export issue in
ProviderSettingsManagerby excluding undefined max token fields, with comprehensive test coverage added.ProviderSettingsManager.export()now excludesmodelMaxTokensandmodelMaxThinkingTokensif undefined, ensuring only explicitly set values are exported.importExport.spec.tsto verify exclusion of undefined fields during export.This description was created by
for 5a01505. You can customize this summary. It will automatically update as commits are pushed.