Cody: Experimental OpenAI autocomplete support#57872

chwarwick · 2023-10-24T15:31:47Z

Adds experimental OpenAI autocomplete support only for BYOK configurations. This brings the changes from the Azure OpenAI provider to OpenAI provider. Additional it updates the default autocomplete model for OpenAI to be gpt-3.5-turbo-instruct as this model continues to support the non-chat based completions api.

part of https://github.com/sourcegraph/sourcegraph/issues/57724

Test plan

Verified Chat & autocomplete work in VS Code
Verified Cody web works
Ran client completions and benchmark tooling.

chwarwick · 2023-10-24T15:45:07Z

@bobheadxi @unknwon @rafax tagged you all because we may bring this to the gateway to start supporting openai autocomplete.

bobheadxi · 2023-10-24T17:28:26Z

tagged you all because we may bring this to the gateway to start supporting openai autocomplete.

Why not just bring it to Gateway straight away? BYOK and Gateway should be functionally the same, and most subscriptions aren't granted access to GPT models through Gateway right now anyway, and the ones that are granted access have already run into Cody Gateway rejected their requests for some time now anyway: 334 requests in the last 35 hours, 11k in the last week

chrsmith

Hi! 👋 This is my first official code review at Sourcegraph, so to state the obvious, I have no idea or well-formed opinions about the code here. So I'll defer approval to @rafax (who is helping bring me up to speed).

But overall things look fine. I just have a few clarifying questions for my own understanding, and some minor nitpicks/suggestions for code readability.

chrsmith · 2023-10-24T17:35:27Z

internal/completions/client/openai/openai.go

+	if err != nil {
+		return nil, errors.Wrap(err, "failed to parse configured endpoint")
+	}
+	url.Path = "v1/chat/completions"


This is totally fine as-is, but if we expect to be setting a lot of these URL paths in the code like this then we may want to consider using a constant or even a proper type. (e.g. const v1ApiChatCompletions = ... or type ApiUrlRoutes { ChatCompletions string }.)

It isn't like we expect these URLs to change all that often, but if we were to put them in the same place it would be a lot easier to make the shift to v2 and/or use the same URL path rather than having the hard-coded values throughout the codebase.

But just a suggestion, feel free to ignore.

chrsmith · 2023-10-24T17:39:34Z

internal/completions/client/openai/openai.go

+	}
+
+	req.Header.Set("Content-Type", "application/json")
+	req.Header.Set("Authorization", "Bearer "+c.accessToken)


Would there be any value to adding a User-Agent header here? e.g. "Sourcegraph client {buildSHA}" or something? Would that be useful for tracking usage in the backend? Or in practice, we expect this to always be in lockstep with the Sourcegraph release. (e.g. we wouldn't have multiple Sourcegraph instances talking to the same backend gateway/proxy/thinger.)

We do have multiple Sourcegraph instances talking to the same Cody Gateway, more context here: https://docs.sourcegraph.com/cody/core-concepts/cody_gateway and go/cody-gateway

This could be useful to have for sure! A User-Agent is set here for requests going to 'external' (outside the Sourcegraph deployment) services: https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/httpcli/client.go?L138 - we should make sure nobody depends on this value being static before changing it though.

I'm open to adding (if so we should add to the other clients too) I think it should already be relatively easy to figure out since we requests are tied to a customer's instance.

chrsmith · 2023-10-24T17:41:38Z

internal/completions/client/openai/openai.go

+	return resp, nil
+}
+
+func (c *openAIChatCompletionStreamClient) makeCompletionRequest(ctx context.Context, requestParams types.CompletionRequestParameters, stream bool) (*http.Response, error) {


Would you mind adding a small comment on top of this function (and possibly makeRequest) to call out what it is doing? Specifically, what is the difference between makeRequest and makeCompletionRequest? Both functions have the same signature (right?) so to the uninitiated, when would you want to call one vs. the other?

chrsmith · 2023-10-24T17:44:07Z

internal/completions/client/openai/openai.go

 	User             string             `json:"user,omitempty"`
 }

