-
Notifications
You must be signed in to change notification settings - Fork 937
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: DynamoDB Typeerror with AWS Bedrock #1163
base: main
Are you sure you want to change the base?
Conversation
Add functions to convert floats to decimal and decimals to float to fix TypeErrors causes when using ChatBedrock
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.
Thank you so much for this catch.
Co-authored-by: Mayaank Vadlamani <[email protected]>
Co-authored-by: Mayaank Vadlamani <[email protected]>
Diffs LGTM. Next step is to get it past pipeline and for @willydouhard / @tpatel / team to test and approve. |
Anything you need from me to progress this @mayaankvad ? |
No, I think the changes look good. Now it needs @willydouhard / teams approval |
Seems like mypy is failing @munday-tech |
Are you able to make this fix @munday-tech? |
Will get it done asap |
@willydouhard @mayaankvad should be good now. |
Any updates on this one? Looking forward to this being merged |
Looks ok to me. |
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.
Thanks!
I'm waiting for @willydouhard to approve #1288 and #1292 so we can start unit testing data layers.
Awaiting that, I'd love to have unit tests, doc strings and (if possible) types on your functions. If you're able to, please merge the aforementioned branches into yours and implement the unit tests for the functions you just implemented.
Thanks!
@@ -60,17 +61,49 @@ def __init__( | |||
def _get_current_timestamp(self) -> str: | |||
return datetime.now().isoformat() + "Z" | |||
|
|||
def _convert_floats_to_decimal(self, obj): |
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.
Could you please add type hints here so that we can use type checking to ensure that what we think this is doing is actually what it's doing?
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.
And a docstring, if possible.
|
||
return obj | ||
|
||
def _convert_decimal_to_floats(self, obj): |
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.
Type hints here as well please. 🙏🏼
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.
And a docstring, if possible.
I am waiting for this one to be merged |
Also waiting on this to be merged - would someone be able to add the required changes so it can be done soon please? |
When using AWS Bedrock,
_update_item
throws a TypeError as AWS Bedrock returns floats in its response. Dynamodb requires floats to be provided asDecimal
. The reverse can also occur if you callget_thread
from anywhere in code, asDecimal
is not JSON serializable.The issue can be seen here #1116
I have added two new functions within
DynamoDBDataLater
that converts floats to decimals and decimals to floats, calling each function from_update_item
andget_thread
respectively. This change resolves the TypeErrors.