Skip to content

refactor: delegate memory models and table creation to memu-py#19

Merged
ankaisen merged 26 commits intomainfrom
feat/sqlmodel-base-and-memory
Feb 24, 2026
Merged

refactor: delegate memory models and table creation to memu-py#19
ankaisen merged 26 commits intomainfrom
feat/sqlmodel-base-and-memory

Conversation

@SparkZou
Copy link
Copy Markdown
Contributor

@SparkZou SparkZou commented Feb 9, 2026

Summary

Remove server-side memory model definitions and Alembic migrations in favor of
memu-py's built-in database management (ddl_mode: "create"). The server now
acts purely as a wrapper — it configures and initializes MemoryService from
memu-py, which handles all ORM models, table creation, and schema management
internally.

Changes

Added

  • config/settings.py — centralized Settings (pydantic-settings) declaring all
    environment variables with typed defaults (DATABASE_*, EMBEDDING_*,
    TEMPORAL_*, STORAGE_PATH, etc.)
  • config/memu.pybuild_memu_config() / build_memu_llm_profiles() to
    construct memu-py configuration from Settings
  • app/services/memu.pycreate_memory_service() factory
  • tests/test_memu_config.py — tests for the new config builders

Modified

  • alembic/env.py — removed app.models imports; set target_metadata = None
  • app/main.py — added module docstring; extracted exception messages to
    variables (EM101/EM102)
  • tests/test_env_validation.py — moved import os to top level; added
    check=False to subprocess.run
  • tests/test_health.py — used HTTPStatus.OK instead of magic 200;
    narrowed exception catch from Exception to (ImportError, RuntimeError)
  • pyproject.toml — changed memu-py to memu-py[postgres]; removed
    sqlmodel, sqlalchemy, pgvector, ksuid, pendulum, openai (provided
    transitively by memu-py); updated deptry ignore rules

Motivation

The previous implementation duplicated model definitions that memu-py already
provides, causing import errors (app.models.base did not exist) and
architectural misalignment. Per review feedback, the server should rely on
memu-py for all memory-related schema management.

- Add BaseModel with KSUID primary keys and automatic timestamps
- Implement Memory model with pgvector support for embeddings
- Add MemoryCreate, MemoryRead, MemoryUpdate schemas
- Migrate from SQLAlchemy DeclarativeBase to SQLModel
- Update database.py to use SQLModel
- Add comprehensive unit tests for BaseModel and Memory
- Add svix-ksuid dependency for KSUID generation

All changes pass make check (ruff, mypy, deptry, pre-commit)
Copilot AI review requested due to automatic review settings February 9, 2026 04:45
Copy link
Copy Markdown

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

Implements new SQLModel-based data modeling foundations (a shared BaseModel with KSUID primary keys + timestamps) and introduces a Memory model with pgvector embeddings, along with corresponding create/read/update schemas and unit tests.

Changes:

  • Replace the previous SQLAlchemy DeclarativeBase approach with a SQLModel BaseModel (KSUID IDs + timestamps).
  • Add Memory table/model and MemoryCreate/MemoryRead/MemoryUpdate schemas with pgvector + JSONB fields.
  • Update database module exports and add unit tests; introduce svix-ksuid dependency.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
app/models/base.py Introduces SQLModel-based BaseModel with KSUID primary key and timestamp fields.
app/models/memory.py Adds Memory table + schemas, including pgvector embedding and JSONB links.
app/models/__init__.py Re-exports models/schemas for simpler imports.
app/database.py Switches exports toward SQLModel to align with the migration.
tests/test_base_model.py Adds unit tests for KSUID IDs and timestamp defaults.
tests/test_memory.py Adds unit tests for Memory model and schemas.
pyproject.toml Adds svix-ksuid dependency.
uv.lock Locks svix-ksuid and transitive dependencies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/database.py Outdated
Comment thread app/models/memory.py Outdated
Comment thread tests/test_memory.py Outdated
Comment thread app/models/base.py Outdated
Comment thread app/models/base.py Outdated
Critical fixes addressing Copilot feedback:
- Add Base alias in base.py for Alembic backward compatibility
- Fix timezone-aware datetimes (use datetime.now(timezone.utc) instead of utcnow())
- Add onupdate lambda for automatic updated_at refresh
- Fix links type annotation from bare list to list[Any]
- Export both Base and SQLModel from database.py
- Correct ksuid import (lowercase ksuid() function)

Test fixes:
- Update test files to use timezone.utc instead of UTC
- Fix KSUID length assertion (40 chars, not 27)
- Ensure all datetime comparisons are timezone-aware

All tests now pass (20/20) and make check succeeds.
…existence

Assert against SQLModel Field metadata (model_fields[].index) rather than
hasattr() checks that would pass even if indexes were removed.
Copilot AI review requested due to automatic review settings February 9, 2026 07:18
Copy link
Copy Markdown

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

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/models/memory.py Outdated
Comment thread app/models/base.py Outdated
Comment thread pyproject.toml
- Set nullable=False on content Column to match required str type
- Import app.models in alembic/env.py so metadata is populated
- Fix bare list to list[Any] in MemoryUpdate schema
Copy link
Copy Markdown

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

Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/models/base.py Outdated
Comment thread app/models/memory.py Outdated
Comment thread alembic/env.py Outdated
- Remove redundant index=True on primary key (PG indexes PKs by default)
- Add sa_type=DateTime(timezone=True) to happened_at field
- Update test to reflect primary key no longer has explicit index flag
Copy link
Copy Markdown

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

Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_base_model.py Outdated
Copy link
Copy Markdown

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

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread alembic/versions/52875d5f3609_create_memories_table.py Outdated
Comment thread pyproject.toml Outdated
Copy link
Copy Markdown

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

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_memory.py Outdated
Copy link
Copy Markdown

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

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread alembic/versions/52875d5f3609_create_memories_table.py Outdated
Copy link
Copy Markdown

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

Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/main.py
Use Python logging framework instead of traceback.print_exc() so
error output respects log levels and integrates with log aggregation
tools in production.
Copy link
Copy Markdown

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

Copilot reviewed 16 out of 18 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/main.py
Comment thread alembic/env.py
Comment thread tests/test_env_validation.py Outdated
Comment thread app/main.py
Comment thread tests/test_health.py Outdated
Comment thread app/services/memu.py
Comment thread pyproject.toml Outdated
…ents

- Add detailed NOTE comment explaining target_metadata = None in
  alembic/env.py and how to re-enable autogenerate for server tables.
- Assert full OPENAI_API_KEY error message in test_env_validation.py.
- Narrow test_health.py exception catch to (RuntimeError, ImportError).
- Update psycopg deptry comment to reflect actual usage.
- Remove stale pydantic_core from DEP003 ignore list.
Copy link
Copy Markdown

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

Copilot reviewed 16 out of 18 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread config/settings.py Outdated
Comment thread app/main.py
Comment thread pyproject.toml
- Settings docstring now shows full resolution order: init kwargs >
  environment variable > .env file > default.
- Add 'config' to hatch wheel packages so the built wheel includes
  the config package at runtime.
Copy link
Copy Markdown

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

Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/main.py Outdated
Copy link
Copy Markdown

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

Copilot reviewed 16 out of 18 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pyproject.toml Outdated
Comment thread alembic/env.py
Replaced hand-rolled DATABASE_HOST/USER/PASSWORD/NAME fallback with
Settings().DATABASE_URL which reads the same POSTGRES_* variables as
the application and already normalises the DSN prefix.
Copy link
Copy Markdown

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

Copilot reviewed 16 out of 18 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

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

Copilot reviewed 16 out of 18 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@SparkZou SparkZou requested a review from ankaisen February 16, 2026 10:39
Copy link
Copy Markdown
Contributor

@ankaisen ankaisen left a comment

Choose a reason for hiding this comment

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

@SparkZou Thank you! LGTM 👍

@ankaisen ankaisen merged commit f7219b0 into main Feb 24, 2026
@ankaisen ankaisen deleted the feat/sqlmodel-base-and-memory branch February 24, 2026 02:43
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