-
Notifications
You must be signed in to change notification settings - Fork 192
Adding support for Bedrock Agentcore Memory Tools #600
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?
Adding support for Bedrock Agentcore Memory Tools #600
Conversation
"\n", | ||
"AgentCore short-term memories are organized by a Memory ID (overall memory store) and then categorized by Actor ID (i.e which user) and Session ID (which chat session). \n", | ||
"\n", | ||
"Long term memories are stored in namespaces. By configuring different strategies (i.e. Summarization, User Preferences) these short term memories are processed async as long term memories in the specified namespace. We will create a separate search tool for each namespace to differentiate between `UserPreferences` and `Summaries`. The tool factories ensure that the LLM will only have to worry about sending the query and limit for searching memories and not worry about any of the configuration IDs." |
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.
"Long term memories are stored in namespaces (https://docs.aws.amazon.com/bedrock-agentcore/latest/devguide/session-actor-namespace.html)"
that is, it would be great to link to that so the user has some clarification on what a namespace is, which I think is our most difficult concept
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.
"... not worry about any of the configuration IDs." does "configuration IDs" refer to Actor, Memory and Session? if so, I think it would be good to say "... not recall the actor, session, and memory IDs."
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.
"... these short term memories are processed async as long term memories... " --> "... these short term memories (i.e., conversation history) are asynchronously converted into long-term memories (i.e., memory records)..."
@@ -0,0 +1,314 @@ | |||
{ | |||
"cells": [ |
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.
should this example also explain how the events get into short-term memory (i.e. conversation history)? As the example is, I am confused how the memories are retrieved because I don't know how the short-term memory was created that enabled extraction of memory records in long-term memory
"source": [ | ||
"## Pre/Post Model Hooks\n", | ||
"\n", | ||
"For this implementation, short term memories (message events) are stored before and after model invocation. Before the model runs, the previous user message is saved. After the model runs, the LLM message is saved. \n", |
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.
"For this implementation, short term memories (i.e., conversation history) are stored..."
I think it's best toe refer to short term memory as conversation history, so it's clear that we're offering hosted conversation history
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.
"Before the model runs, the previous user message is saved. After the model runs, the LLM message is saved"
would it be possible to async CreateEvent during the LLM invocation so that the client application doesnt have any extra time to first LLM byte? And then maybe also make it async to save the LLM event... but I think that's less important
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.
also, I am making those comments assuming it will affect the actual integration code, not just this example code. Please let me know if that is wrong
" \n", | ||
" context_content += \"\"\"\n", | ||
" \n", | ||
" === END HISTORY ===\n", | ||
" \n", | ||
" Please use this context to continue our conversation naturally. You should reference relevant parts of this history when appropriate, but don't explicitly mention that you're loading from memory unless asked.\n", | ||
" \"\"\"\n", | ||
" # Create a special system message with the previous context\n", |
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.
Does it make the most sense to do this as a system prompt? Or would it be better to load this into the messages array that is sent to the LLM to resemble the conversation history? (Or does LangChain not allow that?)
if not memory_id or not memory_id.strip(): | ||
raise ValueError("memory_id cannot be empty") | ||
if not actor_id or not actor_id.strip(): | ||
raise ValueError("actor_id cannot be empty") | ||
if not session_id or not session_id.strip(): | ||
raise ValueError("session_id cannot be empty") |
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.
should this be a function? It is repeated a lot
def retrieve_agentcore_memories( | ||
memory_client: MemoryClient, | ||
memory_id: str, | ||
namespace_str: str, |
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.
should the function take an optional score_threshold
to specify the required score for a memory to be returned?
return event_id | ||
|
||
|
||
def list_agentcore_memory_events( |
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.
is it possible to paginate over events if the user wants more than the latest 100 events?
if not session_id or not session_id.strip(): | ||
raise ValueError("session_id cannot be empty") | ||
|
||
events_to_store = convert_langchain_messages_to_events( |
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.
nit: we would call these AgentCore event messages
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.
that is, an event takes an array of messages
continue | ||
|
||
message = None | ||
if role == "USER": |
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.
since these role strings are repeated, should they be defined as variables and then referenced?
Args: | ||
memory_client: The AgentCore memory client | ||
memory_id: The memory identifier in AgentCore | ||
namespace_str: The namespace to be searched |
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.
should there be a method / some way of resolving the namespace returned by get_memory?
It returns the memory in the format: /strategies/{memoryStrategyId}/actors/{actorId}
@@ -0,0 +1,99 @@ | |||
from unittest.mock import Mock |
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.
nit: can we make the naming consistent (use bedrock agentcore everywhere instead of agentcore in some places and bedrock agent core in others?
This PR is based on @3coins WIP draft here: #559 and adds support for Bedrock Agentcore Memory operations by providing both helper functions and tool factories for the following operations:
store_agentcore_memory_events
list_agentcore_memory_events
retrieve_agentcore_memories
Major Differences
BaseMessage
and Agentcore MemoryEvents
Sample Usage
For each helper function, a tool factory is provided that can be provided to the LLM. These each depend on a pre-created Memory See CreateMemory docs with strategies defined such as
Summarization
orUserPreferences
.Then once these values are defined at the start of the flow, the helper functions can be used during pre/post model hooks like in the first notebook example, or used as individual tools themselves as shown below (and in the second notebook example). Each tool factory returns a LangChain
StructuredTool
:Caveats/Decisions
ToolMessages
only the content can be stored as of now, not thetool_call_id
. A TODO was left to add support for storing this attribute in the future once metadata is supported, but for now the value is just retrieved asunknown
for tool_id.'ASSISTANT'|'USER'|'TOOL'|'OTHER'
as message types, so LangChainSystemMessage
do not have a direct correlation. For implementation, I'm not sure exactly when a user would want to save aSystemMessage
for short term memory but it could provide important context in the event of a flow restarting. For that reason, includingSystemMessage
is enabled through the a flaginclude_system_messages
in thestore_agentcore_memory_events
helper function, and optionally saves those messages asOTHER
type.branch
filters in the ListEvents API. This will need to be added as a follow up.