-
Notifications
You must be signed in to change notification settings - Fork 54
Feat: Improve mem0 memory tool to support storing short-term memories #96
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
""" | ||
try: | ||
from opensearchpy import AWSV4SignerAuth, RequestsHttpConnection |
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.
I had trouble installing faiss
before, so I could understand that we didnt include it as a top level import, but I dont see anything wrong with including this at the top level. We have it specified in the mem_0 memory optional dependencies here: https://github.com/strands-agents/tools/blob/main/pyproject.toml#L77
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.
Yeah, I was thinking that we can make mem0 as a default dependency in the tools repo and make faiss and opensearch as optional so that anyone who only wants to use the mem0 platform, then they don't have to install faiss or opensearch. Let me know what you think.
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.
If someone wants to use the mem0 tool, they should be able to use this optional dependency, and have the tool just work. I had trouble installing faiss, so im guessing others might as well, which is why im fine leaving that out with a runtime check. But for the other two, we should just include them in the optional dependency on mem0.
Since we already have this optional dependency for mem0, i'm hesitant to remove it, and I dont think it makes sense to move the mem0 dependency to a default if we already have this optional dependency.
"4. Delete memories\n" | ||
"5. Get memory history\n\n" | ||
"Actions:\n" | ||
"- store: Store new memory (requires user_id or agent_id)\n" | ||
"- store: Store new memory (requires user_id or agent_id or run_id)\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.
Nit: Any reason to write this as (requires user_id or agent_id or run_id)
instead of requires user_id, agent_id, or run_id
? Not sure if there is some LLM preference here? The second option seems more readable
Description
This PR adds support for storing the short term memories for agents in the
mem0_memory
tool for managing memories (store, retrieve, list, delete, and history) using Mem0 as the backend.Type of Change
Testing
Tested using an example agent here: https://gist.github.com/deshraj/307de1c3385269dda5521edf46567ba0
Checklist