-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add checkbox to control maximum context window for magistral-medium model #7453
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
feat: add checkbox to control maximum context window for magistral-medium model #7453
Conversation
…dium model - Added useMaximumContextWindow field to provider settings schema - Implemented checkbox UI in Mistral provider settings with yellow warning - Updated context window logic to limit to 41k tokens when checkbox is unchecked - Added localization strings for the new feature This allows users to opt-in to using the full 128k context window for the magistral-medium-latest model, with a warning about potential performance degradation.
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 is like debugging in a mirror - everything looks backward but the bugs are still mine.
| // Create a modified info object with reduced context window for better performance | ||
| info = { | ||
| ...info, | ||
| contextWindow: 41000 as any, // Use reduced context for better performance |
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.
Is using as any intentional here? This bypasses TypeScript's type checking and could hide potential type errors. Consider creating a proper type for the modified info object or using a type assertion that's more specific:
| contextWindow: 41000 as any, // Use reduced context for better performance | |
| contextWindow: 41000, | |
| maxTokens: 41000, |
| let info = mistralModels[id as MistralModelId] ?? mistralModels[mistralDefaultModelId] | ||
|
|
||
| // For magistral-medium-latest, optionally limit context window if useMaximumContextWindow is false | ||
| if (id === "magistral-medium-latest" && !this.options.useMaximumContextWindow && info.contextWindow > 41000) { |
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 magic number (41000) into a named constant like MAGISTRAL_MEDIUM_PERFORMANCE_LIMIT? This would make the code more maintainable and self-documenting.
| "magistral-medium-latest": { | ||
| maxTokens: 41_000, | ||
| contextWindow: 41_000, | ||
| maxTokens: 128_000, |
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.
I notice we're setting both maxTokens and contextWindow to 128k here, but the getModel() logic in mistral.ts will limit it to 41k by default when useMaximumContextWindow is false. Is this intentional? It might be clearer to keep the default at 41k here and only expand to 128k when the checkbox is enabled.
| </Checkbox> | ||
| {apiConfiguration?.useMaximumContextWindow && ( | ||
| <div className="flex items-start gap-2 p-2 rounded-md bg-[color-mix(in_srgb,var(--vscode-editorWarning-foreground)_20%,transparent)] border border-[var(--vscode-editorWarning-foreground)]"> | ||
| <span className="codicon codicon-warning text-[var(--vscode-editorWarning-foreground)] mt-0.5"></span> |
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.
Consider adding an aria-label to the warning icon for better accessibility:
| <span className="codicon codicon-warning text-[var(--vscode-editorWarning-foreground)] mt-0.5"></span> | |
| <span className="codicon codicon-warning text-[var(--vscode-editorWarning-foreground)] mt-0.5" aria-label="Warning"></span> |
This PR implements the feature requested by @LousyBook94 in PR #7452.
Summary
Added a checkbox option to allow users to opt-in to using the full 128k context window for the magistral-medium-latest model, with a warning about potential performance degradation.
Changes
useMaximumContextWindowfield to provider settings schemaHow it works
Testing
Screenshots
The feature adds a checkbox with a warning in the Mistral provider settings when the magistral-medium-latest model is selected.
Fixes the enhancement requested in #7452
Important
Adds a checkbox in Mistral settings to toggle between 41k and 128k context window for
magistral-medium-latestmodel, with schema, UI, and localization updates.useMaximumContextWindowfield to provider settings schema inprovider-settings.ts.getModel()inmistral.tsto limit context window to 41k tokens by default formagistral-medium-latest.Mistral.tsxto toggle full 128k context window.Mistral.tsxformagistral-medium-latestmodel.settings.jsonfor new checkbox and warning.mistralModelsinproviders/mistral.tsto setmaxTokensandcontextWindowto 128k formagistral-medium-latest.This description was created by
for 1401531. You can customize this summary. It will automatically update as commits are pushed.