Skip to content

Extract memories, better summarization, etc. #7

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 2 commits into from
May 14, 2025
Merged

Conversation

abrookins
Copy link
Contributor

No description provided.

@Copilot Copilot AI review requested due to automatic review settings May 14, 2025 09:00
@abrookins abrookins merged commit 8805614 into main May 14, 2025
1 check passed
Copy link
Contributor

@Copilot 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.

Pull Request Overview

This PR enhances memory extraction, summarization, and Redis schema support by introducing new migrations, CLI commands, and model refactoring.

  • Add migrations and CLI commands (migrate_memories, rebuild_index) to update Redis schema with memory_hash, discrete_memory_extracted, and memory_type.
  • Implement discrete LLM-based memory extraction and integrate memory_type filtering throughout API and models.
  • Refactor model definitions into a single file, update summarization code, and modernize Docker/Compose commands to use the uv CLI.

Reviewed Changes

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

Show a summary per file
File Description
pyproject.toml Bump redisvl to 0.6.0 for new index-field support
docker-compose.yml Switch service command to uv run agent-memory mcp --mode sse
agent_memory_server/utils/redis.py Add memory_type, discrete_memory_extracted and overwrite flag for index
agent_memory_server/summarization.py Update attribute access, add debug prints
agent_memory_server/models.py Consolidate all Pydantic models, add memory_type and discrete_memory_extracted fields
agent_memory_server/migrations.py New migrations for populating memory_hash, discrete_memory_extracted, memory_type
agent_memory_server/extraction.py Introduce extract_topics_llm and extract_discrete_memories
agent_memory_server/cli.py Add rebuild_index and migrate_memories CLI commands
agent_memory_server/client/api.py Accept memory_type filter in search_long_term_memory
README.md Document uv usage and new CLI commands
Dockerfile Change default CMD to uv run agent-memory api
Comments suppressed due to low confidence (3)

agent_memory_server/migrations.py:102

  • The commented-out check means existing records with discrete_memory_extracted set to "t" will be reset to "f" on re-run; re-enable or update the skip logic to make the migration truly idempotent.
#        # if extracted and extracted.decode() == "t":

agent_memory_server/migrations.py:18

  • There aren’t any tests covering the new migrations; consider adding integration tests to verify that each migration runs idempotently and yields the expected Redis schema changes.
"""
Simplest possible migrations you could have.
"""

Dockerfile:31

  • Ensure that the image installs the uv CLI and that the working directory is set so uv run agent-memory resolves correctly; otherwise, the container may fail to start.
CMD ["uv", "run", "agent-memory", "api"]

@@ -132,6 +132,7 @@ async def summarize_session(
client: The client wrapper (OpenAI or Anthropic)
redis_conn: Redis connection
"""
print("Summarizing session")
Copy link
Preview

Copilot AI May 14, 2025

Choose a reason for hiding this comment

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

[nitpick] Debug prints using print should be replaced with structured logging (e.g., logger.debug or logger.info) to maintain consistency and allow log-level control.

Suggested change
print("Summarizing session")
logger.info("Summarizing session")

Copilot uses AI. Check for mistakes.

@@ -142,8 +143,9 @@ async def summarize_session(
await pipe.watch(messages_key, metadata_key)

num_messages = await pipe.llen(messages_key) # type: ignore
print(f"<task> Number of messages: {num_messages}")
Copy link
Preview

Copilot AI May 14, 2025

Choose a reason for hiding this comment

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

[nitpick] This debug output should use the project's logging framework instead of print for better observability and log management.

Suggested change
print(f"<task> Number of messages: {num_messages}")
logger.info(f"<task> Number of messages: {num_messages}")

Copilot uses AI. Check for mistakes.

@@ -117,6 +125,8 @@ def safe_get(doc: Any, key: str, default: Any | None = None) -> Any:
Returns:
The value if found, or the default
"""
if isinstance(doc, dict):
Copy link
Preview

Copilot AI May 14, 2025

Choose a reason for hiding this comment

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

Consider checking for collections.abc.Mapping instead of dict to support dict subclasses and other mapping types in safe_get.

Suggested change
if isinstance(doc, dict):
if isinstance(doc, Mapping):

Copilot uses AI. Check for mistakes.

Comment on lines +52 to +53


Copy link
Preview

Copilot AI May 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The rebuild_index command runs without user feedback on completion; consider adding a click.echo after the async call to confirm success.

Suggested change
click.echo("Search index rebuilt successfully.")

Copilot uses AI. Check for mistakes.

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.

1 participant