+type openAICompletionsRequestParameters struct {


(Minor suggestion, feel free to ignore if it isn't applicable.)

I'm guessing that many of these parameters are specific to an OpenAI API? If that is the case, would it be useful to not only add a small doc comment above the type definition, but maybe add a link to the OpenAI docs to learn more? e.g. about what User should be referring to or the range of PresencePenalty?

That sort of thing might be a useful hint for the next person to come across the code. But perhaps this should be well known.

These values are primarily pass though and set in the client code that calls these apis. That allows the clients to fine tune the results as they are updated more frequently.

It's not true passthrough if we don't make sure this struct is always up to date - if we unmarshal passthrough requests into this struct, and then marshal this struct, any fields missing from the struct will be missing from the resulting JSON (similar to the bug you mentioned in https://github.com/sourcegraph/sourcegraph/pull/57872#discussion_r1370674508). Do we do that, or is this read-only? If it's not read-only we should definitely make sure docstrings linking to upstream definitions are tracked here

Passthough wasn't really the right wording, what I was trying to convey is that it's a mapping of these values from original client request, on the backend we are not opinionated regarding what the values should be. Added mapping details and links to openai docs

internal/completions/client/openai/openai.go

chrsmith · 2023-10-24T17:49:02Z

internal/conf/computed.go

 		// Set a default completions model.
 		if completionsConfig.CompletionModel == "" {
-			completionsConfig.CompletionModel = "gpt-3.5-turbo"
+			completionsConfig.CompletionModel = "gpt-3.5-turbo-instruct"


Similar to the comment earlier, would it make sense to have a constant or something for this? Or is this one function pretty much the only place we'll ever refer to the model by name/id?

internal/completions/client/openai/openai.go

Co-authored-by: Chris Smith <chrsmith@users.noreply.github.com>

chwarwick · 2023-10-24T19:59:08Z

tagged you all because we may bring this to the gateway to start supporting openai autocomplete.

Why not just bring it to Gateway straight away? BYOK and Gateway should be functionally the same, and most subscriptions aren't granted access to GPT models through Gateway right now anyway, and the ones that are granted access have already run into Cody Gateway rejected their requests for some time now anyway: 334 requests in the last 35 hours, 11k in the last week

@bobheadxi It's likely we will bring it soon, I do want to allow the extension changes to roll out first since it won't work well the the current version of the extensions.

…c infill mode (#1480) Previously the unstable-openai provider was based on the original anthropic prompt, this updates it to be similar to the newer anthropic infill mode and accompanying context improvements. ## Test plan Ran completions tooling & benchmarks against Azure OpenAI (GPT 3.5-Turbo) and OpenAI (GPT-3.5-instruct) Verified chat & autocomplete works with OpenAI and Azure OpenAI - OpenAI testing based on https://github.com/sourcegraph/sourcegraph/pull/57872

emidoots · 2023-10-25T19:35:01Z

internal/completions/client/openai/openai.go

+		if resp != nil {
+			resp.Body.Close()
+		}
+	})()


This is a pretty weird way to defer a Close operation; usually you just wait until the relevant code where you know for sure that resp != nil and then defer resp.Body.Close() directly there.

yeah we previously had that (in the azure version) and then the linter started complaining.

emidoots

Looks good!

(cherry picked from commit 6029d04)

#57931)

Chris Warwick added 2 commits October 24, 2023 10:42

update openai client for autocomplete

8f23509

update stream endpoint and computed openai values for autocomplete

aaae2a9

cla-bot bot added the cla-signed label Oct 24, 2023

chwarwick requested review from a team, bobheadxi, rafax and unknwon October 24, 2023 15:32

chwarwick mentioned this pull request Oct 24, 2023

Update openai autocomplete provider to use prompt similar to anthropic infill mode sourcegraph/cody-public-snapshot#1480

Merged

rafax requested a review from chrsmith October 24, 2023 15:58

chrsmith reviewed Oct 24, 2023

View reviewed changes

Chris Warwick and others added 2 commits October 24, 2023 14:44

Update internal/completions/client/openai/openai.go

3ce496d

Co-authored-by: Chris Smith <chrsmith@users.noreply.github.com>

fix typo from commit and document structs/funcs

623c9e6

emidoots reviewed Oct 25, 2023

View reviewed changes

emidoots approved these changes Oct 25, 2023

View reviewed changes

chwarwick merged commit 6029d04 into main Oct 25, 2023

chwarwick deleted the cw/open-ai-autocomplete branch October 25, 2023 20:05

chwarwick pushed a commit that referenced this pull request Oct 26, 2023

Cody: Experimental OpenAI autocomplete support (#57872)

ecff0e4

(cherry picked from commit 6029d04)

chwarwick added the backported-to-5.2 label Oct 26, 2023

unknwon pushed a commit that referenced this pull request Oct 30, 2023

[Backport 5.2] Cody: Experimental OpenAI autocomplete support (#57872) (

dd0375a

#57931)

varungandhi-src mentioned this pull request Jan 16, 2024

vg/matlab fix #59635

Closed

Conversation

chwarwick commented Oct 24, 2023

Test plan

Uh oh!

chwarwick commented Oct 24, 2023

Uh oh!

bobheadxi commented Oct 24, 2023

Uh oh!

chrsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobheadxi Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chwarwick commented Oct 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emidoots left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bobheadxi Oct 24, 2023 •

edited

Loading