Page MenuHomePhabricator

[storage] live updating run storage
ClosedPublic

Authored by alangenfeld on Wed, Aug 21, 9:10 PM.

Details

Reviewers
max
schrockn
Group Reviewers
Restricted Project
Commits
R1:09932e2d5b74: [storage] live updating run storage
Summary

origin D789

This adds the ability for the filesystem run storage to pick up runs invoked by other processes so they can be displayed in dagit.

This does not solve for live updating event logs from other processes runs - I could hack that on but I feel that is better handled after more systemic refactors.

Test Plan

stack on top of D858
run dagit --log in one window
run dagster-graphql --log in another

see live updating runs history

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.Wed, Aug 21, 9:10 PM
max updated this revision to Diff 3909.Wed, Aug 21, 9:25 PM

Rebase

max updated this revision to Diff 3911.Wed, Aug 21, 9:33 PM

Rebase

alangenfeld added inline comments.Wed, Aug 21, 9:40 PM
python_modules/dagit/dagit/cli.py
69

log wipe sounds odd to me

--clear-logs
--delete-all-logs

also this should really be its own diff

python_modules/dagster/dagster/core/events/log.py
26–51 ↗(On Diff #3911)

I still feel like we shouldn't need this if we are managing everything correctly - relying on this feels like a sign that we are doing something wrong architecturally

python_modules/dagster/dagster/core/storage/runs.py
125–126

rename should match that in lower diff

alangenfeld requested changes to this revision.EditedWed, Aug 21, 9:50 PM

I will commandeer this

This revision now requires changes to proceed.Wed, Aug 21, 9:50 PM
alangenfeld commandeered this revision.Wed, Aug 21, 10:43 PM
alangenfeld edited reviewers, added: max; removed: alangenfeld.
alangenfeld edited the test plan for this revision. (Show Details)Fri, Aug 23, 5:05 PM
alangenfeld updated this revision to Diff 3973.Fri, Aug 23, 5:19 PM
alangenfeld edited the test plan for this revision. (Show Details)

watchdog req

alangenfeld updated this revision to Diff 3975.Fri, Aug 23, 5:49 PM

only watch when running dagit

alangenfeld retitled this revision from Add naive filesystem log watchers to [storage] live updating run storage.Fri, Aug 23, 5:57 PM
alangenfeld edited the summary of this revision. (Show Details)
alangenfeld updated this revision to Diff 3978.Fri, Aug 23, 6:02 PM
alangenfeld edited the summary of this revision. (Show Details)

fix base take 2

natekupp added inline comments.
python_modules/dagster/dagster/core/storage/runs.py
254

Can we remove the ref to run_storage from PipelineRun, make it a namedtuple, and then add a from_json() method on that object and use it here? Would be nice to clean this up for the future when PipelineRun could be stored in a variety of serialization formats, SQL, Thrift, on floppy disks, etc.

alangenfeld added inline comments.Fri, Aug 23, 6:37 PM
python_modules/dagster/dagster/core/storage/runs.py
254

aw yis

alangenfeld updated this revision to Diff 3984.Fri, Aug 23, 7:27 PM

rm mistake

alangenfeld updated this revision to Diff 3985.Fri, Aug 23, 7:40 PM

pull event_log_exists - wasnt working as it intended

schrockn accepted this revision.Sun, Aug 25, 1:32 AM
schrockn added a subscriber: schrockn.
schrockn added inline comments.
python_modules/dagster/dagster/core/storage/pipeline_run.py
143

maybe do some argument verificaiton

This revision is now accepted and ready to land.Sun, Aug 25, 1:32 AM
This revision was automatically updated to reflect the committed changes.