-
Notifications
You must be signed in to change notification settings - Fork 128
Assistant: add setting for user-defined model listing #10611
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: main
Are you sure you want to change the base?
Conversation
- add positron.assistant.configuredModels setting to allow users to statically configure model settings per provider, which overrides dynamic model resolution - update and add tests around model resolution
|
E2E Tests 🚀 |
| // Update a stale model configuration to the latest defaults | ||
| const models = availableModels.get('amazon-bedrock')?.map(m => m.identifier) || []; | ||
| if (!(_config.model in models)) { | ||
| _config.name = AWSLanguageModel.source.defaults.name; | ||
| _config.model = AWSLanguageModel.source.defaults.model; | ||
| } |
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 removed this as we no longer use the hardcoded model listing, as we've switched to using the Bedrock SDK to dynamically retrieve the models.
melissa-barca
left a comment
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.
Nice work! Thanks for including thorough test coverage
Co-authored-by: Melissa Barca <[email protected]> Signed-off-by: sharon <[email protected]>
Co-authored-by: Melissa Barca <[email protected]> Signed-off-by: sharon <[email protected]>
Co-authored-by: Melissa Barca <[email protected]> Signed-off-by: sharon <[email protected]>
melissa-barca
left a comment
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.
Overall this is looking great. Thinking about this as a Workbench feature, it's possible an admin will misconfigure the setting and then need to play a back and forth game of telephone with the user and possibly our support/dev teams to debug the misconfiguration. We can try to make this process easier by adding in some validation that runs when the assistant extension activates. I think we have everything we need to verify the provider and surface any errors (probably as a notification and to the output pane?) The current experience isn't ideal if the configured model doesn't actually exist, but I'm not sure if there's a great way to improve this as we already log an error that includes the model we attempted to use.
|
What do you think about adding a command like I'm leaning towards a command as checking all the models on each extension activation would cause a delay, with how the code is currently structured (each provider is connected to in sequence). I can open an issue for this separately if that sounds like a decent approach to try!
|
|
I like that idea! I think it will be helpful for Admins and support if there are model issues. How do you feel about adding provider verification as part of this PR? |
47bba03 to
1dbf111
Compare
Added provider verification and a couple basic tests in 1dbf111! settings.json Here's what the warning looks like looks like -- runs during extension activation:
There's also a message in the output log: |
logged #10702 to capture this idea! |

Summary
positron.assistant.configuredModelssetting to allow users to statically configure model settings per provider, which overrides dynamic model resolutionApproach
resolveModels()method now callsretrieveModelsFromConfig()to check for statically defined models viapositron.assistant.configuredModelsor hardcoded models, thenretrieveModelsFromApi()to dynamically retrieve the model listing if applicable for the providerRelease Notes
New Features
QA Notes
npm run test-extension -- -l positron-assistant)