Page MenuHomePhabricator

Considerably reduce volume of type data fetched by ConfigEditor
ClosedPublic

Authored by bengotow on Dec 5 2019, 12:54 AM.

Details

Summary

This diff removes dead weight and duplicative data from the GraphQL query that powers autocompletion in the config editor, at the cost of slightly more explicit passing of a "list of all the inner types" to the ConfigTypeSchema component.

  • We were expanding+fetching fully populated innerTypes of every type
  • We were fetching two layers of innerTypes / ofTypes for ListConfigType

This reduces the number of lines in the GraphQL response for "Sleepy Pipeline" from 7623 to 1520, and that pipeline has very little config. Did not try with the huge dag.

Test Plan

Run Dagit 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

bengotow created this revision.Dec 5 2019, 12:54 AM
bengotow added inline comments.Dec 5 2019, 12:55 AM
js_modules/dagit/src/ConfigTypeSchema.tsx
110

Rather than expand this by default we ask for allInnerTypes as a separate prop - this allows us to re-use the top level data that is fetched for config autocompletion rather than getting it all again for every type.

This revision is now accepted and ready to land.Dec 5 2019, 5:33 PM
schrockn requested changes to this revision.Dec 5 2019, 6:13 PM

I'm actually seeing crashes now in the config editor with this applied

This revision now requires changes to proceed.Dec 5 2019, 6:13 PM
bengotow planned changes to this revision.Dec 5 2019, 7:21 PM

Oh interesting! Will check this out this afternoon—I wonder if TypeScript didn't catch usage of some of this data somehow.

@bengotow I think this might be the breaking point for doing a more fundamental server side rearchitecture of the config type system. there are some really problematic core aspects of it that this is exposing. I did throw up a hack that gets this working but not the best at all https://dagster.phacility.com/D1544

This revision was not accepted when it landed; it landed in state Changes Planned.Dec 6 2019, 12:12 AM
This revision was automatically updated to reflect the committed changes.