Page MenuHomePhabricator

Consolidate double implementation of run storage
ClosedPublic

Authored by max on Mon, Aug 19, 9:33 PM.

Details

Reviewers
alangenfeld
Group Reviewers
Restricted Project
Commits
R1:8a2e2fcbc437: Consolidate double implementation of run storage
Summary

Resolves https://github.com/dagster-io/dagster/issues/1676.

Pushes implementation of run storage down to dagster level. Will disentangle the inheritance in a follow-on diff.

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.Mon, Aug 19, 9:33 PM
max edited the summary of this revision. (Show Details)Mon, Aug 19, 9:38 PM
max updated this revision to Diff 3819.Mon, Aug 19, 9:39 PM
max edited the summary of this revision. (Show Details)

Nits

alangenfeld added inline comments.Mon, Aug 19, 9:56 PM
pyproject.toml
3

?

python_modules/dagster/dagster/core/storage/config.py
7–8

we need to consolidate this behavior with --log in dagster-graphql/dagit i think

python_modules/dagster/dagster/core/storage/runs.py
137

grumpycat

349

mistake so nice we made it twice

would love to see a stacked diff on top of this that takes a pass at cleaning up the weird class hierarchy / poor naming

max updated this revision to Diff 3827.Mon, Aug 19, 10:45 PM

Black

max updated this revision to Diff 3828.Mon, Aug 19, 10:51 PM

Black

max edited the summary of this revision. (Show Details)
max updated this revision to Diff 3829.Mon, Aug 19, 10:51 PM
max edited the summary of this revision. (Show Details)
max removed a reviewer: schrockn.

Black

Harbormaster failed remote builds in B3063: Diff 3829!
max edited the summary of this revision. (Show Details)Tue, Aug 20, 5:26 PM

just looking for answers to [1]

pyproject.toml
3

[1]

python_modules/dagster/dagster/core/storage/config.py
7–8

[1]

This is a solid step forward. I'd love for you to comment on this diff re: what next steps are in terms of reining it in. I assume we want to get rid of the PipelineRun hierarchy?

class FilesystemRunStorage(InMemoryRunStorage): also seems very dubious.

I'm going to let alex accept this as he's been the primary reviewer.

pyproject.toml
3

seems like this should be its own change

python_modules/dagster/dagster/core/storage/runs.py
117–122

I don't really think we should be trying to make somethink like RunStorage act like a python built-in

182

I think this might be worth using __slots__ because we might end up dealing with quite a few of these in memory

314–317

this method seems of dubious value. why not have clients construct this directly?

max added a comment.Wed, Aug 21, 12:07 AM

I think that https://dagster.phacility.com/D850 should address @schrockn's questions

max added inline comments.Wed, Aug 21, 12:09 AM
pyproject.toml
3

yep

python_modules/dagster/dagster/core/storage/config.py
7–8

I'll do this in a follow-on

python_modules/dagster/dagster/core/storage/runs.py
137

see D850

182

yep

max updated this revision to Diff 3879.Wed, Aug 21, 12:11 AM

Rebase

alangenfeld accepted this revision.Wed, Aug 21, 3:10 PM
This revision is now accepted and ready to land.Wed, Aug 21, 3:10 PM