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

Draft
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

norton120
Copy link
Contributor

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

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.
TODO:
- mount the test sqlite/chroma somewhere that doesn't
clutter up the repo
@norton120 norton120 force-pushed the feature/1437/condense-configs branch from 393f982 to eb50aff Compare June 19, 2024 21:19
@norton120
Copy link
Contributor 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.

Ethan Knox and others added 8 commits June 26, 2024 15:10
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To triage
Development

Successfully merging this pull request may close these issues.

None yet

1 participant