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

Fix: DynamoDB Typeerror with AWS Bedrock #1163

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

Conversation

munday-tech
Copy link

When using AWS Bedrock, _update_item throws a TypeError as AWS Bedrock returns floats in its response. Dynamodb requires floats to be provided as Decimal. The reverse can also occur if you call get_thread from anywhere in code, as Decimal 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 and get_thread respectively. This change resolves the TypeErrors.

Add functions to convert floats to decimal and decimals to float to fix TypeErrors causes when using ChatBedrock
Copy link
Contributor

@mayaankvad mayaankvad left a 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.

backend/chainlit/data/dynamodb.py Outdated Show resolved Hide resolved
backend/chainlit/data/dynamodb.py Outdated Show resolved Hide resolved
backend/chainlit/data/dynamodb.py Outdated Show resolved Hide resolved
backend/chainlit/data/dynamodb.py Outdated Show resolved Hide resolved
@mayaankvad
Copy link
Contributor

mayaankvad commented Jul 28, 2024

Diffs LGTM. Next step is to get it past pipeline and for @willydouhard / @tpatel / team to test and approve.

@munday-tech
Copy link
Author

Anything you need from me to progress this @mayaankvad ?

@mayaankvad
Copy link
Contributor

No, I think the changes look good. Now it needs @willydouhard / teams approval

@willydouhard
Copy link
Collaborator

Seems like mypy is failing @munday-tech

@mayaankvad
Copy link
Contributor

Are you able to make this fix @munday-tech?

@munday-tech
Copy link
Author

Will get it done asap

@munday-tech
Copy link
Author

@willydouhard @mayaankvad should be good now.

@dokterbob dokterbob added backend Pertains to the Python backend. data layer Pertains to data layers. and removed backend Pertains to the Python backend. labels Aug 22, 2024
@antoniordz96
Copy link

Any updates on this one? Looking forward to this being merged

@mayaankvad
Copy link
Contributor

Looks ok to me.

@dokterbob dokterbob added the bug Something isn't working label Sep 3, 2024
Copy link
Collaborator

@dokterbob dokterbob left a 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):
Copy link
Collaborator

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?

Copy link
Collaborator

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):
Copy link
Collaborator

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. 🙏🏼

Copy link
Collaborator

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.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 25, 2024
@karthikvarmak1
Copy link

I am waiting for this one to be merged

@liv-watson
Copy link

liv-watson commented Oct 30, 2024

Also waiting on this to be merged - would someone be able to add the required changes so it can be done soon please?

@nestorcolt
Copy link

any news about this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working data layer Pertains to data layers. size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants