-
Notifications
You must be signed in to change notification settings - Fork 119
Assistant: Improve managing language models #9799
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: prerelease/2025.10
Are you sure you want to change the base?
Conversation
E2E Tests 🚀 |
Looks like I've broken something, will be busy over the new few days but will debug failing tests when I can. |
I can take over and see what's going on with the tests. It might be the test echo model that is broken here. |
Sure, feel free to commit straight to this branch if you figure it out. |
src/vs/workbench/contrib/chat/browser/actions/manageModelsActions.ts
Outdated
Show resolved
Hide resolved
Thank you for fixing the tests! Note that the change in 4e65121 does not work for me, because the display names for the "Copilot" vendor and the "GitHub Copilot Chat" provider do not match. So Copilot is missing from the model management quickpick. We have access to the vendor IDs here, I think we should match on that instead. |
- required an update to the display name of AWS Bedrock to Amazon Bedrock in our UI (to match the name in package.json)
Run test locally via `./scripts/test.sh --grep "LanguageModels"`
causes unnecessary debug noise in the logs
this is the model we've done the most testing with and seems to work well, even though claude sonnet 4.5 is newer
…ed model over default model
065ff0f
to
b6de4b7
Compare
maxOutputTokens: maxOutputTokens, | ||
capabilities: this.capabilities, | ||
isDefault: isFirst, | ||
isDefault: this.isDefaultUserModel(model.id, model.display_name), |
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'm not sure if the defaultModels
setting works as intended. If I set the default to Claude Sonnet 4.5
, it does set it as default but Claude Sonnet 4
is also set as default. I end up with 4 selected and not 4.5.
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've changed it so that there will never be more than one default model.
// Get and apply LLM allow filters from configuration. | ||
const config = this._configurationService.getValue<{ filterModels: string[] }>('positron.assistant'); | ||
this._logService.trace('[LM] Applying model filters:', config.filterModels); | ||
if (config.filterModels.length > 0 && !config.filterModels.some(pattern => |
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 somehow got into a bad state when using this with Bedrock and these settings:
"positron.assistant.filterModels": [
"Claude*",
"GPT-5"
]
When I went to manage models, I could see Bedrock as a provider in the quick pick. Selecting it resulted in nothing showing. I can see Bedrock as a provider in the chat view but it's filtered out due to how we track available providers. Available providers are calculated from available models but the model filter is essentially filtering out providers.
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've added some eventing around the filterModels
setting so that the provider selector updates.
Ensures there cannot be more than one default model
Sets a new current provider if it is filtered out
This PR makes a few changes in an attempt to improve managing LLM provider models.
The manage models quickpick menu has been improved:
managementCommand
property has been added to Assistant providers, so that the cog icon works.A new configuration setting
positron.assistant.filterModels
has been added. With this users (and in the future admins) can set glob filters. Models will only be shown if they match one of the given globs.A
positron.assistant.preferredModel
setting has been added. With this, when the Positron window first loads it should default to loading the model given in that setting. Here we use partial matching on the ID or model names.A
positron.assistant.defaultModels
settings has been added. With this, users can specify the default model for a particular provider, which we will pre-select in the model selector if the user hasn't specified a preferred model instead.Claude Sonnet 4
and Bedrock toClaude 4 Sonnet Bedrock
Release Notes
New Features
positron.assistant.filterModels
to configure available models (Assistant: Include/Exclude filters for LLM models #9788)positron.assistant.preferredModel
to set the preferred model to pre-select in the model selector (Assistant: configure default model per provider #9388)positron.assistant.defaultModels
to set the default model per provider (Assistant: configure default model per provider #9388)Bug Fixes
QA Notes
Manage models quickpick
Model filtering
Setting preferred model
positron.assistant.filterModels
setting and set the following:Setting default model per provider
Note 1: Upstream has code to remember your last selection, so after the reload window you have to change providers to see the default kick in.
Note 2: Sometimes Assistant gets stuck in a state showing "Pick Model..." in the selector. Switching providers does nothing to clear that, but selecting a model first, and then switching providers, gets things working again. I don't know if this is related to this commit, or the changes in #9799.