Page MenuHomePhabricator

EnvironmentSchema in GraphQL
ClosedPublic

Authored by schrockn on May 24 2019, 6:45 PM.

Details

Summary

Right now the way you get at the config schema of a pipeline is
as a property off of that pipeline. I think this is bit of an awkward
way to model it as each of those have to take mode and solidSubset and
then you have to have an error union in a nested part of the tree.

Instead here we just have a top level environment schema that we can
hang elements off of. Interested in hearing feedback on this. I'm not
100% convinced that it is better.

Depends on D301

Test Plan

unit tests

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 updated this revision to Diff 1007.May 24 2019, 6:45 PM
schrockn created this revision.

up

schrockn updated this revision to Diff 1010.May 24 2019, 6:53 PM
schrockn edited the summary of this revision. (Show Details)
schrockn added reviewers: max, alangenfeld, bengotow.

up

the thrust of this seems good but im a bit confused by the specific schema

js_modules/dagit/src/schema.graphql
77–79

its not obvious to me from name alone what this schema represents exactly, these two fields are particularly confusing

schrockn added inline comments.May 28 2019, 7:52 PM
js_modules/dagit/src/schema.graphql
77–79

The environmentType is effectively the "root type" that describes the config of the *entire* document. ConfigTypes are the list of all the individual config types in the environmentType transitively. It is what you index into for things like the typeahead.

alangenfeld accepted this revision.May 28 2019, 8:02 PM

I think clearer names or descriptions are worth adding

js_modules/dagit/src/schema.graphql
77–79

ah I see - throwing out some names to consider:

rootEnvironmentType

allConfigTypes / flattenedConfigTypes / containedConfigTypes

python_modules/dagster-graphql/dagster_graphql/schema/roots.py
333–334

can your provide descriptions via dauphin?

This revision is now accepted and ready to land.May 28 2019, 8:02 PM

ya i will rename to something better and also add descriptions. still curious re: @bengotow 's thoughts

This looks great. I think making this a top level GraphQL query that requires the execution selector makes it more clear that the environment is dependent on the solid subset. I think this is also a nice step toward lowering the systems dependence on the idea of a pipeline. (In case we want to allow any composite solid to be an addressable unit of execution one day, etc.)

js_modules/dagit/src/schema.graphql
77–79

I agree it's a bit confusing that these are both config types, but one is called "environment type" which sounds like a different concept. I'd +1 for calling it "rootType". (I think the fact that it defines the root of the environment is clear just by the fact that it's within the EnvironmentSchema, so we may not need to call it rootEnvironmentType)

I'm also a big fan of making it clear that the second set is flattened. I feel like we have several places where we expose a tree of data and expose a list of it's flattened nodes as well. I wonder if it'd be possible to pull in the same naming conventions we used for solid retrieval? (solids for tree, solidHandles for list). I guess the "handle" concept isn't applicable, but it might be useful to be able to request their parent nodes, etc. using the same terms.

bengotow accepted this revision.May 28 2019, 8:39 PM
max added inline comments.May 28 2019, 8:39 PM
js_modules/dagit/src/schema.graphql
77–79

what about rootConfigType and allConfigTypes? i think it'd be very nice to have an explicit description on the list so that the graphql explorer is more helpful

python_modules/dagster-graphql/dagster_graphql/schema/roots.py
333–334

+1

schrockn updated this revision to Diff 1463.Jun 5 2019, 3:35 PM
schrockn edited the summary of this revision. (Show Details)

up

schrockn updated this revision to Diff 1464.Jun 5 2019, 3:35 PM
schrockn edited the summary of this revision. (Show Details)

up

schrockn updated this revision to Diff 1465.Jun 5 2019, 4:28 PM

test things

Harbormaster completed remote builds in B1187: Diff 1465.
This revision was automatically updated to reflect the committed changes.