-
Notifications
You must be signed in to change notification settings - Fork 23
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(chore): Adding tags and usage for LLM Models #1894
Conversation
@get:OptionTag(tag = "tags", nameAttribute = "") var tags: MutableList<String> by list() | ||
|
||
@get:OptionTag(tag = "usage", nameAttribute = "") var usage: MutableList<String> by 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.
I don't know the implications of this. Where is it getting this state from? Is this going to break immediately?
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 state saved in a IntelliJ xml settings files.
You will need to add migration which will convert old format to the new one .
For the reference you can look at the DeprecatedChatLlmMigration
.
In practice you will most likely have to:
- Keep
ChatModelProvider::deprecated
field but mark it as@Deprecated
. - In the migration function (see
DeprecatedChatLlmMigration::migrate
for reference):
- read all the old chats
- for each chat read the content from
deprecated
field - set
tags
appropriately - reset
deprecated
to benull
(as it won't be used anymore, we will remove it in future)
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.
Alright. I created ChatTagsLllmMigration
that I believe accomplishes this.
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.
Thank you! It looks good to me now, I just added one question about the default
but otherwise I think it is mergeable.
In order for the tests to work, you'll need to update the |
@@ -190,22 +198,20 @@ class SettingsMigrationTest : BasePlatformTestCase() { | |||
fun `test DeprecatedChatLlmMigration`() { |
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.
That is incorrect I think.
That is migration function, so it specifically migrates data form one old format to another.
It needs to work after your changes.
Then your migration function can pick up state after all existing migrations, and migrate it further (so you will have to add new test for 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.
Understood. I reverted those changes and added a new test for my migration.
if (selectedModel?.codyProOnly == true && isCurrentUserFree()) | ||
availableModels.find { it.default } | ||
if (selectedModel?.isCodyProOnly() == true && isCurrentUserFree()) | ||
availableModels.getOrNull(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.
Out of curiosity, if we do not have default
anymore isn't there any tag which would replace it?
Like recommended LLM?
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 just replaced it with the fallback convention of the head of the list. The model structs are immutable having a mutable field tracking the user's selection was a bit tricky and also easy to get wrong (multiple default's was possible). And we always fell back to just grabbing the head of the list if we couldn't find one.
I could see adding a default tag, but I would think to only use it for what the server originally sent as the default, not as the user's current selection.
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.
LGTM, but with two caveats:
- Isn't
default
replace by any tag? Don't we have any LLMs which we recommend to use? - Are those changes already in the Cody?
We bind cody version we are using by settingcody.commit
there: https://github.com/sourcegraph/jetbrains/blob/main/gradle.properties#L27
Please make sure version from this commit already contains changes to the model. If not, you need to bump that commit in this PR (you can just set it to the hash of the last commit on Codymain
)
|
Thanks! BTW. Looks like it's merged now :) |
if (selectedModel?.codyProOnly == true && isCurrentUserFree()) | ||
availableModels.find { it.default } | ||
if (selectedModel?.isCodyProOnly() == true && isCurrentUserFree()) | ||
availableModels.getOrNull(0) | ||
else selectedModel | ||
|
||
val isEnterpriseAccount = |
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 need to remove this line and just set
isVisible = true
always. - Also, I noticed that there is now
enterprise
tag present, shouldn't we filter out LLMs displayed for enterprise users based on if that tag is present? Or maybe for an enterprise account we are guaranteed to get a correct 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.
I pushed first change to your branch already.
I'm not sure if we need the second.
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 updated this because we don't want it to be visible for enterprise users that don't have multiple models enabled. However I'm still struggling with the integration tests failing and I'm not sure how to debug them. Do you know any good tricks for figuring out why they keep timing out? I tried opening the IDE to the test file and trying the document code action but it seemed to work fine.
To your second point, I think the current idea is that an enterprise user can use any model that the server provided (but most likely they will all be enterprise models).
Also, unrelated, but do you know how to access the generated kotlin code from the Cody repo? I don't see it referenced anywhere.
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.
Problem with integration tests was that updated recordings were missing (requires run of the ./gradlew :recordingIntegrationTest
). I updated them.
Interestingly it seems we were not reporting that fact properly, I will make sure we fixed that.
Also, unrelated, but do you know how to access the generated kotlin code from the Cody repo? I don't see it referenced anywhere.
Right now they are just working as a manual reference, BUT it will change very soon as there is @RXminuS PR in flight which adress that.
This change adds a new `serverSentModels` feature to the `ConfigFeatures` class, which is used to track whether the server supports sending models directly to the client. The `LlmDropdown` class is updated to subscribe to changes in the `CurrentConfigFeatures` and show/hide the dropdown based on the value of `serverSentModels`. Additionally, the `CodyAgentClientTest` and `CurrentConfigFeaturesTest` are updated to reflect the new feature.
I updated recordings for integration tests + reformatted the code to make spotless happy. Thanks @jamesmcnamara and @abeatrix for fixing bug with the auth and models! |
CODY-2746
This update is to align with a refactor 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