Skip to content

Add support for streaming responses for Amazon Bedrock LLM#2367

Merged
Jacob Lee (jacoblee93) merged 11 commits intolangchain-ai:mainfrom
Benyuel:aws-bedrock-streaming
Aug 24, 2023
Merged

Add support for streaming responses for Amazon Bedrock LLM#2367
Jacob Lee (jacoblee93) merged 11 commits intolangchain-ai:mainfrom
Benyuel:aws-bedrock-streaming

Conversation

@Benyuel
Copy link
Copy Markdown
Contributor

@Benyuel Ben Pleasanton (Benyuel) commented Aug 22, 2023

This PR adds support for using the Amazon Bedrock LLM with streaming. See the added test for an example use.

Fixes #2321

@vercel
Copy link
Copy Markdown

vercel bot commented Aug 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
langchainjs-docs ✅ Ready (Inspect) Visit Preview Aug 24, 2023 11:16am

Comment thread langchain/package.json
@@ -590,9 +593,11 @@
"@aws-sdk/client-sagemaker-runtime": "^3.310.0",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great work on the PR! I've noticed that this diff introduces two new dependencies, "@aws-sdk/eventstream-marshaller" and "@aws-sdk/util-utf8-universal", which may be peer/dev/hard dependencies. I'm flagging this for maintainers to review.

Comment thread langchain/src/llms/bedrock-chat.ts Outdated
@@ -0,0 +1,267 @@
import { SignatureV4 } from "@aws-sdk/signature-v4";
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This comment is flagging the addition of a new HTTP request using the fetch function in the BedrockChat class, and it should be reviewed by maintainers.

Comment thread langchain/src/llms/bedrock-chat.ts Outdated
@@ -0,0 +1,267 @@
import { SignatureV4 } from "@aws-sdk/signature-v4";
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This PR adds a dependency on an environment variable AWS_DEFAULT_REGION via the getEnvironmentVariable function on line 97. Please review this change to ensure it is handled correctly.

@jacoblee93
Copy link
Copy Markdown
Collaborator

This looks really fantastic, but I don't think it really fits as a chat model (there's no dialog turns/messages, for example). The OpenAIChat class was really just a one-off when chat models were new.

We can just add streaming to the LLM version I think?

Comment thread langchain/src/llms/bedrock-chat.ts Outdated
Comment thread langchain/src/llms/bedrock-chat.ts Outdated
Comment thread langchain/src/llms/bedrock-chat.ts Outdated
);
}

if (this.streaming) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We now have a new interface for .stream():

https://js.langchain.com/docs/guides/expression_language/interface

Also, it'd be more elegant to always use the streaming call and implement non-streaming requests as just concatenating the entire stream:

https://github.com/hwchase17/langchainjs/blob/main/langchain/src/llms/ollama.ts#L124

@jacoblee93 Jacob Lee (jacoblee93) added the in progress PRs that are in progress but not ready to merge label Aug 23, 2023
@jacoblee93
Copy link
Copy Markdown
Collaborator

Let me know if you need help using the newer package! I'm happy to dig in tomorrow.

@Benyuel Ben Pleasanton (Benyuel) marked this pull request as draft August 23, 2023 10:24
@Benyuel Ben Pleasanton (Benyuel) changed the title Add Amazon Bedrock LLM chat model integration with streaming support Add support for streaming responses for Amazon Bedrock LLM Aug 23, 2023
@jacoblee93
Copy link
Copy Markdown
Collaborator

Thanks for the quick updates!

To get .stream() working you'll actually need to implement _streamResponseChunks as shown here:

https://github.com/hwchase17/langchainjs/blob/main/langchain/src/llms/openai.ts#L452

I can do this later today though - it should be very similar to what you have, and we can share the streaming logic.

@jacoblee93 Jacob Lee (jacoblee93) marked this pull request as ready for review August 23, 2023 22:00
@jacoblee93
Copy link
Copy Markdown
Collaborator

Ben Pleasanton (@Benyuel) Refactored a bit so we can support the .stream() method and added an integration test - can you confirm yarn test:single langchain/src/llms/tests/bedrock.int.test.ts works for you?

I also added retries and the ability to pass an abort signal to cancel in flight requests (hackily).

Also CC Vitaly Neyman (@vitaly-ps) for a second look?

@jacoblee93 Jacob Lee (jacoblee93) added question Further information is requested close PRs that need one or two touch-ups to be ready and removed in progress PRs that are in progress but not ready to merge labels Aug 23, 2023
@Benyuel
Copy link
Copy Markdown
Contributor Author

Thanks so much Jacob Lee (@jacoblee93)!

I made a minor change to get the tests working for me, and also added passing in temperature and maxTokens into the request.

I think this is ready from my perspective

I also added retries and the ability to pass an abort signal to cancel in flight requests

I tested this works as well 👍

chunks.push(chunk);
}
expect(chunks.length).toBeGreaterThan(1);
expect(chunks.length).toBeGreaterThanOrEqual(1);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be greater than 1?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

maxTokens: 20,
region,
model,
async fetchFn(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah I intended this one to test that this works with a live Bedrock instance, without a fetch function mock. Can you try that out?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oh sorry, you can revert the mock I added. I just assumed no connection to bedrock so mocked it.

To answer your question though, yes I tried this .stream against a live bedrock endpoint too.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok cool!

@jacoblee93
Copy link
Copy Markdown
Collaborator

Hey Ben Pleasanton (@Benyuel), I think you changed the test I added to use a mocked function - does this work with a live Bedrock instance?

return responseBody.completion;
} else if (provider === "ai21") {
return responseBody.completions[0].data.text;
return responseBody.data.text;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This was wrong before?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was correct before for the non-streaming bedrock api. But when I change to the bedrock invoke-with-response-stream endpoint, these change to what's in this PR currently.

@jacoblee93 Jacob Lee (jacoblee93) merged commit f23711f into langchain-ai:main Aug 24, 2023
@jacoblee93
Copy link
Copy Markdown
Collaborator

Looks great! Thanks again for your patience!

@uzaymacar
Copy link
Copy Markdown

Hey both, I've found this PR from one of the linked issues -- I'm still confused about how to implement streaming with Bedrock similar to what is shown in the docs: https://js.langchain.com/docs/modules/model_io/models/chat/how_to/streaming. Simply changing ChatOpenAI with ChatBedrock does not work, but maybe I need to do something else?

@jacoblee93
Copy link
Copy Markdown
Collaborator

Ah apologies, we really need to update that - instead use:

https://js.langchain.com/docs/expression_language/interface#stream

@uzaymacar
Copy link
Copy Markdown

Awesome thanks Jacob Lee (@jacoblee93), I'm now able to stream with Bedrock too 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

close PRs that need one or two touch-ups to be ready question Further information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for chat model and response streaming with Amazon Bedrock

3 participants