Adding support to save the locally selected model so that new chats don´t require repeated model selection #2438

arafatkatze · 2023-12-19T04:51:08Z

I was quite frustrated with the loading issue as well so I changed it. The video is pretty self explanatory. It saves the state even after you quit and load Cody again.

I have a strong suspicion that a lot of users will switch to GPT 4-Turbo once and then never switch back

Untitled.mov

Test plan

Just try it out and open close a few times

valerybugakov

I also started looking into it because I want GPT-4 by default. TY!

philipp-spiess

This is awesome! I would expect some kind of validation to go on though, did you look into this?

Removing @valerybugakov's approval not to merge this too eagerly since it could cause some nasty errors

philipp-spiess · 2023-12-19T08:54:33Z

vscode/src/chat/chat-view/ChatPanelsManager.ts

@@ -205,7 +207,7 @@ export class ChatPanelsManager implements vscode.Disposable {
                      this.options.contextProvider,
                      this.options.platform
                  ),
-                  defaultModelID: defaultModel.model,


We should check if the saved model is actually a valid model name for the current configuration. Otherwise, if you switch from a dotcom account to enterprise or from pro to free, this may result in a wrong model identifier which, when used, might cause errors.

It's the right call. I was too eager to get into the release to start using it.

It seems strange that we write the last selected model ID in SimpleChatPanelProvider, but read it in ChatPanelsManager to pass into SimpleChatPanelProvider as a constructor param. Where possible, it's better to localize state management. Could we move the localStorage.get call to an onReady method in SimpleChatPanelProvider that runs when we receive the ready message?

Moved the model selection to this function

cody/vscode/src/chat/chat-view/SimpleChatPanelProvider.ts

Lines 176 to 192 in 2b9310b

private selectModel(models: ChatModelProvider[]): string {

// Check for the last selected model

const lastSelectedModelID = localStorage.get('model')

if (lastSelectedModelID) {

// If the last selected model exists in the list of models then we return it

const model = models.find(m => m.model === lastSelectedModelID)

if (model) {

return lastSelectedModelID

}

}

// If the user has not selected a model before then we return the default model

const defaultModel = models.find(m => m.default) || models[0]

if (!defaultModel) {

throw new Error('No chat model found in server-provided config')

}

return defaultModel.model

}

Also it takes into account that the model selected is valid for that user.

Could we move the localStorage.get call to an onReady method in SimpleChatPanelProvider that runs when we receive the ready message?

Not sure if I fully understand this but hopefully moving the model selection to a single place makes the code better than before.

beyang · 2023-12-19T17:00:26Z

vscode/src/chat/protocol.ts

@@ -77,6 +77,7 @@ export type WebviewMessage =
    | { command: 'abort' }
    | { command: 'custom-prompt'; title: string; value?: CustomCommandType }
    | { command: 'reload' }
+    | { command: 'saveModel'; model: string }


The chatModel command already exists; why not use that one?

Need to store and load from storage.

@arafatkatze Not sure I follow: If we already send a chatModel message, why would we need another one to save that if the behavior is "always save the last one as default"? 🤔

beyang · 2023-12-19T17:06:35Z

vscode/src/chat/chat-view/ChatPanelsManager.ts

