Page MenuHomePhabricator

(Runs 4/N) Split runs.py into package
AbandonedPublic

Authored by schrockn on Wed, Sep 4, 3:29 PM.

Details

Reviewers
natekupp
Group Reviewers
Restricted Project
Summary

Splitting up and organizing storage/runs.py into a package to avoid some circular dep issues and organize things better in general

Depends on D931

Test Plan

Buildkite

Diff Detail

Repository
R1 dagster
Branch
runs-from-db-4
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

schrockn created this revision.Wed, Sep 4, 3:29 PM
natekupp accepted this revision.Wed, Sep 4, 4:12 PM
natekupp added a subscriber: natekupp.

LGTM, but see comments

python_modules/dagster/dagster/core/storage/runs/__init__.py
2

Is this temporary to keep the diff small? otherwise should stay consistent on whether we permit this sort of thing

python_modules/dagster/dagster/core/storage/runs/sql.py
1 ↗(On Diff #4221)

maybe name this file sqlite.py? presumably we'll need different versions of RunStorage for other SQL DBs

python_modules/dagster/dagster_tests/core_tests/test_run_storage.py
10

per above, clean up imports to fully-qualified names from dagster.core.storage.runs.<type>

This revision is now accepted and ready to land.Wed, Sep 4, 4:12 PM
max added a subscriber: max.Wed, Sep 4, 5:35 PM
max added inline comments.
python_modules/dagster/dagster/core/storage/runs/sql.py
1 ↗(On Diff #4221)

+1

schrockn added inline comments.Wed, Sep 4, 9:07 PM
python_modules/dagster/dagster/core/storage/runs/__init__.py
2

i don't have a coherent unified theory around this. was just minimizing thrash here

python_modules/dagster/dagster/core/storage/runs/sql.py
1 ↗(On Diff #4221)

good idea

python_modules/dagster/dagster_tests/core_tests/test_run_storage.py
10

will follow up and then clean up in subsequent diff if needed

schrockn abandoned this revision.Fri, Sep 6, 9:27 PM