Skip to content
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

fix(chat): support non-streaming chat completion requests #5565

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

abeatrix
Copy link
Contributor

@abeatrix abeatrix commented Sep 13, 2024

Follow up on #5508
CLOSE https://linear.app/sourcegraph/issue/CODY-3741/oai-o1-hits-parsing-error

This change adds support for non-streaming requests, particularly for new OpenAI models that do not support streaming. It also fixes a "Parse Error: JS exception" issue that occurred when handling non-streaming responses in the streaming client.

  • Remove HACK for handling non-streaming requests in the stream method in nodeClient.ts that's resulting in parsing error
  • Implement a new _fetchWithCallbacks method in the SourcegraphCompletionsClient class to handle non-streaming requests
  • Update the SourcegraphNodeCompletionsClient and SourcegraphBrowserCompletionsClient classes to use the new _fetchWithCallbacks method when params.stream is false
  • This change ensures that non-streaming requests, such as those for the new OpenAI models that do not support streaming, are handled correctly and avoid issues like "Parse Error: JS exception" that can occur when the response is not parsed correctly in the streaming client

The SourcegraphCompletionsClient class now has a new abstract method _fetchWithCallbacks that must be implemented by subclasses

Test plan

Verify that chat requests using non-streaming models (e.g., OpenAI O1, OpenAI O1-mini) work correctly without any parsing errors. Example: ask Cody about cody repo with "cody what are the latest updates in this repo?"

After

First try

image

Second try

image

Before

First try

image

Changelog

feat(chat): support non-streaming requests

This change adds support for non-streaming requests, particularly for new OpenAI models that do not support streaming. It also fixes a "Parse Error: JS exception" issue that occurred when handling non-streaming responses in the streaming client.

- Remove HACK for non-streaming requests in nodeClient.ts
- Implement a new `_fetchWithCallbacks` method in the `SourcegraphCompletionsClient` class to handle non-streaming requests
- Update the `SourcegraphNodeCompletionsClient` and `SourcegraphBrowserCompletionsClient` classes to use the new `_fetchWithCallbacks` method when `params.stream` is `false`
- This change ensures that non-streaming requests, such as those for the new OpenAI models that do not support streaming, are handled correctly and avoid issues like "Parse Error: JS exception" that can occur when the response is not parsed correctly in the streaming client

The `SourcegraphCompletionsClient` class now has a new abstract method `_fetchWithCallbacks` that must be implemented by subclasses
@abeatrix abeatrix requested review from valerybugakov, slimsag and a team September 13, 2024 07:30
@abeatrix abeatrix changed the title feat(chat): support non-streaming requests feat(chat): support non-streaming chat completion requests Sep 13, 2024
@abeatrix abeatrix changed the title feat(chat): support non-streaming chat completion requests fix(chat): support non-streaming chat completion requests Sep 13, 2024
Copy link
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

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

Nice! Tested locally ✅

vscode/CHANGELOG.md Outdated Show resolved Hide resolved
vscode/src/completions/nodeClient.ts Show resolved Hide resolved
lib/shared/src/sourcegraph-api/completions/client.ts Outdated Show resolved Hide resolved
@abeatrix abeatrix enabled auto-merge (squash) September 13, 2024 08:09
@abeatrix abeatrix merged commit 24192e4 into main Sep 13, 2024
19 checks passed
@abeatrix abeatrix deleted the bee/non-stream-client branch September 13, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants