Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Update CodyLLMConfiguration GraphQL object to be modelconfig aware#63886

Merged
chrsmith merged 2 commits into
mainfrom
chrsmith/gql-view-of-supported-models
Jul 17, 2024
Merged

Update CodyLLMConfiguration GraphQL object to be modelconfig aware#63886
chrsmith merged 2 commits into
mainfrom
chrsmith/gql-view-of-supported-models

Conversation

@chrsmith

@chrsmith chrsmith commented Jul 17, 2024

Copy link
Copy Markdown
Contributor

We recently updated the completions APIs to use the modelconfig system 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 modelconfig system. 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 CodyLLMConfiguration GraphQL 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 graphqlbackend package. But it's definition is in internal/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

@chrsmith chrsmith requested review from a team and emidoots July 17, 2024 18:45
@cla-bot cla-bot Bot added the cla-signed label Jul 17, 2024

@emidoots emidoots left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) :)

@chrsmith chrsmith merged commit 3cb1c45 into main Jul 17, 2024
@chrsmith chrsmith deleted the chrsmith/gql-view-of-supported-models branch July 17, 2024 19:38
@chrsmith

Copy link
Copy Markdown
Contributor Author

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.

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 .../supported-models.json.

So just getting the two things "to agree" is a starting point :D

chrsmith referenced this pull request Aug 6, 2024
…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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants