-
Notifications
You must be signed in to change notification settings - Fork 291
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(enterprise): allow multiple models for enterprise customers #4780
Conversation
- Add ModelTag enum to categorize models - Remove deprecated models from model options - Update ModelSelectField to only show non-deprecated models TODO: - replace uiGroup with tags - remove codyProOnly & uiGroupfields & usage from Model class
- Add `isCodyProModel`, `isCustomModel`, `isLocalModel`, and `isOllamaModel` utility functions to identify different types of models - Update model tags to include `Local` and provide more context around the meaning of different tags - Update usage of these utility functions in the codebase
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.
left some comments in line. It looks like the model/index.ts file was removed by accident so I wasn't able to test it locally, but most of the changes look good to me!
There is an error caused by the change from my other commits that was merged into your branch, I've started the PR (that target your current branch) with the changes to fix that: #4795
Feel free to merge that into this branch and make any additional changes as you need!
Great job!
agent/src/agent.ts
Outdated
modelID = ModelsService.getDefaultChatModel(authStatus) | ||
} | ||
if (!modelID) { | ||
throw new Error('agent::chat/restore: no chat model found') |
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.
This assumes the models are loaded before the chat is restore. I think we can just skip this check for chat/restore because of the model doesn't exists it will just fall back to use the default anyway, instead of breaking the chat with no way to fix it in the UI. Wdyt?
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.
Sure. Except I don't see where it would fallback to the default, everywhere in SimpleChatProvider
it just uses the modelId directly.
lib/shared/src/models/dotcom.ts
Outdated
usage: [ModelUsage.Chat, ModelUsage.Edit], | ||
// Has a higher context window with a separate limit for user-context. | ||
contextWindow: expandedContextWindow, | ||
deprecated: false, | ||
uiGroup: ModelUIGroup.Balanced, | ||
tags: [ModelTag.Gateway, ModelTag.Balanced, ModelTag.Free], |
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.
We can probably add a comment to this file or turn this file into a file we use for testing, since this file will soon be deprecated.
@@ -33,5 +33,5 @@ export function useChatModelByID( | |||
|
|||
export function useCurrentChatModel(): Model | undefined { | |||
const { chatModels } = useChatModelContext() | |||
return chatModels?.find(model => model.default) ?? chatModels?.[0] | |||
return chatModels?.[0] |
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.
Before this PR, whatever that is set to the current default would be the current model. Did we make new changes where we would move the last used model to the front of the model list?
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.
Yes it happens here. Trying to update the field default
was kind of a violation of the immutability of Model
and did lots of weird casting which I didn't love. And besides we alway fell back to the head of the list so I just settled on that convention.
2053803
to
1bcbf5e
Compare
Can be ignored, it's just a syntactic change to an existing function |
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 left some comments inline. Most of them are non-blockers, except for some of the the ModelTags are not assigned correctly for the pre-definded dotcom models. Other than that they all look good to me, and the models are set correctly on reload and updated when I run it locally. Great job!!
// BUG: There is data loss here and the potential for ambiguity. | ||
// BUG: We are assuming the modelRef is valid, but it might not be. |
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.
maybe in a follow-up, we could create a helper function to validate if the modelRef is valid or not?
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.
Definitely. But I can look into that in a subsequent PR as we do more end-to-end testing once all the backend changes are in-place.
Co-authored-by: Beatrix <68532117+abeatrix@users.noreply.github.com>
Co-authored-by: Beatrix <68532117+abeatrix@users.noreply.github.com>
vscode/CHANGELOG.md
Outdated
@@ -9,6 +9,7 @@ This is a log of all notable changes to Cody for VS Code. [Unreleased] changes a | |||
- Ollama: Added support for running Cody offline with local Ollama models. [pull/4691](https://github.com/sourcegraph/cody/pull/4691) | |||
- Edit: Added support for users' to edit the applied edit before the diff view is removed. [pull/4684](https://github.com/sourcegraph/cody/pull/4684) | |||
- Autocomplete: Added experimental support for Gemini 1.5 Flash as the autocomplete model. To enable this experimental feature, update the `autocomplete.advanced.provider` configuration setting to `unstable-gemini`. Prerequisite: Your Sourcegraph instance (v5.5.0+) must first be configured to use Gemini 1.5 Flash as the autocomplete model. [pull/4743](https://github.com/sourcegraph/cody/pull/4743) | |||
- Enterprise: Enabled support for multiple dynaic models if the Sourcegraph backend provides them. Requires the experimental flag `cody.dev.useServerDefinedModels` which currently returns a static list of models. Note: this won't work until the server side of this feature is merged. |
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.
- Enterprise: Enabled support for multiple dynaic models if the Sourcegraph backend provides them. Requires the experimental flag `cody.dev.useServerDefinedModels` which currently returns a static list of models. Note: this won't work until the server side of this feature is merged. | |
- Enterprise: Enabled support for multiple dynaic models if the Sourcegraph backend provides them. Requires the experimental flag `cody.dev.useServerDefinedModels` which currently returns a static list of models. Note: this won't work until the server side of this feature is merged. [pull/4780](https://github.com/sourcegraph/cody/pull/4780) |
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.
Ooh this actually reminds me that that changelog is no longer true. I'll update both.
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.
This is awesome! I've manually tested the following and verified it works accordingly:
[x] Can switch between chat models
[x] Chat models are displayed with the correct icons
[x] Chat models are displayed correctly with the correct UI group assigned
[x] New chat panel defaulted to use the last selected model correctly
[x] Can switch between inline-edit models
[x] Inline-edit models are displayed with the correct icons
[x] New chat panel defaulted to use the last selected model correctly
[x] Reloaded editor and the last selected models are showing up as default correctly
Screen.Recording.2024-07-10.at.10.37.03.AM.mov
Thanks @abeatrix for the thorough testing! |
@chrsmith Do you want me to hold off merging this until you get a chance to review it? |
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.
From my understanding of how things all fit together this looks good to me! I assume there will be some subtle details to work out as we do more testing with server-supplied LLM models. But overall this looks like a great step in the right direction.
I wouldn't block submission on any of my suggestions or nit picks. Just things to consider.
usage: [ModelUsage.Chat, ModelUsage.Edit], | ||
contextWindow: expandedContextWindow, | ||
deprecated: false, | ||
uiGroup: ModelUIGroup.Accuracy, | ||
tags: [ModelTag.Gateway, ModelTag.Accuracy, ModelTag.Recommended, ModelTag.Free], |
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.
Would you mind elaborating on why you added this refactoring? It's clearly an improvement over having fields like default
and deprecated
, and it's also a cleaner way to represent things like "UI Groups". (Read: 👍 💯 !)
I'm just wondering if this was a change that had been discussed on the Cody Core team for a while, and/or if it was just something you saw as beneficial in this PR. Since, depending on that, maybe we'd want to reflect this "model tags" approach in the backend, too.
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.
This started as @abeatrix brain child, so I'll let her elaborate on it.
// in the UI | ||
public deprecated = false | ||
/** | ||
* The tags assigned for categorizing the 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.
Would we want to add some sort of invariant to enforce certain standards for tags? e.g. enforcing that a model can only have one tag in the set { accuracy, speed, balanced }? I can see a high-quality model being both accurate and fast... but since those terms are subjective, it might just lead to that type of classification being meaningless over time.
WDYT?
// BUG: There is data loss here and the potential for ambiguity. | ||
// BUG: We are assuming the modelRef is valid, but it might not be. |
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.
Definitely. But I can look into that in a subsequent PR as we do more end-to-end testing once all the backend changes are in-place.
await ModelsService.storage?.set(ModelsService.storageKeys[type], resolved.model) | ||
} | ||
|
||
public static canUserUseModel(status: AuthStatus, model: string | Model): boolean { |
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.
Nit: canUserUseModel
is a bit clunky. What do you think about just canUseModel
or isModelAvailable
?
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.
For me, that's asking "Is this model functionally usable" but the question is really "does this user have permissions for this model". I could see isModelAvailableFor
but that might be just as clunky.
export function capabilityToUsage(capability: ModelCapability): ModelUsage[] { | ||
switch (capability) { | ||
case 'autocomplete': | ||
return [] |
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.
Not sure how this particular function will impact things, but my understanding is that "Edit" actually means "Code Editing" AKA "autocompletions". (And renaming "edit" to "autocomplete" was an intentional break in the new server-side schema.)
So this might not be pedantically correct, but I'm not sure if it matters.
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.
This is just trying to parse what was specified here to something the extension can understand.
From what I've seen on the front end, autocomplete models seem to be handled by configuration values and not from the ModelsService, but I'm not sure how that works for enterprise. I've tried to wade through the completions code but it's non-trivial to say the least.
Outside of autocomplete, each model may flag that they support being used for chat or for being used for code-editing (like generate unit tests, fixup etc), but it's possible that some models aren't available for both.
So I'm not sure if what you were describing was on the server side or current client side, but that's my brain dump. Given that, what do you think the mapping should be?
} | ||
} | ||
|
||
// TODO(jsm): delete these |
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.
Friendly reminder to delete these :D
vscode/CHANGELOG.md
Outdated
@@ -9,6 +9,7 @@ This is a log of all notable changes to Cody for VS Code. [Unreleased] changes a | |||
- Ollama: Added support for running Cody offline with local Ollama models. [pull/4691](https://github.com/sourcegraph/cody/pull/4691) | |||
- Edit: Added support for users' to edit the applied edit before the diff view is removed. [pull/4684](https://github.com/sourcegraph/cody/pull/4684) | |||
- Autocomplete: Added experimental support for Gemini 1.5 Flash as the autocomplete model. To enable this experimental feature, update the `autocomplete.advanced.provider` configuration setting to `unstable-gemini`. Prerequisite: Your Sourcegraph instance (v5.5.0+) must first be configured to use Gemini 1.5 Flash as the autocomplete model. [pull/4743](https://github.com/sourcegraph/cody/pull/4743) | |||
- Enterprise: Enabled support for multiple dynaic models if the Sourcegraph backend provides them. Requires the experimental flag `modelsAPIEnabled` to be sent by the client config API. Note: this won't work until the server side of this feature is merged. [pull/4780](https://github.com/sourcegraph/cody/pull/4780) |
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.
It seems odd to add a caveat like "this won't work until..." to the changelog, since by the time we expect someone to actually read it in a release that should already be the case, right? (Also, @slimsag can keep me honest, but I believe the modelsAPIEnabled
field is already available.)
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.
We should just add the changelog entries when we're done with everything yeah.
And yes, modelsAPIEnabled
already works today - it will be true
as long as you add this to your site config:
"modelsConfiguration": {},
@chrsmith I started to work on the server sent defaults but I realized there was going to be a significant amount of work. Since this keeps getting outdated I'm going to merge what we have now, and then continue with the defaults work on another. |
This update is to align with a [refactor](sourcegraph/cody#4780) in the vscode repo around model types to remove certain hard coded fields and replace them with ModelTags. This is very rough around the edges because I don't know how to access the generated code in that PR (nor am I aware of the context on this repo) so any feedback is welcome! ## Test plan Green CI --------- Co-authored-by: Piotr Kukielka <pkukielka@virtuslab.com>
Allows for enterprises to support multiple models. Additionally contains some refactors for unifying further a single Model and ModelsService class.
Test plan
Added E2E test