-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[Fix] Graceful rejection of token input for AWS Embeddings API #1685
Conversation
This PR is being deployed to Railway 🚅 litellm: ◻️ REMOVED |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
litellm/llms/bedrock.py
Outdated
@@ -702,6 +702,11 @@ def _embedding_func_single( | |||
encoding=None, | |||
logging_obj=None, | |||
): | |||
if type(input) != str: |
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.
@rmann-nflx doing the check here as opposed to 810, because on 810 we'd have to loop through all elements in the list and assert none of them are str
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.
Makes sense -- much cleaner approach imho
model="amazon.titan-embed-text-v1", | ||
input=[1], | ||
) | ||
|
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.
added your unit test to our ci/cd @rmann-nflx
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.
Awesome!
litellm/llms/bedrock.py
Outdated
@@ -702,6 +702,11 @@ def _embedding_func_single( | |||
encoding=None, | |||
logging_obj=None, | |||
): | |||
if type(input) != str: |
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.
Heads up, I think this needs to be:
if isinstance(input, str) is False:
...
If you inherit from str
then this rejects when I think it would be valid for it to work:
Example:
class CustomStr(str):
pass
def test_lower_str():
x = CustomStr("x")
assert x == "x"
assert x != "y"
assert type(x) != str
assert isinstance(x, str)
This is a toy example, but I have seen stuff like this in the wild to constrain str to known patterns
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.
updated !
will merge once main is stable |
Bedrock Embeddings API for accepts only str | List[str].