@@ -139,7 +140,8 @@ export class ChatPanelsManager implements vscode.Disposable {
        // Get the view column of the current active chat panel so that we can open a new one on top of it
        const activePanelViewColumn = this.activePanelProvider?.webviewPanel?.viewColumn

-        const provider = this.createProvider()
+        const savedModel = localStorage.get('model')


Let's give the key a more distinctive name, like "lastSelectedModelID"

beyang · 2023-12-19T17:14:05Z

vscode/src/chat/chat-view/ChatPanelsManager.ts

@@ -205,7 +207,7 @@ export class ChatPanelsManager implements vscode.Disposable {
                      this.options.contextProvider,
                      this.options.platform
                  ),
-                  defaultModelID: defaultModel.model,


It seems strange that we write the last selected model ID in SimpleChatPanelProvider, but read it in ChatPanelsManager to pass into SimpleChatPanelProvider as a constructor param. Where possible, it's better to localize state management. Could we move the localStorage.get call to an onReady method in SimpleChatPanelProvider that runs when we receive the ready message?

…vider

philipp-spiess

This looks crisp now but I’m still unsure why we need a new notification for this instead of reusing. Do you mind elaborating?

philipp-spiess

Sharp! This looks good now. Let's add a change log entry though 🙇

philipp-spiess · 2023-12-21T16:52:05Z

I wonder why the test still fails 🤔 Did you create the PR in a fork of the cody repo? You can now create your branches on this repo instead, that should help!

philipp-spiess

I found one more nit I’m afraid. I don't feel great that we have a compute function but keep a copy of the value in the class properties. Can we avoid this somehow?

philipp-spiess · 2023-12-21T16:55:16Z

vscode/src/chat/chat-view/SimpleChatPanelProvider.ts

@@ -108,7 +109,7 @@ export class SimpleChatPanelProvider implements vscode.Disposable, IChatPanelPro
    private readonly contextStatusAggregator = new ContextStatusAggregator()
    private readonly editor: VSCodeEditor
    private readonly treeView: TreeViewProvider
-    private readonly defaultModelID: string
+    private readonly modelID: string


Do we need this as a member now that we can compute it based on the localStorage? Asking because I just noticed that we have this.modelID and this.chatmodel.modelID and we only update one at some point. It's better to not keep duplicate references around I think.

philipp-spiess · 2023-12-21T16:55:46Z

vscode/src/chat/chat-view/SimpleChatPanelProvider.ts

@@ -336,6 +355,8 @@ export class SimpleChatPanelProvider implements vscode.Disposable, IChatPanelPro
                break
            case 'chatModel':
                this.chatModel.modelID = message.model


Should this also update this.modelID (and if yes, can we make it so that there is no duplication?)

philipp-spiess

Bliss

we fixed the issues

arafatkatze requested review from philipp-spiess, beyang and dominiccooney December 19, 2023 04:51

valerybugakov approved these changes Dec 19, 2023

View reviewed changes

philipp-spiess reviewed Dec 19, 2023

View reviewed changes

philipp-spiess requested a review from valerybugakov December 19, 2023 08:55

beyang previously requested changes Dec 19, 2023

View reviewed changes

Incorporating feedback to limit model selection to SimpleChatpanelpro…

2b9310b

…vider

arafatkatze force-pushed the savemodel branch from d403b80 to 2b9310b Compare December 20, 2023 21:58

philipp-spiess reviewed Dec 21, 2023

View reviewed changes

arafatkatze added 3 commits December 21, 2023 22:14

Removing redundant function

3ecd48d

Removing redundant function

5f4dc15

Removing redundant function

0672114

philipp-spiess approved these changes Dec 21, 2023

View reviewed changes

philipp-spiess suggested changes Dec 21, 2023

View reviewed changes

Removing redundant variable

58fc7a3

philipp-spiess approved these changes Dec 21, 2023

View reviewed changes

arafatkatze enabled auto-merge (squash) December 21, 2023 17:15

arafatkatze disabled auto-merge December 21, 2023 17:15

arafatkatze enabled auto-merge (squash) December 21, 2023 17:23

arafatkatze mentioned this pull request Dec 21, 2023

Adding support to save the locally selected model so that new chats don´t require repeated model selection #2459

Closed

arafatkatze merged commit 9ae53a8 into sourcegraph:main Dec 21, 2023
25 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support to save the locally selected model so that new chats don´t require repeated model selection #2438

Adding support to save the locally selected model so that new chats don´t require repeated model selection #2438

arafatkatze commented Dec 19, 2023 •

edited

Loading

valerybugakov left a comment

philipp-spiess left a comment

philipp-spiess Dec 19, 2023

valerybugakov Dec 19, 2023

beyang Dec 19, 2023

arafatkatze Dec 20, 2023

beyang Dec 19, 2023

arafatkatze Dec 20, 2023

philipp-spiess Dec 21, 2023

beyang Dec 19, 2023

beyang Dec 19, 2023

philipp-spiess left a comment

philipp-spiess left a comment

philipp-spiess commented Dec 21, 2023

philipp-spiess left a comment

philipp-spiess Dec 21, 2023

philipp-spiess Dec 21, 2023

philipp-spiess left a comment

	private selectModel(models: ChatModelProvider[]): string {
	// Check for the last selected model
	const lastSelectedModelID = localStorage.get('model')
	if (lastSelectedModelID) {
	// If the last selected model exists in the list of models then we return it
	const model = models.find(m => m.model === lastSelectedModelID)
	if (model) {
	return lastSelectedModelID
	}
	}
	// If the user has not selected a model before then we return the default model
	const defaultModel = models.find(m => m.default) \|\| models[0]
	if (!defaultModel) {
	throw new Error('No chat model found in server-provided config')
	}
	return defaultModel.model
	}

Adding support to save the locally selected model so that new chats don´t require repeated model selection #2438

Adding support to save the locally selected model so that new chats don´t require repeated model selection #2438

Conversation

arafatkatze commented Dec 19, 2023 • edited Loading

Test plan

valerybugakov left a comment

Choose a reason for hiding this comment

philipp-spiess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipp-spiess left a comment

Choose a reason for hiding this comment

philipp-spiess left a comment

Choose a reason for hiding this comment

philipp-spiess commented Dec 21, 2023

philipp-spiess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipp-spiess left a comment

Choose a reason for hiding this comment

arafatkatze commented Dec 19, 2023 •

edited

Loading