Page MenuHomePhabricator

Scaffold alembic
ClosedPublic

Authored by max on Sat, Nov 23, 1:52 AM.

Details

Summary

Prep for the ability to migrate SQL schemata. Stacked on D1470, D1472.

Test Plan

Unit

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

max created this revision.Sat, Nov 23, 1:52 AM

this one might be best to do in person walk through - not obvious what here is all machine generated artifacts vs edited / manually written

max added a comment.Mon, Nov 25, 6:34 PM

All of this is boilerplate, I've separated it into its own diff so as not to complicate review of all the manual stuff. We could maybe condense this by having our own library functions, like dagster.utils.sql.run_migrations_offline and dagster.utils.sql.run_migrations_online.

This is fine. More high level, can I get context as to why are we doing this? This is weird that we are saying, you have to use alembic if you use the dagster postgres library. Also why is run storage coupled so tightly to this lib? Is this lib purely internal? (Sorry I am catching up here)

max added a comment.Mon, Nov 25, 8:11 PM

Context is, we need some way to version and migrate the schemas for our storages. The dagster-postgres library contains postgres-backed storages. We are trying to use a single schema definition for all of our SQL-backed storages, so that we can have unified migration tooling. (Obviously if you write your own storage class, you don't need to worry about backwards compatibility if you don't need to worry about it.)

themissinghlink accepted this revision.Mon, Nov 25, 8:18 PM

Got it. Essentially, clients who want to use our postgres backed solution, get this for free and need to use alembic. If they disagree they can implement their own thing (with relative ease since it's all configurable in DAGSTER_HOME. Cool!

This revision is now accepted and ready to land.Mon, Nov 25, 8:18 PM
max added a comment.Mon, Nov 25, 8:30 PM

Ideally, they won't use alembic directly -- we will call alembic APIs from dagster instance migrate, which will call the upgrade() method on the storages. Would love your eyes on this setup in https://dagster.phacility.com/D1471

alangenfeld added inline comments.Mon, Nov 25, 11:29 PM
python_modules/dagster/dagster/core/storage/event_log/sqlite/alembic/alembic.ini
16

-N should go away given the new version of black we are on right?

max added inline comments.Tue, Nov 26, 5:29 PM
python_modules/dagster/dagster/core/storage/event_log/sqlite/alembic/alembic.ini
16

hyup

max updated this revision to Diff 6926.Tue, Nov 26, 7:22 PM

Rebase

This revision was automatically updated to reflect the committed changes.