Initial language model configuration management #6878

timtmok · 2025-03-19T20:08:19Z

First PR for #6612

Adds a new dialog for configuring language models. The dialog is hidden behind a preference positron.assistant.newModelConfiguration, which shows the new dialog when true.

This also removes the other models that are not initially supported from the selection list. The pared down list is Anthropic and Gemini. There are a few others added when in dev mode (AWS Bedrock [Configured by environment variables], Echo, and Error [test models]).

Adding an Anthropic or Gemini model is not expected to work properly. It can add but the UI will not be a good workflow. It doesn't handle adding it properly as a chat or completion model and this will be addressed in #6613 and #6614.

For now, the Echo test model is available to show that a model can be signed in and signed out. The dialog now has an action callback so that the extension can perform a save or delete on the model. Another callback is added to signal to the extension that it can clean up the action callback.

Release Notes

New Features

N/A

Bug Fixes

N/A

QA Notes

Not much to test here at this point.

Add horizontal scrolling for model providers

Cleans up callbacks on dialog close

github-actions · 2025-03-19T20:08:40Z

E2E Tests 🚀
This PR will run tests tagged with: @:critical

^readme ^{valid tags}

sharon-wang

Works for me on Desktop and Server Web on Mac 👍

Recognizing that this is the initial modal implementation -- a couple questions on the UI/UX:

Do we have a plan for accessibility? In particular, I noticed the modal is not yet keyboard-navigable
Do we intend to move the feedback messages from notification toasts to feedback within the modal?

sharon-wang · 2025-03-20T15:12:20Z

src/vs/workbench/api/common/positron/extHostAiFeatures.ts

