Skip to content

Conversation

@heyitsaamir
Copy link
Collaborator

Summary of PR

Previously, topic or query was a requirement for search_memories. But if the dev wants to look at all memories for a user, then this function shouldn't assert this requirement. They may have a good reason to do this (Maybe they want to answer a question using all the previous knowledge of the user)

Details

Testing

Added a unit test

Checklist

  • I thought through documentation (and updated the docs if necessary)
  • I updated the CHANGELOG.md file (or have a good reason not to)

@heyitsaamir heyitsaamir marked this pull request as ready for review February 1, 2025 00:34
Copilot AI review requested due to automatic review settings February 1, 2025 00:34
@heyitsaamir heyitsaamir requested a review from a team as a code owner February 1, 2025 00:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

@singhk97
Copy link
Contributor

singhk97 commented Feb 3, 2025

They can use the memory_module.get_memories(user_id=user_id) method to get all the memories for a given user.

@heyitsaamir
Copy link
Collaborator Author

They can use the memory_module.get_memories(user_id=user_id) method to get all the memories for a given user.

I know it seems weird to do the same thing in two different ways, but they're for two different purposes. Basically this would make search_memories a superset of get_memories. We could remove get_memories, but it's semantically cleaner for normal CRUD. thoughts?

@singhk97
Copy link
Contributor

singhk97 commented Feb 4, 2025

They can use the memory_module.get_memories(user_id=user_id) method to get all the memories for a given user.

I know it seems weird to do the same thing in two different ways, but they're for two different purposes. Basically this would make search_memories a superset of get_memories. We could remove get_memories, but it's semantically cleaner for normal CRUD. thoughts?

I'm fine with that. It's simpler from a dev ex perspective as well. I think get_memories still has its place with accepting memory ids as inputs.

@heyitsaamir heyitsaamir merged commit 0eaa1c7 into main Feb 4, 2025
5 checks passed
@heyitsaamir heyitsaamir deleted the aamirj/allow_search_via_anything branch February 4, 2025 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants