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

WIP - Condense configurations into conventions for Database (Metadatastore) Adapters #1460

Merged

Conversation

norton120
Copy link
Collaborator

WIP

this may be a long-running branch since cutting the tests over to use httpx app + FastAPI dependency injection is gonna be a bit of work.

Preamble

The Database(s) that support the application state, agent memory (including vector lookup) and the application itself (user/org management, permissions, settings config etc) interface with the rest of the codebase via a MetadataStore object.

Goals here

  • The metadatastore stays as a gateway for now, but all the configuration gets conventionalized to each adapter type. Overrides need to happen in the config stack (so 1. envars 2. config file 3. default (lives in the adapter)). Don't start moving to doing ORM'y stuff here yet, keep this focused on config squashing.
  • Way more test hooks. We want to start seeing unit tests in this PR, the best way to do that is to add override hooks to the existing classes where they are useful and break these down more.

@norton120
Copy link
Collaborator Author

@yoaquim this is the working PR we were talking about

@sarahwooders sarahwooders self-requested a review June 17, 2024 04:14
@norton120
Copy link
Collaborator Author

K - thinking through where the complication that prevents us using the orm directly, it's really only the archive. So if we add accessors on the related objects, the adapter can probably obfuscate that complication.
Something like

current_agent = authed_user.agents.get(agent_id)
# here's the magic
# archive_memory is not necessarily a sqlalchemy model
return current_agent.archive_memory.search(search_value)

In this case the adapter interface duck types as an orm - so with the pgvector adapter archive_memory is just a model, in SQLite it is a chroma wrapper.

@norton120
Copy link
Collaborator Author

@cpacker @sarahwooders do you know if the init.sql file at the top level of the repo is for deployment? creating the initial user/password/db for the docker image would just be setting those envars

I'd like to create the test db in the docker db init, ideally, I'd like to not add a second init file and switch them around, so that's why I'm trying to track down what it is used for at the moment

@norton120
Copy link
Collaborator Author

@cpacker @sarahwooders do you know if the init.sql file at the top level of the repo is for deployment? creating the initial user/password/db for the docker image would just be setting those envars

I'd like to create the test db in the docker db init, ideally, I'd like to not add a second init file and switch them around, so that's why I'm trying to track down what it is used for at the moment

For the moment I dumped into that init, overriding it without disturbing it is a bit of work. Can revisit before we start merging.

@norton120 norton120 force-pushed the feature/1437/condense-configs branch from 393f982 to eb50aff Compare June 19, 2024 21:19
@norton120
Copy link
Collaborator Author

OK. So the shortest path I can see from here is:

  1. add alembic migrations
  2. move to migration and connection instead of create_all (because that won't work anymore)
  3. overload the metadatastore methods to get parity - this should expose the chroma conflict naturally
  4. solve for chroma/pgvector as an overloaded model in the ORM
  5. get all tests passing, merge in all upstream changes
  6. delete all the dead code. there will be a lot. there already is.

name:Mapped[Optional[str]] = mapped_column(String, nullable=True, doc="a human-readable identifier for an agent, non-unique.")
persona: Mapped[str] = mapped_column(doc="the persona text for the agent, current state.")
# TODO: reconcile this with persona,human etc AND make this structured via pydantic!
# TODO: these are vague and need to be more specific and explained. WTF is state vs _metadata?
Copy link
Collaborator

Choose a reason for hiding this comment

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

state is the in-process state of the agent (e.g. the memory, in-context message IDs) while metadata is arbitrary info the user might want to attach to the agent

@sarahwooders sarahwooders changed the base branch from main to 1.0.0-pre July 9, 2024 22:40
@sarahwooders sarahwooders marked this pull request as ready for review July 9, 2024 22:40
is working. Next up:
- isolate the test_server failing tests
- move the settings mock into a conftest fixture
- add a test hook for SyncServer so you can do the
same thing there.
- propigate.
for default persona, human, and preset. Now all
derived from settings (which is in turn derived
from envars). Still need to square away with the
config file hierarchy, so once we resolve the value
there is only one definitive source of truth across
the rest of the code.
hit by a bus the next person doesn't need to spend
a week getting up to speed.

This helps clarify the goal in this PR: one config
hierarchy assembled once, with one mega hook.
norton120 and others added 27 commits July 9, 2024 19:09
TODO:
- mount the test sqlite/chroma somewhere that doesn't
clutter up the repo
…le stripping out extraneous elements. The memory thing needs to be abstracted in a later time, never clear if these are strings or templates or references to a related object
…to be helpers like palm to do migrations and such
…scheme. the settings.backend object is self-contained, so no more external double-setting
1. the metadata.py file is being updated to use the ORM
2. conflicting models are being sunset and/or quarantined for this PR
3. CRUD accessors stay in metadatastore but are now managed behind the scenes by the ORM

This is going to break a lot of things (which is goodTo get unbroken:
1. update the tests to no longer be aware of the backend configs
2. update the code to same
3. remove all the SQLModel and deprecated backend code
4. document (loom) how the ORM works, how to create migrations, how
to traverse the ORM tree etc etc.

Strategy here should be to merge this into a long-running branch and start CI against it,
then keep pulling main into it until we're ready for a major release (this will be a major).

Configs will be extremely thin after this PR. We should be
set up to move docker dev to a single stack and docker quickstart to a single image.
…sses still a mess, but I think we can get around that to get things working
stop spinning up servers in tests!
But...
1. need to move the db_session all the way up to
the request (where it belongs).
2. dep inject that thing at request time!
3. dep override it in conftest!
@norton120 norton120 force-pushed the feature/1437/condense-configs branch from 94ce074 to 7f4711e Compare July 9, 2024 23:11
@sarahwooders sarahwooders merged commit efd4b8b into letta-ai:1.0.0-pre Jul 10, 2024
1 of 5 checks passed
@norton120 norton120 deleted the feature/1437/condense-configs branch July 15, 2024 11:54
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