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

feat: ChatPromptBuilder copies entire ChatMessage rather than copying content field only #8317

Merged
merged 6 commits into from
Sep 2, 2024

Conversation

vblagoje
Copy link
Member

@vblagoje vblagoje commented Sep 2, 2024

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 the ChatPromptBuilder 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 the ChatMessage 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.

What:

  • Modified the chat prompt builder logic to utilize deep copy for message processing, replacing the previous method of selectively copying attributes and creating new message instances based on the content and origin (user/system).

How can it be used:

No visible changes to user, all modifications are internal to ChatPromptBuilder

How did you test it:

  • Uses exhaustive existing tests
  • Adds a new test_run_with_meta in test/components/builders/test_chat_prompt_builder.py

@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Sep 2, 2024
@coveralls
Copy link
Collaborator

coveralls commented Sep 2, 2024

Pull Request Test Coverage Report for Build 10668756300

Warning: 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

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.002%) to 90.198%

Files with Coverage Reduction New Missed Lines %
components/builders/chat_prompt_builder.py 1 98.51%
Totals Coverage Status
Change from base Build 10664748045: 0.002%
Covered Lines: 7003
Relevant Lines: 7764

💛 - Coveralls

@vblagoje vblagoje changed the title feat: Initial implementation of ChatMessage copy and deepcopy feat:Add ChatMessage copy and deepcopy Sep 2, 2024
@vblagoje 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 vblagoje changed the title feat: ChatPromptBuilder copies entire ChatMessage rather than copying content field only feat: ChatPromptBuilder copies entire ChatMessage rather than copying content field only Sep 2, 2024
@vblagoje vblagoje marked this pull request as ready for review September 2, 2024 14:12
@vblagoje vblagoje requested review from a team as code owners September 2, 2024 14:12
@vblagoje vblagoje requested review from dfokina and silvanocerza and removed request for a team September 2, 2024 14:12
Copy link
Contributor

@silvanocerza silvanocerza left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@vblagoje vblagoje merged commit b2c19a8 into main Sep 2, 2024
30 checks passed
@vblagoje vblagoje deleted the chat_msg_copy branch September 2, 2024 16:06
@silvanocerza silvanocerza added this to the 2.5.0 milestone Sep 3, 2024
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
Labels
topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy ChatMessage meta fields in ChatPromptBuilder
3 participants