-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Fix Mem0 OSS #2604
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
Merged
Merged
Fix Mem0 OSS #2604
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
4fe9ac7
Fix Mem0 OSS
Dev-Khant 4bbf3bc
add test
Dev-Khant d079e36
fix lint and tests
Dev-Khant f93bb01
fix
Dev-Khant 3e66254
add tests
Dev-Khant bdda726
drop test
Dev-Khant 4a2697b
changed to class comparision
Vidit-Ostwal a001b4d
fixed test cases
Vidit-Ostwal 14ac1f9
Update src/crewai/memory/storage/mem0_storage.py
Dev-Khant 8f7ff2c
Update src/crewai/memory/storage/mem0_storage.py
Dev-Khant 7d20499
Merge pull request #3 from Vidit-Ostwal/pr-2604-fix
Dev-Khant e9cb65d
fix
Dev-Khant a353ad6
Merge branch 'main' into fix-mem0-oss
Dev-Khant a9297e4
fix lock file
Dev-Khant 69653aa
fix lock file
Dev-Khant File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
qq, double checking..
If the memory does not support llm, you set
output_format
, right? But I noticed there’s another logic at line 121 in the search method over there, you’re doing: “if memory has llm, delete parameters”That feels a bit confusing. Can you confirm if that’s expected behavior, or if we might need to align those two cases?
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.
@lucasgomide, I think what's happening in here is a class comparision, whether the memory object belongs to Memory or Memory Client
Basically MemoryClient does not have any llm attribute to it, so the first check is to make sure that if
self.memory
is not a MemoryClient class object, we need to have a parameter calledoutput_format
. and in the second case check I believe they are checking whether it's a object of Memory class, and if yes then we do not need this parameter.Bit confusing how
mem0
is configured.Still author can confirm more on this.
One suggestion from my side would be if I am right about class comparision, would be to that there are better way to check whether a object belongs to a particular class, like
isinstance
, I think better to use this for better code visibility. Lucas can surely add more context on this part.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.
@Vidit-Ostwal I also prefer class comparison, but even after changing it, the question remains… I’m asking because the test didn’t confirm that.
From reading this line, I expected that when running with Memory, the output_format shouldn’t be included in the
add()
parameters - but is not happing (on test at least)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.
@Dev-Khant can you help with that?
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.
Hey @lucasgomide,
output_format="v1.1"
-> {"results": }output_format="v1.0"
-> []We’ve now fully deprecated the
output_format
parameter from the OSS version, though it’s still in the process of being removed from the Platform. To maintain consistency in output format, I’ve addedoutput_format="v1.1"
on line 92.On line 121, there's a check to determine if the call is targeting OSS. If so, it removes the
output_format
parameter since it's deprecated in OSS but still required for the Platform or Mem0 client.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.
@Dev-Khant that is the point..
It's a bit tricky because in your tests
both implementation are propagating
output_format
, but by reading your code it should be sent ONLY when the memory does not hasllm
.This is the last touchable point that isnot clear to me
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.
Hey @lucasgomide Then can you please help with the test?