-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: refactor CoreMemory
to support generalized memory fields and memory editing functions
#1479
Conversation
Hi, I just wanted to show you @sarahwooders what I did in my MemGPT version to make the system prompt generation customizable. Similar to your MemoryModule, I introduced the SystemPromptModules, which are basically sections added to the system prompt. memory_prompt = """1. Core Memory - Stores essential context about the user, your persona and your current scratchpad, it is divided into a user section, a persona section and your scratchpad section. You can use the scratchpad to plan your next actions. You can edit the core memory by calling the functions: 'core_memory_append', 'core_memory_remove' and 'core_memory_replace'.
2. Archival Memory - Archive to store and retrieve general information and events about the user and your interactions with it. Can be used by calling the functions: 'archival_memory_search' and 'archival_memory_insert'.
3. Conversation History - Since you are only seeing the latest conversation history, you can search the rest of the conversation history. Search it by using: 'conversation_search' and 'conversation_search_date'.
Always remember that the user can't see your memory or your interactions with it!"""
memory_intro_section = SystemPromptModule(section_name="memory_intro",
prefix="To support you in your task as a AI assistant and to help you remembering things, you have access to 3 different types of memory.",
position=SystemPromptModulePosition.after_system_instructions)
memory_intro_section.set_content(memory_prompt) You can modify the content at runtime. |
Thanks for sharing this! We're planning to also modularize the system prompt sections of the context as well, but will do it in another PR to try to control the scope of this one. |
memgpt/agent.py
Outdated
@@ -912,146 +847,111 @@ def heartbeat_is_paused(self): | |||
|
|||
def rebuild_memory(self): | |||
"""Rebuilds the system message with the latest memory object""" | |||
print("rebuild memory") |
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 should remove these print
s
self.update_state() | ||
printd(msg) | ||
return msg | ||
# TODO: refactor |
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.
probably should raise NotImplementedError
instead?
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.
or raise a Warning
?
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.
import warnings
warnings.warn("...")
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 did a NotImplementedError
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.
Need to strip stray prints, will test in CLI now
Also should we be adding a migration script for this pre-release?
|
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.
LGTM
Thank you @Maximilian-Winter ! <3 |
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'd like to pull this into #1460 once it's merged and update the datamodel to match - I think this will really help clear up the configs too
|
||
class MemoryModule(BaseModel): |
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.
Looking at this section here I'm not entirely clear on what a MemoryModule
is vs a BaseMemory
, and by association, I'm not clear on what the attributes represent (so it's hard to follow along).
More unique names, descriptive class docstrings, and Field
annotations for attributes with descriptions would help.
For example, is the limit
attribute on MemoryModule
the number of characters in a Memory
, or the number of Memory
objects, or something else?
value
is another one - it almost looks like that's another subschema here. I'm not sure what values are compared to memories.
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.
The limit
is how many characters can be in the string representation of value
, since the string representation of value
is placed into the context window for the LLM.
We are actually planning to eventually store the MemoryModule
(or whatever its renamed to) as a DB row that is read from at inference time (when the context is "compiled"). This would allow multiple agents to share a given row (e.g. where value
represents organizational memory across agents). So maybe another potential name for the class could be StateVariable
or ContextVariable
.
|
||
class MemoryModule(BaseModel): |
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.
Module
is a reserved word in Python - modules are the things you import from files. MemoryModule
also semantically very close to Memory
which makes it harder to code and easier to mess up.
The general rule is if you find a whole bunch of things having the same or similar name ("Delivery", "ShipperDelivery", "VendorDelivery")
don't allow any of them to use the overused words. Then the names you are left with are distinct and you avoid confusion i.e. ("CustomerReceipt", "CarrierShipment", "VendorReceipt")
.
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 just used the term "Thought" below because it was what made sense when typing (memories are made up of thoughts) - I'm not suggesting that be the name, but I would recommend getting a nice wide Levenshtein distance between the names so it's easy to remember which is which when coding (and thinking)
self.human = human | ||
self.persona_char_limit = persona_char_limit | ||
self.human_char_limit = human_char_limit | ||
@validator("value", always=True) |
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 this is supposed to be defaulting to the class limit it should only be set there.
# affects the error message the AI will see on overflow inserts | ||
self.archival_memory_exists = archival_memory_exists | ||
# Check if the value exceeds the limit | ||
if isinstance(v, 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.
are these length values set in code downstream? or passed as instance values? if they are code this can just be a field
prop (as a value, then I agree this is probably the best/most readable option)
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.
what do you mean by the length values? Both the value and potentially the limit (though we don't support this now) could be modified for an existing agent.
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.
Sorry, I was thinking ahead of my words with code - the "limit" values, not "length" values 🤦🏻
basically, what I was trying to figure out by looking at the 2 classes is if one is a shell for the other and the limit is a default, or if the limit is the total that all the values must add up to. It's still not super clear (classic "naming things is the hardest part of programming" thing).
Going through this PR review has been helpful - I think I get most of the relationships now, the values are intended to be the actual string content of a given section/partial of core memory, and it can be a single string or many strings that get concatenated into a single utterance within the context window.
It would be solid to add this context into the objects (using class docstrings and Field
description attributes) - it doesn't need to be in this PR. That's a great "first issue" to take the knowledge from these discussions and instrument the code base.
return "" | ||
|
||
|
||
class BaseMemory: |
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.
how come this isn't a pydantic model?
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 guess it should be yeah... I forgot that they can also have class functions...
obj.memory[key] = MemoryModule(**value) | ||
return obj | ||
|
||
def __str__(self) -> 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.
It feels like there's a desired schema (it's implied) for memory objects - that would be a good thing to make another pydantic object and validate here.
return self.edit_human(new_content) | ||
else: | ||
raise KeyError(f'No memory section named {field} (must be either "persona" or "human")') | ||
def core_memory_append(self, name: str, content: str) -> Optional[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.
How come core_memory_apend
is in the child method and not the base method?
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.
The docstrings are specific to the ChatMemory
class (they mention human/persona) so would need to be overriden by customized memory.
|
||
def __init__(self, persona: str, human: str, limit: int = 2000): | ||
self.memory = { | ||
"persona": MemoryModule(name="persona", value=persona, limit=limit), |
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.
Ahh ok, so is a MemoryModule
the contents of a section within a BaseMemory
?
If I can make a suggestion, the BaseMemory.memory structure is awkward as a dict of typed objects with duplicate props. It seems like it should be a list (since that also preserves order, in case you care about that).
Something like
>> ChatMemory.thoughts
[<class 'MemoryModule' persona 145>,
<class 'MemoryModule' human 492>,
<class 'MemoryModule' company 941>]
There's a data normalization issue with the duplication here, you can have this:
>> chat_memory.memory.persona
<class 'MemoryModule' human 1998> # This shouldn't be possible!
If it's about lookups, there's some easy metaprogramming we can do for that
def __getattr__(self, val):
try:
return next([m in self.thoughts if m.name == 'val'])
except StopIteration as e:
raise AttributeError(f'{self.__class__.__name__} has no attribute {val}') from e
which looks like this in practice
>> chat_memory.persona
<class 'MemoryModule' persona 2401>
>> chat_memory.three_laws
<class 'MemoryModule' three_laws 100>
>> chat_memory.hamburger
AttributeError: ChatMemory has no attribute 'hamburger'
there would need to be a little extra to enforce only a single MemoryModule of a given type, unless you want to be able to have more than one persona or human? worth a thought
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 the MemoryModule
basically represents a reserved section of the context (or at least allocation of a context up to the size limit
) for placing the actual text value of the section of memory - we will also probably generalize this eventually to also include the system prompt (not just core memory), and store things like pre-defined human/persona values or system prompts under the same schema.
We also considered Variable
, Snipped
, and ContextSlice
- Thoughts
overlaps with inner-thoughts which is very different so I think that might be confusing.
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.
^^^ bingo! if we can add this into the code even as a docstring, it'll help when the name is a little sticky.
So if a Memory
is the entire context window (like the core memory
computing paradigm),
are these slots
or registers
or MemoryPartials
...
But I have held this PR up enough, and perfect is the enemy of gsd. I vote roll with MemoryModule
until a more natural object name comes to mind, then migrate.
…memory editing functions (#1479) Co-authored-by: cpacker <[email protected]> Co-authored-by: Maximilian-Winter <[email protected]>
Please describe the purpose of this pull request.
This PR refactors the code to allow for customizable memory modules and deprecates usage of presets for agent creation. I also implemented functionality to match the
LocalClient
andRESTClient
to make testing easier.Generalized Memory Class
This PR introduces to create custom memory classes that have custom fields (must be either a
str
ofList[str]
) and custom memory editing functions, inspired by work in #895.Sections of core memory are defined by a
MemoryModule
class (corresponds to sections of the in-context memory)The default memory class is implemented on top of a base
BaseMemory
class:Improve Agent Creation
Agent creation is modified to take in a memory class instead of human/persona/preset:
Deprecated
human
,persona
andpreset
fields from agent creationHow to test
poetry run pytest -s tests/test_memory.py
Have you tested this PR?
Yes
Related issues or PRs
#895