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

Add support for Amazon Bedrock Titan models #339

Closed
wants to merge 0 commits into from

Conversation

viveksilimkhan1
Copy link
Contributor

@viveksilimkhan1 viveksilimkhan1 commented Oct 24, 2023

Description

Add support for Amazon Bedrock Titan models (#338) with a usage example

Corresponding documentation PR

Types of change

Support for Titan models

Checklist

  • I confirm that I have the right to submit this contribution under the project's MIT license.
  • I ran all tests in tests and usage_examples/tests, and all new and existing tests passed. This includes
    • all external tests (i. e. pytest ran with --external)
    • all tests requiring a GPU (i. e. pytest ran with --gpu)
  • I've added all required information about usage in the example.

@shadeMe shadeMe added feat/model Feature: models feat/new New feature labels Oct 25, 2023
@viveksilimkhan1 viveksilimkhan1 marked this pull request as draft October 27, 2023 05:01
@viveksilimkhan1 viveksilimkhan1 marked this pull request as ready for review October 27, 2023 06:15
@rmitsch
Copy link
Collaborator

rmitsch commented Oct 27, 2023

Hey @viveksilimkhan1, thanks for your PR! We'll give some feedback shortly, looks great at first glance. Quick question - is boto3 a hard requirement for the Bedrock API? I. e. there is no way to make this work using the native Python REST libraries?

@viveksilimkhan1
Copy link
Contributor Author

viveksilimkhan1 commented Oct 27, 2023

Hi @rmitsch, as far as I know, boto3 is the recommended way for using Bedrock services in python, and since boto3 is maintained by AWS, any updates in the API in future would require minimal changes. I am not sure how this can be implemented with native REST libraries.

TITAN_LITE = "amazon.titan-text-lite-v1"


class Bedrock:
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency we'd like all of the REST models to inherit from the REST class. Could you modify your Bedrock class to do so? You can have a look at any of the other REST-based model classes in spacy_llm/models/rest for inspiration (particularly the azure directory in particular, as it's a rather similar situation).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure @rmitsch, will do it.


def _request(json_data: str) -> str:
try:
import boto3
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it's ok to expect boto3 when using Bedrock models. It seems to be tedious to work around it otherwise.

Comment on lines 87 to 97
contentType = "application/json"
r = bedrock.invoke_model(
body=json_data,
modelId=self._model_id,
accept=accept,
contentType=contentType,
)
responses = json.loads(r["body"].read().decode())["results"][0][
"outputText"
]
return responses
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no error processing here. Could you add some safeguards for common errors? Auth errors etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll add them

_DEFAULT_STOP_SEQUENCES: List[str] = []


@registry.llm_models("spacy.Bedrock.Titan.Express.v1")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Titan Express and Titan Lite are quite similar in their usage, so let's merge this to one registry entry.

@svlandeg
Copy link
Member

Closed in favour of #343

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat/model Feature: models feat/new New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants