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

feat: add Amazon Bedrock support #6226

Merged
merged 44 commits into from
Nov 15, 2023
Merged

Conversation

viveksilimkhan1
Copy link
Contributor

@viveksilimkhan1 viveksilimkhan1 commented Nov 3, 2023

Related Issues

Proposed Changes:

Added support for Amazon Bedrock Models

  • Added AmazonBedrockInvocationLayer
  • All currently available models on Bedrock are supported using model adapters
  • AWS common functionality with Sagemaker was refactored into AWSBaseInvocationLayer
  • streaming is supported for all models that support streaming

How did you test it?

  • added lots of tests
  • tested manually all models except Amazon Titan as they are currently unavailable

Notes for the reviewer

Checklist

@viveksilimkhan1 viveksilimkhan1 requested a review from a team as a code owner November 3, 2023 07:28
@viveksilimkhan1 viveksilimkhan1 requested review from anakin87 and removed request for a team November 3, 2023 07:28
@CLAassistant
Copy link

CLAassistant commented Nov 3, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the type:documentation Improvements on the docs label Nov 3, 2023
@tstadel
Copy link
Member

tstadel commented Nov 6, 2023

Thanks @viveksilimkhan1 for this awesome contribution. Could you please sign the CLA?

As we want to get this as fast as possible into haystack, I will support you in writing tests and pushing it over the finish line. Hope that's ok.

Copy link
Member

@tstadel tstadel left a comment

Choose a reason for hiding this comment

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

I left some comments. Functionality-wise we have to:

  • add anthropic claude support
  • support streaming
  • request supported models from bedrock
  • use DefaultPromptHandler to implement _ensure_token_limit

Quality-wise we have to:

  • implement tests
  • improve the params validation

@tstadel
Copy link
Member

tstadel commented Nov 6, 2023

I left some comments. Functionality-wise we have to:

  • add anthropic claude support
  • support streaming
  • request supported models from bedrock

Quality-wise we have to:

  • implement tests
  • improve the params validation

@viveksilimkhan1 please let me know if you find time to tackle one or more of those points in the next two days. Otherwise I'm happy to jump on it. :-)

haystack/errors.py Outdated Show resolved Hide resolved
@tstadel tstadel changed the title Add Amazon Bedrock support feat: add Amazon Bedrock support Nov 7, 2023
@tstadel
Copy link
Member

tstadel commented Nov 7, 2023

@viveksilimkhan1 I did some refactorings and improved the supports method a bit. Even though we are checking against the available models on Bedrock, we should check against a static list for which we provide correct handling of inputs and outputs.

I will continue writing tests for instantiation and supports. If you like, you can continue improving and completing the invoke method, e.g.:

  • refactor common expressions
  • support all params for existing implementations
  • support claude models

Also make sure to install the precommit hooks running pre-commit install. Thx! 👍

@tstadel tstadel requested a review from a team as a code owner November 14, 2023 16:46
@tstadel
Copy link
Member

tstadel commented Nov 14, 2023

@anakin87 this PR is good to go from my side. Feel free to have a look.

**/*.py
files_ignore: |
test/**
rest_api/test/**
haystack/preview/**/*.py
Copy link
Member

Choose a reason for hiding this comment

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

Misconfig tried to lint my changes and failed because of missing dependencies that are not available to in preview.

haystack/nodes/prompt/invocation_layer/amazon_bedrock.py Outdated Show resolved Hide resolved
haystack/nodes/prompt/invocation_layer/amazon_bedrock.py Outdated Show resolved Hide resolved
haystack/nodes/prompt/invocation_layer/amazon_bedrock.py Outdated Show resolved Hide resolved
haystack/nodes/prompt/invocation_layer/amazon_bedrock.py Outdated Show resolved Hide resolved
haystack/nodes/prompt/invocation_layer/amazon_bedrock.py Outdated Show resolved Hide resolved
haystack/nodes/prompt/invocation_layer/aws_base.py Outdated Show resolved Hide resolved
releasenotes/notes/bedrock-support-bce28e3078c85c12.yaml Outdated Show resolved Hide resolved
releasenotes/notes/bedrock-support-bce28e3078c85c12.yaml Outdated Show resolved Hide resolved
@anakin87
Copy link
Member

@tstadel I had a quick look and it looks good to me...

If you haven't already done so, I would ask you to play around with these changes locally a bit.

In particular, I would check if the PromptNode still detects the appropriate invocation layers
(for example, you can try a local HF model and and another one hosted on SageMaker).
With respect to this PR, there should be no problem, but sometimes seemingly unrelated changes have broken this (fragile) mechanism.

@tstadel
Copy link
Member

tstadel commented Nov 15, 2023

@tstadel I had a quick look and it looks good to me...

If you haven't already done so, I would ask you to play around with these changes locally a bit.

In particular, I would check if the PromptNode still detects the appropriate invocation layers (for example, you can try a local HF model and and another one hosted on SageMaker). With respect to this PR, there should be no problem, but sometimes seemingly unrelated changes have broken this (fragile) mechanism.

So I've tested the following manually:

  • test streaming for all supported Bedrock models (found a bug for cohere and fixed it)
  • load and prompt a local HF model, that is PromptNode("google/flan-t5-base")
  • load and prompt a Meta Sagemaker model (llama-2-7b-chat)
  • load and prompt a non-Meta Sagemaker model (mistral-7b-instruct)

I'll finish the last one in the meantime. @anakin87 would you have one final look?

Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

👍

@tstadel tstadel added type:feature New feature or request topic:LLM labels Nov 15, 2023
@tstadel
Copy link
Member

tstadel commented Nov 15, 2023

@viveksilimkhan1 thanks a lot for this contribution 🚀 ! I had to change a few things regarding streaming support, but now we're good to merge.

@tstadel tstadel merged commit f998bf4 into deepset-ai:main Nov 15, 2023
64 checks passed
vblagoje pushed a commit that referenced this pull request Nov 22, 2023
* Add Bedrock

* Update supported models for Bedrock

* Fix supports and add extract response in Bedrock

* fix errors imports

* improve and refactor supports

* fix install

* fix mypy

* fix pylint

* fix existing tests

* Added Anthropic Bedrock

* fix tests

* fix sagemaker tests

* add default prompt handler, constructor and supports tests

* more tests

* invoke refactoring

* refactor model_kwargs

* fix mypy

* lstrip responses

* Add streaming support

* bump boto3 version

* add class docstrings, better exception names

* fix layer name

* add tests for anthropic and cohere model adapters

* update cohere params

* update ai21 args and add tests

* support cohere command light model

* add tital tests

* better class names

* support meta llama 2 model

* fix streaming support

* more future-proof model adapter selection

* fix import

* fix mypy

* fix pylint for preview

* add tests for streaming

* add release notes

* Apply suggestions from code review

Co-authored-by: Agnieszka Marzec <[email protected]>

* fix format

* fix tests after msg changes

* fix streaming for cohere

---------

Co-authored-by: tstadel <[email protected]>
Co-authored-by: tstadel <[email protected]>
Co-authored-by: Agnieszka Marzec <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Amazon Bedrock support
7 participants