-
Notifications
You must be signed in to change notification settings - Fork 314
summarization manager - add summary prompt to messages #698
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
base: main
Are you sure you want to change the base?
Conversation
@@ -94,7 +95,7 @@ def restore_from_session(self, state: dict[str, Any]) -> Optional[list[Message]] | |||
""" | |||
super().restore_from_session(state) | |||
self._summary_message = state.get("summary_message") | |||
return [self._summary_message] if self._summary_message else None | |||
return [self._summary_prompt, self._summary_message] if self._summary_message else None |
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.
This is the one change I am somewhat worried about. The return type is Optional[list[Message]]
and so I feel it should be okay to include the summary prompt. Here is an example of how the method is used.
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.
Ohh this is clever! At first I didnt think this would work since we only store a single message in the conversation manager state (ref), but since this message is static it should be fine!
src/strands/agent/conversation_manager/summarizing_conversation_manager.py
Show resolved
Hide resolved
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.
Curious if you thought about just changing the role of the summary message to user?
I did think about this yes and actually tested with it. There were 2 (minor) concerns I had though:
Again these are minor concerns and so I could get behind just switching the role type. It would certainly simplify the PR. |
Description
Certain models (e.g., nova) require the messages array to start with a user message. The summarization manager however adds its summarization as an assistant message to the beginning of the array and thus is incompatible with certain models. To fix, I am adding the summarization prompt ("Please summarize this conversation") to the beginning of the array in front of the assistant summary message.
Related Issues
#611
Documentation PR
N/A
Type of Change
Bug fix
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
hatch run prepare
: Updated existing unit testshatch test tests_integ/test_summarizing_conversation_manager_integration.py
- Locally altered the tests to use the nova pro model to properly ensure fix works.
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.