fix: serialization of nested ChatMessage in GeneratedAnswerdataclass#9497
fix: serialization of nested ChatMessage in GeneratedAnswerdataclass#9497
ChatMessage in GeneratedAnswerdataclass#9497Conversation
Amnah199
requested review from
dfokina and
julian-risch
and removed request for
a team
Pull Request Test Coverage Report for Build 15487358601Details
💛 - Coveralls |
julian-risch
left a comment
julian-risch
left a comment
There was a problem hiding this comment.
Looks quite good to me already. I have only minor change requests.
| "meta": {"meta_key": "meta_value"}, | ||
| "meta": { | ||
| "meta_key": "meta_value", | ||
| "all_messages": [ChatMessage.from_user("What is the answer?").to_dict()], |
There was a problem hiding this comment.
Just a comment: I thought about pros and cons of using ChatMessage.from_user("What is the answer?").to_dict() here instead of
{
"_role": "user",
"_meta": {},
"_name": None,
"_content": [
{
"text": "What is the answer?"
}
],
}I believe it's okay to use ChatMessage.from_user here because with this test, we don't want to test the to_dict representation of the ChatMessage and we don't want the test to fail if the to_dict method in future provides a different output. We do want to test that all_messages contains whatever ChatMessage.to_dict() returns, which is exactly what this test does. 👍
There was a problem hiding this comment.
Exactly and if ChatMessage serialization gets refactored we don't have to worry about updating these tests.
| ) | ||
| assert isinstance(answer, Answer) | ||
|
|
||
| def test_to_dict(self): |
There was a problem hiding this comment.
I suggest we keep the plain test_to_dict and test_from_dict tests that don't have any meta in the GeneratedAnswer too.
| return default_to_dict(self, data=self.data, query=self.query, documents=documents, meta=self.meta) | ||
|
|
||
| # Serialize ChatMessage objects to dicts | ||
| meta = self.meta.copy() |
There was a problem hiding this comment.
We don't need to copy if all_messages doesn't contain any ChatMessages, correct? Therefore we could move the copy call into the if block. We would need to change the if condition then.
There was a problem hiding this comment.
I made a shallow copy of meta inside the if block.
| meta = self.meta | ||
| all_messages = meta.get("all_messages") |
There was a problem hiding this comment.
@Amnah199 sorry this is a bit late, but we could have used _serializable_value from haystack.tracing.utils on the entire meta field. This way it would go in and try to serialize everything that has a to_dict method instead of making this specific to ChatMessage.
4 participants
Related Issues
GeneratedAnswerwhere nestedChatMessageobjects in themeta.all_messagescreated by AnswerBuilder were not being properly serialized/deserialized. This came up as a JSON serializable error while working on breakpoints.Snapshot of the case:
{'answer_builder': {'answers': [GeneratedAnswer(... meta={... 'all_messages': [ChatMessage(_role=<ChatRole.ASSISTANT: 'assistant'>, _content=[TextContent(text='The capital of France is Paris.')], _name=None, _meta={'model': ...}]}}Proposed Changes:
Updated the de/serialization of
GeneratedAnswerto properly handle nestedChatMessageobjects by ensuring they are converted to their dictionary representation.How did you test it?
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.