fix: Memory validation fix + core_memory_replace runaway content repeating fix #1616
Conversation
…ation for the changes fails.
…causes an exponential amount of repeating data attempting to be saved.
|
I'm having the AI test the function with me throwing a ValueError when old_content is "". Works as intended but then it's a little more complex to have it do a full replace if it wanted. But making that too easy is also kind of scary. What are you guys' thoughts on the logic design of this situation? I can update PR if it's straightforward. Could do value error and then I'd just make a custom function if I wanted to replace. Which I don't really, just testing the empty old_content issue. |
sarahwooders
changed the title
memgpt/memory.py
Outdated
| @@ -124,7 +126,10 @@ def core_memory_replace(self, name: str, old_content: str, new_content: str) -> | |||
| Returns: | |||
| Optional[str]: None is always returned as this function does not produce a response. | |||
| """ | |||
| self.memory[name].value = self.memory[name].value.replace(old_content, new_content) | |||
| if old_content == "": | |||
| self.memory[name].value = new_content | |||
There was a problem hiding this comment.
@Vandracon is this logic correct?
Doesn't this mean if the agent calls core_memory_replace("", "hello"), it'll wipe all of core memory and replace it with "hello"?
There was a problem hiding this comment.
Yeah, I wasn't sure of the intended design since I was working to get it not making a huge string. After thinking about it some more, throwing a validation error makes more sense and making the AI less capable of emptying their core memory is a better approach. I just pushed an update that reflects this and can confirm it returns an error to the AI if they try to send old_content = "".
There was a problem hiding this comment.
Also, I think it needs a sqlite migration to repair the function in the database for existing agents. Was less comfortable making one for sqlite / postgres / idk what else. But even without that, this fixes two things.
…t string was empty.
|
Anything new with this? For my use case, I haven't run into any new blockers since I fixed the issue. |
| if name == "value": | ||
| # run validation | ||
| self.__class__.validate(self.dict(exclude_unset=True)) | ||
| # Temporarily set the attribute to run validation |
There was a problem hiding this comment.
I'm a bit confused by what this code - what is it doing?
There was a problem hiding this comment.
Creates a copy of itself and runs validation on the value. Otherwise, even if the validation fails it still modifies the value and will go beyond the 2000 (or configured) limit.
| @@ -124,7 +126,10 @@ def core_memory_replace(self, name: str, old_content: str, new_content: str) -> | |||
| Returns: | |||
| Optional[str]: None is always returned as this function does not produce a response. | |||
| """ | |||
| self.memory[name].value = self.memory[name].value.replace(old_content, new_content) | |||
| if old_content == "": | |||
There was a problem hiding this comment.
Maybe give it a hint like "Use core_memory_append to add new content without replacing any existing content." to give to the model?
There was a problem hiding this comment.
I updated the ValueError that it gets
|
HI sorry for the delay - I added a few comments. Could you also please resolve the merge conflicts? |
…ile. Adds local to poetry install to support local models.
…eep hitting LLM if it can't summarize or cause an error. This was made to handle a problem caused by core memories being able to save beyond their limits.
…oviding old_content.
Please describe the purpose of this pull request.
Is it to add a new feature? Is it to fix a bug?
Fixes bug (Also see note at bottom)
How to test
How can we test your PR during review? What commands should we run? What outcomes should we expect?
Make agent core memory (human or persona be 1999 characters with a 2000 limit) Ask agent to save something more to the memory. If it does actually do it, the validation and function call would fail but the data was still applied to the agent. On next agent load it will throw an error because the memory is over the limit.
For next item, just have the AI update a memory block with core_memory_replace and have it send "" for old content. It will replace the text with a huge blob (size based on # characters on the existing content in that bank)
Have you tested this PR?
Have you tested the latest commit on the PR? If so please provide outputs from your tests.
I am now able to trigger a save, function fail, and then the length remained at (1999).
I was also able to have AI repair its own persona by replacing it entirely with old_content = "" and new_content = "lorem ipsum etc".
Related issues or PRs
Please link any related GitHub issues or PRs.
Is your PR over 500 lines of code?
If so, please break up your PR into multiple smaller PRs so that we can review them quickly, or provide justification for its length.
Additional context
Add any other context or screenshots about the PR here.
Idk if the validation system being used normally recommends set_attr to be overridden like it is but I'm not an expert on the lib being used. I needed to move super call to be after the validation, and to run validation on a copy so the old value is reserved if it fails (exits before super called).
[Action item #1] - Also, there was a bug with core_memory_replace. If the AI sends a "" for old content, it will replace every existing character with the new_content value. This leads to trying to save repeating text in the hundreds of thousands of characters long (depending on the existing length of old_content on agent already. I updated the function in code to replace all if no old content was specified. I don't know if that's ideal per design or not.. if not then please update this PR with a validation rule so the AI can at least be aware of an issue.
[Action item #2] - My sqlite database toolmodel table for core_memory_append and replace referenced the memory as self.memory.memory[name] = ... but the code I edited in this PR would only apply to new agents (I believe) So there may need to be a migration script added to this PR to repair existing agents if you guys deem the code update necessary. Also, the code I edited for the append referenced memory as self.memory[name].value instead of self.memory.memory[name].value. I'm not sure of the reasons why there is a difference but memory.memory (in the db) is what works for me.
I added the two action items above for some minor help/adjustments to this PR if needed. I fixed this to get past some blockers I had using local llms and updated the tool in my own DB but the migration stuff I want to leave to you guys. I assume it's sqlite, postgres, etc or something. Overall I think this contains most of a critical fix. Someone else in the chats were having agents break from some of the things happening here. (huge text sizes and it saving huge text sizes in the first place)