Skip to content
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

Add ability to specify topics for memory extraction and retrieval #93

Merged
merged 13 commits into from
Jan 10, 2025

Conversation

heyitsaamir
Copy link
Contributor

@heyitsaamir heyitsaamir commented Jan 8, 2025

This PR introduces the ability for the client to specify topics for extraction and retrieval.

Specifically, inside the memory_module config we add the ability to specify a Topic:
eg:

Topic(name="Device Type", description="The type of device the user has"),
Topic(name="Operating System", description="The user's operating system"),
Topic(name="Device year", description="The year of the user's device"),

Here, we are telling the system to specifically look for these topics during extraction. After extraction, when we store the memories, we also store the associated topic.
By default, we provide some general topics that the system uses - these retain the current behavior of the system (See default topics).

During retrieval, we now provide the ability to query for memories with topic as well. So if the user specifies a topic, but no query, then all the memories associated for that particular user for that topic will be returned ordered from latest to oldest memory. If a query is specified, then both query and topic will be taken into consideration.
The new signature for retrieval is as follows:

@abstractmethod
async def retrieve_memories(
    self,
    user_id: Optional[str],
    config: RetrievalConfig,
) -> List[Memory]:
    """Retrieve relevant memories based on a query."""
    pass

Here RetrievalConfig is:

class RetrievalConfig(BaseModel):
    query: Optional[str] = None
    topic: Optional[Topic] = None
    limit: Optional[int] = None

TODO:
[ ] Add a test for topic retrieval

Addresses:
#40

@heyitsaamir heyitsaamir changed the title Topics Add ability to specify topics for memory extraction and retrieval Jan 9, 2025
@heyitsaamir heyitsaamir marked this pull request as ready for review January 9, 2025 06:11
@Copilot Copilot bot review requested due to automatic review settings January 9, 2025 06:11
@heyitsaamir heyitsaamir requested a review from a team as a code owner January 9, 2025 06:11

Choose a reason for hiding this comment

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

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

Files not reviewed (9)
  • packages/memory_module/storage/migrations/14_add_topic_to_memories.sql: Language not supported
  • packages/memory_module/core/memory_core.py: Evaluated as low risk
  • packages/evals/benchmark_memory_module.py: Evaluated as low risk
  • packages/memory_module/interfaces/base_memory_storage.py: Evaluated as low risk
  • tests/memory_module/test_memory_storage.py: Evaluated as low risk
  • packages/memory_module/core/memory_module.py: Evaluated as low risk
  • src/tech_assistant_agent/tools.py: Evaluated as low risk
  • packages/memory_module/interfaces/base_memory_module.py: Evaluated as low risk
  • packages/memory_module/interfaces/base_memory_core.py: Evaluated as low risk
Comments suppressed due to low confidence (2)

tests/memory_module/test_memory_module.py:360

  • Consider using the DEFAULT_TOPICS constant instead of hardcoding the list of topics to make the test more robust.
Topic(name="Device Type", description="The type of device the user has"),

packages/memory_module/storage/in_memory_storage.py:2

  • The _cosine_similarity function currently only calculates the dot product. It should be updated to correctly calculate the cosine similarity by dividing by the magnitudes of the vectors.
def _cosine_similarity(self, memory_vector: List[float], query_vector: List[float]) -> float:
Copy link
Contributor

@limamicro limamicro left a comment

Choose a reason for hiding this comment

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

Leave some nit comments

@heyitsaamir heyitsaamir changed the base branch from main to aamirj/improve_dedupe_prompot January 10, 2025 17:20
Base automatically changed from aamirj/improve_dedupe_prompot to main January 10, 2025 17:52
@heyitsaamir heyitsaamir merged commit 069677b into main Jan 10, 2025
6 checks passed
@heyitsaamir heyitsaamir deleted the aamirj/topics branch January 10, 2025 21:41
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.

2 participants