Skip to content

fix: TypeError: Object of type datetime is not JSON serializable - #335

Open
Pat Nadolny (pnadolny13) wants to merge 2 commits into
langchain-ai:mainfrom
pnadolny13:main
Open

fix: TypeError: Object of type datetime is not JSON serializable#335
Pat Nadolny (pnadolny13) wants to merge 2 commits into
langchain-ai:mainfrom
pnadolny13:main

Conversation

@pnadolny13

Copy link
Copy Markdown

This defaults to string when serializing the input body. In particular it was having issues serializing datetime objects. They now get serialized to a string appropriately instead of causing an exception to be raised.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@pnadolny13

Pat Nadolny (pnadolny13) commented Jan 22, 2025

Copy link
Copy Markdown
Author

Pat Nadolny (@pnadolny13) Thanks!

Michael Chin (@michaelnchin) No problem! Happy to help 😄. I dont have merge access, are you able to merge this for me or what is the usual process?

@3coins

Copy link
Copy Markdown
Collaborator

Pat Nadolny (@pnadolny13)
Thanks for submitting this fix. Would it be better to limit the default to serializing datetime objects, rather than converting everything to string, some of which cannot be deserialized to JSON?

import datetime
import json

def default(o):
    if isinstance(o, (datetime.date, datetime.datetime)):
        return o.isoformat()

    raise TypeError(f'Cannot serialize object of {type(obj)}')

@pnadolny13

Copy link
Copy Markdown
Author

Would it be better to limit the default to serializing datetime objects

Piyush Jain (@3coins) I'm with that since it still solves the issue I'm seeing!

The downside is that users dont have control over this serialization so they may continue to run into these edge cases. I think their options would then be to either open another PR here to handle their case or first serialize then deserialize their messages to ensure theyre safe before passing them into this library to avoid exceptions. If thats the recommended approach then wouldnt it be better to not accept a list of dicts but instead push the serialization up to the user by accepting a list of message strings? To be fair I'm not very familiar with this code though so this is a bit off the cuff 😄 . I'd defer to you.

@michaelnchin

Copy link
Copy Markdown
Collaborator

...wouldn't it be better to not accept a list of dicts but instead push the serialization up to the user by accepting a list of message strings?

IMO, this would unnecessarily restrict the set of options for dealing with serialization issues. We should let the user decide how their input is modified to compensate when json.dumps() fails. Converting everything to string would be one of several possible solutions here.

Also, note that brute force converting everything to string is risky, as the input may be modified in a way that the model misinterprets it (relative to what the user actually wants). In this respect, using default=str to perform conversions on behalf of the user could potentially cause confusion.

@3coins Piyush Jain (3coins) removed this from the Milestone Jan 2025 milestone Feb 6, 2025
@edgarrmondragon

Edgar Ramírez Mondragón (edgarrmondragon) commented Feb 18, 2025

Copy link
Copy Markdown
Contributor

Piyush Jain (@3coins) what would say about giving langchain_aws.llms.bedrock.BedrockBase a json_encoder attribute that defaults to None and is passed where needed as json.dumps(..., cls=self.json_encoder)?

That way, it's up to the user how and what objects are serialized.

PS: I would prefer the solution to be more generic, but it's probably easier to make the change here and later generalize it upstream in langchain itself.

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.

4 participants