Skip to content

Conversation

@abrookins
Copy link
Collaborator

No description provided.

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 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"]

client: The client wrapper (OpenAI or Anthropic)
redis_conn: Redis connection
"""
print("Summarizing session")
Copy link

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

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.
Returns:
The value if found, or the default
"""
if isinstance(doc, dict):
Copy link

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

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.

2 participants