Page MenuHomePhabricator

(snap-backed-graphql-7) Back all dependency-related things via snap
ClosedPublic

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

Details

Summary

This uses the pipeline and dependency indexes to back all
dependency-related graphql types.

This is the most thorny part of the graphql schema and I believe it
should be relatively straightforward from here to convert the rest
of the schema to be snapshot backed.

Depends on D2310

finallydone

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:51 PM
schrockn updated this revision to Diff 10825.Mar 21 2020, 8:32 PM
schrockn edited the summary of this revision. (Show Details)

upmessage

Harbormaster completed remote builds in B8756: Diff 10826.

even with the diagram its pretty confusing what were threading here for arguments to these dauphin types. Definitely worth a clean up pass.

I am intrigued by a world where we only pass pipeline_index and key ( or pipeline_index, path, name) to each of these dauphin types, and PipelineIndex gets fleshed out with methods around handling these keys/paths. Something about passing around copies of the value objects already inside the index feels off to me. Since the inner objects are unaware of their place it seems like it could be tough to debug. After staring at it long enough this all makes sense - but I think this will be easier to understand on first read if more is consolidated in to the PipelineIndex and a pointer type of some variety is used to key back in to it to resolve values. This would move DependencyStructureSnapshotIndex to be an implementation detail of PipelineIndex instead of something passed all over the place.

python_modules/dagster-graphql/dagster_graphql/schema/pipelines.py
68

just pass index?

python_modules/dagster-graphql/dagster_graphql/schema/solids.py
282โ€“288

asside:

i forget what the distinction is between Input and InputDefinition is

seems like we can remove Solid here and just fetch it on the definition?

472

ha interesting - i didnt realize that definition name would be sufficient to identity a dependency plane

schrockn added inline comments.Mar 23 2020, 9:21 PM
python_modules/dagster-graphql/dagster_graphql/schema/pipelines.py
68

need to pass both because in some cases the dep structure is for a composite def

python_modules/dagster-graphql/dagster_graphql/schema/solids.py
185

see here

288

mind if i do that in a follow up so that we avoid client changes?

288

Input is to InputDefinition as Solid is to SolidDefinition.

There is one Input per Solid Instance. And many Solid instances can share 1 InputDefinition

alangenfeld added inline comments.Mar 23 2020, 10:00 PM
python_modules/dagster-graphql/dagster_graphql/schema/solids.py
288

ya file an issue or fast follow - no need to do in this diff

schrockn updated this revision to Diff 10911.Mar 23 2020, 10:37 PM

pass around strings instead of snaps

schrockn updated this revision to Diff 10913.Mar 23 2020, 10:47 PM

lil cleanup

@alangenfeld your suggestion was *excellent*. cleaned this up so much ๐Ÿ‘๐Ÿ‘๐Ÿ‘

alangenfeld accepted this revision.Mar 23 2020, 11:38 PM

verygood

python_modules/dagster-graphql/dagster_graphql/schema/solids.py
346โ€“347

maybe a more detailed note: "should be able to remove - fetch off definition.solid"

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