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

Added AmazonBedrockVectorizer class #1151

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dgallitelli
Copy link

There was no SentenceVectorizer that leveraged Amazon Bedrock. I've created one.

There was no SentenceVectorizer that leveraged Amazon Bedrock. I've created one.

# Initialize the Bedrock client
self.bedrock_client = boto3.client(
Copy link
Collaborator

Choose a reason for hiding this comment

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

could this potentially be replaced with the existing DSPy AWS model integrations? would be neat to tie it in!

Copy link
Author

Choose a reason for hiding this comment

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

I'm not exactly sure what you would like to see here to be honest 🤔

})
else:
raise Exception("How did you even get here?")
Copy link
Collaborator

Choose a reason for hiding this comment

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

would appreciate a more informative exception message :).

@@ -1,6 +1,6 @@
import abc
from typing import List, Optional

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.

Update with try/except block like in aws_providers.

@arnavsinghvi11
Copy link
Collaborator

Thanks @dgallitelli for the PR! left a few comments with the main change required in wrapping the import boto3 with a try/except block to pass the failing the tests.

Also, please run ruff check . --fix-only and push to pass the failing ruff test.

@dgallitelli
Copy link
Author

Thanks @arnavsinghvi11 for your comments! I've addressed the wrapping of boto3 and the more explicit error messaging, and pushed the updates to the branch. Also, I've run the ruff test, with the following output:

warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'extend-unsafe-fixes' -> 'lint.extend-unsafe-fixes'
warning: Docstring at dspy/teleprompt/mipro_optimizer.py:111:9 contains implicit string concatenation; ignoring...
warning: Docstring at dspy/teleprompt/mipro_optimizer.py:122:9 contains implicit string concatenation; ignoring...

This makes me think that all is good so far.

I'm not sure your point about the existing DSPy AWS model integrations. What would you be looking for here? A new AWSBedrockEmbedding class? Just switching the boto3 client to the DSPy AWSProvider? I think that can be a separate Issue and PR.

@arnavsinghvi11
Copy link
Collaborator

Thanks @dgallitelli . Yes I was curious if the boto3 client could be switched to use the supported DSPy AWS providers. Would be a seamless way to tie in the DSPy library together.

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.

None yet

2 participants