-		if (!onSave) {
+	async $responseLanguageModelConfig(id: string, config: IPositronLanguageModelConfig, action: string): Promise<void> {
+		const onAction = this._languageModelRequestRegistry.get(id);
+		if (!onAction) {
 			throw new Error('No matching language model configuration request found');


Could it be helpful to log some part of the id/config and the action for a bit more info in the error?

This part is pretty weird code TBH. The extension wants to pass a callback to Positron but it really caches it here. When Positron wants to do something with the extension, its callback leads back here to execute what to do for the action. In theory, this would never happen and, if it did, something really went wrong. Hopefully, a reprex would show what happened as this structure would only hold one item at a time.

I think eventually, part of the extension code should move into Positron so this weird stuff isn't needed just to save or delete a model configuration.

sharon-wang · 2025-03-20T15:15:13Z

src/vs/workbench/common/theme.ts

+// < --- Positron Icon Button --- >
+export const POSITRON_ICON_BUTTON_BORDER = registerColor('positronIconButton.border', {
+	dark: '#175ab5',
+	light: '#70a6e0',
+	hcDark: '#ff0000',
+	hcLight: '#ff0000'
+}, localize('positronIconButton.border', "Positron icon button border color."));
+
+export const POSITRON_ICON_BUTTON_BACKGROUND = registerColor('positronIconButton.background', {
+	dark: '#214984',
+	light: '#a1c5ef',
+	hcDark: '#ff0000',
+	hcLight: '#ff0000'
+}, localize('positronIconButton.background', "Positron icon button background color."));
+


Should we match the colours from the project wizard above, for consistency?

Good idea. I kind of picked something that looked decent initially when trying to find colours.

sharon-wang · 2025-03-20T15:17:32Z

src/vs/workbench/contrib/positronAssistant/browser/icons/anthropic.tsx

+import * as React from 'react';
+import type { SVGProps } from 'react';
+
+export const Anthropic = (props: SVGProps<SVGSVGElement>) => (


Are there any copyright/license notices to include with these logos?

sharon-wang · 2025-03-20T15:20:39Z

extensions/positron-assistant/src/config.ts

 	// Get the configuration option listing enabled providers
 	let enabledProviders: string[] =
 		vscode.workspace.getConfiguration('positron.assistant').get('enabledProviders') || [];
+	const supportedProviders = await positron.ai.getSupportedProviders();
+	enabledProviders.push(...supportedProviders);


any chance we could get duplicate providers?

sharon-wang · 2025-03-20T15:26:49Z

extensions/positron-assistant/src/md/welcome.md

-Positron Assistant is an AI copilot, designed to help you with data science and visualization tasks.
-
-Always verify results. AI assistants can sometimes produce **incorrect or misleading information**.
+To use Positron Assistant you must first select and authenticate with a language model provider.


The messaging here has changed quite a bit from the existing text, in particular, the disclaimer about verifying results. Is it intentional to change the messaging here?

Another thought is to match the text with the link to Add a Language Model that appears below. Maybe we can use "Select a Language Model" everywhere?

Yes, this was intentional from the design spec. There will be another change coming to make the link a Sign-in with provider button.

extensions/positron-assistant/src/config.ts

sharon-wang · 2025-03-20T15:30:50Z

extensions/positron-assistant/src/config.ts

+	let { name, model, baseUrl, apiKey, ...otherConfig } = userConfig;
+	name = name.trim();
+	model = model.trim();
+	baseUrl = baseUrl?.trim();
+	apiKey = apiKey?.trim();


nit to do the trimming in a separate function, so these variables can all be consts in here

extensions/positron-assistant/src/config.ts

sharon-wang · 2025-03-20T15:39:31Z

extensions/positron-assistant/src/config.ts


-		// Register the new model
-		const registered = await registerModel(newConfig, context, storage);
+async function deleteConfigurationByProvider(context: vscode.ExtensionContext, storage: SecretStorage, providerId: string) {


For my understanding, when do we use this method of deletion a config via provider versus deleteConfiguration?

I do want to clean this up in subsequent PR's. The initial implementation can add multiple models from a single provider. deleteConfiguration deletes by the UUID generated when configuring the model. deleteConfigurationByProvider removes by the provider id. My intent is to register once with a provider and it will create a model configuration for chat and completion when possible. When that happens, the UUID would no longer be required.

jmcphers · 2025-03-20T18:17:10Z

src/vs/workbench/contrib/positronAssistant/browser/positronAssistantService.ts

+
+	getSupportedProviders(): string[] {
+		const providers = ['anthropic', 'google', 'copilot'];
+		if (IsDevelopmentContext.getValue(this._contextKeyService)) {


Could you add a config value that would cause this to happen in release builds too? Then you could close #6876 with this change, too.

Co-authored-by: sharon <sharon-wang@users.noreply.github.com> Signed-off-by: Tim Mok <timtmok@users.noreply.github.com>

timtmok added 9 commits March 19, 2025 09:30

Initial UI

fb620c4

Add signedIn status to model source

c54fbf2

Toggle sign in button state

73eb23c

Add hidden config for new dialog

55a5a65

Remove a model

ff85f80

Add test models in dev mode

b27142c

Add horizontal scrolling for model providers

Clean up callback

9044201

Add close event

0ad9e2a

Cleans up callbacks on dialog close

Restore completion models

63a0362

timtmok requested review from jmcphers and sharon-wang March 19, 2025 20:08

Fix compile error

8287ccd

sharon-wang reviewed Mar 20, 2025

View reviewed changes

jmcphers previously approved these changes Mar 20, 2025

View reviewed changes

Add error details to message

959ac4b

Co-authored-by: sharon <sharon-wang@users.noreply.github.com> Signed-off-by: Tim Mok <timtmok@users.noreply.github.com>

timtmok dismissed jmcphers’s stale review via 959ac4b March 21, 2025 14:28

timtmok and others added 4 commits March 21, 2025 10:38

Add detail to error message

89a14c0

Co-authored-by: sharon <sharon-wang@users.noreply.github.com> Signed-off-by: Tim Mok <timtmok@users.noreply.github.com>

Add hidden setting for enabling test models

cd0233a

Match hover colours to new project dialog

062a456

Use constants for destructuring

2648ae6

timtmok merged commit dcc1568 into main Mar 21, 2025
8 checks passed

timtmok deleted the 6612-model-provider-management branch March 21, 2025 17:43

github-actions bot locked and limited conversation to collaborators Mar 21, 2025

Initial language model configuration management #6878

Initial language model configuration management #6878

Uh oh!

Conversation

timtmok commented Mar 19, 2025

Release Notes

New Features

Bug Fixes

QA Notes

Uh oh!

github-actions bot commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharon-wang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 19, 2025 •

edited

Loading