Page MenuHomePhabricator

(snap-backed-graphql-6) Add all snaps to backed dependencies in pipelines and composites
ClosedPublic

Authored by schrockn on Mar 21 2020, 7:49 PM.

Details

Summary

Add all the information needed to encode all dependency
information for composites and pipelines in the snapshots.

This also adds indexes that will be necessary for reasonable performance
in the graphql layer. We has DependencyStructureSnapshotIndex
which contains indexes to quickly access upstream and downstream
solids within a given scope (e.g. composite or pipeline)
and then a higher level PipelineIndex that indexes all solid
definitions as well as all the dependency indexes for composite
solids in scope.

Depends on D2309

Test Plan

BK

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

schrockn created this revision.Mar 21 2020, 7:49 PM

Here is an excalidraw demonstrating where this is all going: https://excalidraw.com/#json=6223518794514432,4k6LWok4zDT7dsngGdY4-w

Inline image:

Including the indexes that will provide lookup as this stack proceeds:

alangenfeld requested changes to this revision.Mar 23 2020, 5:26 PM

gonna round trip these so i can take a second pass - these seem worthy of higher than average scrutiny

python_modules/dagster/dagster/core/snap/dep_snapshot.py
62–63

should it be named snap then?

76

consider:
DependencyStructureIndex
DependenciesIndex

157–163

dont love prev_ (im guessing this is what we call it in the other places - but this one we cant really change ever so we can try harder)

maybe prev_output_snaps -> output_snap_deps or something like that. maybe just output_snaps

python_modules/dagster/dagster/core/snap/solid.py
190–192

I *believe* we don't need this in the snap, this only affects composition function evaluation

288–290

this confuses me
mapped / parent -> from / to ?

309–311

same as above

This revision now requires changes to proceed.Mar 23 2020, 5:26 PM

absolutely. this diff in particular warrants it

schrockn marked 2 inline comments as done.Mar 23 2020, 7:11 PM
schrockn added inline comments.
python_modules/dagster/dagster/core/snap/dep_snapshot.py
157–163

upabout upstream_output_snaps?

schrockn added inline comments.Mar 23 2020, 7:14 PM
python_modules/dagster/dagster/core/snap/solid.py
190–192

correct. will remove

288–290

parent is the containing composite output.

mapped is the internal, constiuent solid

schrockn updated this revision to Diff 10896.Mar 23 2020, 7:14 PM

alex feedback

still have not renamed "parent" and "mapped" stuff. interested at what you think

alangenfeld added inline comments.Mar 23 2020, 7:48 PM
python_modules/dagster/dagster/core/snap/dep_snapshot.py
157–163

👍

python_modules/dagster/dagster/core/snap/solid.py
288–290

parent_ is what gets me - maybe just drop it to output_name? If not just something other than parent_ since parent/child doesn't feel right here

alangenfeld accepted this revision.Mar 23 2020, 9:11 PM
alangenfeld added inline comments.
python_modules/dagster/dagster/core/snap/solid.py
288–290

external_

This revision is now accepted and ready to land.Mar 23 2020, 9:11 PM