-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Conversation
00f029e
to
ee23841
Compare
Hey @viveksilimkhan1, thanks for your PR! We'll give some feedback shortly, looks great at first glance. Quick question - is |
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. |
spacy_llm/models/bedrock/model.py
Outdated
TITAN_LITE = "amazon.titan-text-lite-v1" | ||
|
||
|
||
class Bedrock: |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
spacy_llm/models/bedrock/model.py
Outdated
|
||
def _request(json_data: str) -> str: | ||
try: | ||
import boto3 |
There was a problem hiding this comment.
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.
spacy_llm/models/bedrock/model.py
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
spacy_llm/models/bedrock/registry.py
Outdated
_DEFAULT_STOP_SEQUENCES: List[str] = [] | ||
|
||
|
||
@registry.llm_models("spacy.Bedrock.Titan.Express.v1") |
There was a problem hiding this comment.
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.
Closed in favour of #343 |
Description
Add support for Amazon Bedrock Titan models (#338) with a usage example
Corresponding documentation PR
Types of change
Support for Titan models
Checklist
tests
andusage_examples/tests
, and all new and existing tests passed. This includespytest
ran with--external
)pytest
ran with--gpu
)