Update CodyLLMConfiguration GraphQL object to be modelconfig aware#63886
Conversation
emidoots
left a comment
There was a problem hiding this comment.
Oh this is interesting.. generally LGTM
I didn't realize you were working on this, me and @jamesmcnamara were talking about this issue (CodyLLMConfiguration being deprecated/superseded by the newer model and client config APIs) and how to approach it in clients yesterday.
Luckily no conflict, I think, having this be more reliable is all around good (esp. for older clients) :)
Oh, sorry about that. LMK what you two come up with... we don't want to break older clients that will call this endpoint. But at the same time, it is a subset of what would be returned from the So just getting the two things "to agree" is a starting point :D |
…4276) This PR fixes Cody autocomplete in some situations, fixing [PRIME-426](https://linear.app/sourcegraph/issue/PRIME-426/autocomplete-completely-broken-in-main-with-and-sometimes-without-the). Our story begins with this PR: https://github.com/sourcegraph/sourcegraph/pull/63886. The PR updated the `CodyLLMConfigurationResolver.Provider` GraphQL endpoint to no longer return the "provider" that was put into the site configuration, but to instead return the _display name_ of the provider, or if multiple were configured the string "various". ```diff - func (c *codyLLMConfigurationResolver) Provider() string { - return string(c.config.Provider) - } + func (c *codyLLMConfigurationResolver) Provider() string { + if len(r.modelconfig.Providers) != 1 { + return "various" + } + return r.modelconfig.Providers[0].DisplayName +} ``` This change was wrong on several levels, and unfortunately stemmed from the original author (me) not tracking down and understanding how the GraphQL endpoint was actually used. Once we discovered the problem, we quickly rectified this by changing the behavior to the more correct version of returning the Provider ID of the code completion model with https://github.com/sourcegraph/sourcegraph/pull/64165: ```go func (r *codyLLMConfigurationResolver) Provider() string { return string(r.modelconfig.DefaultModels.CodeCompletion.ProviderID()) } ``` However, after some more testing we discovered yet another edge case. We didn't realize that the provider "sourcegraph" (which is how you configure Sourcegraph to use Cody Gateway, using the older site config method) was required in some scenarios on the Cody client. So the new logic, of returning the Provider ID was incorrect. (Because we report the _model_ provider, e.g. "anthropic". Not the _API_ provider, e.g. "sourcegraph"/"Cody Gateway".) With this change, we should just have the behavior we had initially: returning whatever the admin had configured in the "completions.provider" section of the site config. If only the newer modelconfig settings were used, we return "sourcegraph" if using Cody Gatway. And if not, just return the Provider ID. (Though in some situations even that will lead to incorrect results, because ultimately we need to update the client here.) ## Test plan I wasn't able to test this super-well manually, and am relying on Stephen and Taras' knowledge of how the client uses this today. ## Changelog NA
We recently updated the completions APIs to use the
modelconfigsystem for managing LLM model configuration. Behind the scenes, we automatically converted the existing site configuration ("completions config") into the newer format so things work as expected.However, the GraphQL view of the Sourcegraph instance's LLM configuration was not updated to use the
modelconfigsystem. And so fi the site admin and opted into using the new-style of configuration data, the data returned would be all sorts of wrong.(Because the GraphQL handler looked for the "completions config" part of the site config, and not the newer "model configuration" section.)
This PR updates the
CodyLLMConfigurationGraphQL resolver to return the data from the modelconfig component of the Sourcegraph instance.Some careful refactoring was needed to avoid a circular dependency in the Go code. So the resolver's type declaration is in the
graphqlbackendpackage. But it's definition is ininternal/modelconfig.Test plan
I only tested these changes manually.
If you open the Sourcegraph instance's API console, this is the GraphQL query to serve all of the data:
{ site { codyLLMConfiguration { chatModel fastChatModel completionModel provider disableClientConfigAPI chatModelMaxTokens fastChatModelMaxTokens completionModelMaxTokens } } }Changelog
NA