-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat: ChatPromptBuilder
copies entire ChatMessage
rather than copying content field only
#8317
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 10668756300Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
vblagoje
changed the title
feat: Initial implementation of ChatMessage copy and deepcopy
feat:Add ChatMessage copy and deepcopy
Sep 2, 2024
vblagoje
changed the title
feat:Add ChatMessage copy and deepcopy
feat: ChatPromptBuilder copies entire ChatMessage rather than copying content field only
Sep 2, 2024
vblagoje
changed the title
feat: ChatPromptBuilder copies entire ChatMessage rather than copying content field only
feat: Sep 2, 2024
ChatPromptBuilder
copies entire ChatMessage
rather than copying content field only
vblagoje
requested review from
dfokina and
silvanocerza
and removed request for
a team
September 2, 2024 14:12
silvanocerza
approved these changes
Sep 2, 2024
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.
Looks good 👍
Amnah199
pushed a commit
that referenced
this pull request
Sep 4, 2024
…ying content field only (#8317) * Initial implementation of ChatMessage copy and deepcopy * Add reno release note * Satisfy hawkeye * Remove copy and deepcopy, no need to complicate things * Add new reno note * Add unit test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why:
The main motivation for this PR is the need to ensure that all
ChatMessage
fields including meta dict keys/values are copied properly. In theChatPromptBuilder
when we render templates, currently, we just create copy of the content and create a new ChatMessage using that content instead of proper copy of all fields (including meta which might carry important contextual info). Because we don't do proper copies of theChatMessage
in this case we can't do things like prompt caching (flags in meta dict are currently lost) and perhaps some other important payloads users might store in meta.See this issue for additional context.
ChatMessage
meta fields inChatPromptBuilder
haystack-core-integrations#1011What:
How can it be used:
No visible changes to user, all modifications are internal to
ChatPromptBuilder
How did you test it:
test_run_with_meta
intest/components/builders/test_chat_prompt_builder.py