Page MenuHomeElementl

Remove Dauphin
ClosedPublic

Authored by max on Jan 26 2021, 10:17 PM.

Details

Summary

This makes it easier to reuse pieces of the dagster-graphql schema, as well as smoothing the path for new contributors. Resolves https://github.com/dagster-io/dagster/issues/2270.

Test Plan

Unit

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
python_modules/dagster-graphql/dagster_graphql/implementation/fetch_runs.py
5

where there are collisions between the native/dagster type and the graphene type, we import the dagster type with the prefix Dagster

python_modules/dagster-graphql/dagster_graphql/implementation/utils.py
9

rename

python_modules/dagster-graphql/dagster_graphql/schema/__init__.py
6–65

Previously we were relying on the implicit registry functionality / metaclass magic provided by Dauphin to load all of the types in the schema; this moves us to be explicit, which may or may not be better, but more importantly allows pieces of the schema to be imported by other projects without having to pull in the entire registry

python_modules/dagster-graphql/dagster_graphql/schema/asset_key.py
2

There's nothing final or definitive about this reorganization, but I have tried to break up the very large files in dagster-graphql into more manageable chunks

python_modules/dagster-graphql/dagster_graphql/schema/backfill.py
32

retain Meta for unions

python_modules/dagster-graphql/dagster_graphql/schema/config_types.py
41

drop this boilerplate

47

wrap in lambda to avoid circular reference issue

100

move all interfaces to use tuples rther than lists

215–248

could also be __types__ or __all__

python_modules/dagster-graphql/dagster_graphql/schema/errors.py
9–11

this moved off dauphin

27

python 3 super throughout

95

more format strings

python_modules/dagster-graphql/dagster_graphql/schema/inputs.py
2

moved input types together

python_modules/dagster-graphql/dagster_graphql/schema/jobs.py
26

no need for this to go through dauphin wrapper without the registry

python_modules/dagster-graphql/dagster_graphql/schema/logs/__init__.py
2

lazy to avoid any import issues and allow subsets of the schema to be imported

python_modules/dagster-graphql/dagster_graphql/schema/runs.py
30

here's an example of a fully-qualified string reference

python_modules/dagster-graphql/dagster_graphql/schema/solids.py
75–76

trying to be explicit

I think this all makes sense to me.

We should probably merge this after the release today... (good luck with rebasing?) as we're probably relying more than usual on graphql test coverage to surface issues.

push_n_pray

... will give others some time to weigh in before accepting.

Are we losing any greppability here? It was kind of nice that all the
graphql type classes had a common prefix, we could consider adding a
GraphQL prefix or something by convention? Or is that what python modules
are for :)

Very much support getting rid of the registry. Thanks for doing this! Let's just try to mitigate risk as much as possible with a diff this large

definitely think we should keep prefix like @dgibson. grepability is good

happy to reintroduce the prefix, it will entail a whole bunch of overhead in Meta classes but so be it -- does anyone have suggestions for the prefix? Gql?

rename with Graphene* prefix

Graphene works for me!

This revision is now accepted and ready to land.Jan 29 2021, 7:25 PM
This revision was automatically updated to reflect the committed changes.