Page MenuHomePhabricator

Basic composite expansion in the Dagit explore tab
ClosedPublic

Authored by bengotow on Jan 21 2020, 9:57 PM.

Details

Summary

This diff adds basic expansion of composite solids to the Explore tab, addressing https://github.com/dagster-io/dagster/issues/1979. This is achieved by transforming the handles we receive from the GraphQL response and essentially "cutting" composites away and replacing them with their interior solids.

This diff adds a checkbox to enable this feature in the bottom left. Toggling it forces a re-query because we need to load solids recursively and also resets the solid query string because it's very likely your previous query is now invalid.


Test Plan

Run 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.Jan 21 2020, 9:57 PM
bengotow edited the summary of this revision. (Show Details)Jan 21 2020, 10:00 PM
bengotow added inline comments.Jan 21 2020, 10:04 PM
js_modules/dagit/src/Loading.tsx
31

I think this has been missing for a while and may resolve some of the problems we've been having with apollo "serving up" half-baked or out-of-sync data. It turns out it was telling us the data wasn't ready / was partially ready any we were choosing to use it.

js_modules/dagit/src/PipelineExplorerRoot.tsx
216

I broke this arg apart from the previous one because we want to pass null here to request the entire composite tree, but we still need to pass a valid solid handle ID to solidHandle(handleId:) above or conditionally send/not-send that part of the request.

js_modules/dagit/src/SolidQueryImpl.ts
75 ↗(On Diff #8788)

This allows this expanded-composite-style solid name composite.inner.inner to be selected in the bar.

bengotow updated this revision to Diff 8791.Jan 21 2020, 10:10 PM
bengotow edited the summary of this revision. (Show Details)

Cleanup & Update tests

alangenfeld added inline comments.Jan 22 2020, 8:12 PM
js_modules/dagit/src/PipelineExplorer.tsx
191

maybe "explode composites"
also should we hide this if there are no composites in the pipeline?

js_modules/dagit/src/PipelineExplorerRoot.tsx
92–93

handle-like names

hmm bit confused by this - why cant we just use the actual handle names?

101

is this is one of those JS things were what seems silly is actually the best way? this is the best way to deep copy a big object tree?

alangenfeld accepted this revision.Jan 22 2020, 8:13 PM

I'm cool with this if 'handle-like' names are actually the same as the handles - if not it would be good to get an explanation of what exactly is going on there

This revision is now accepted and ready to land.Jan 22 2020, 8:13 PM

How difficult would it be to make this not required a full reload? E.g. have the flatten composites button/check stay in view while only dag view reloads. Extra credit for some sort of animation or fade.

bengotow planned changes to this revision.Jan 28 2020, 5:18 PM

Will update this to address the feedback! Re: full reload, I think that the flashing loading indicator is annoying but also not realistic. For large pipelines this will probably take many seconds to switch over because it has to load every solid handle. I don't think we can do much of a transition because the graph shape changes, and navigating / continuing to play with the DAG while it loads isn't productive because it'll blow away your state when it replaces the graph. I think we should probably revisit better loading state when we make it expand the composites in-place via fancier edits to the rendering algo, since that'd allow for more of a transition!

js_modules/dagit/src/PipelineExplorer.tsx
191

Ahh that's a great idea—will address both of these!

js_modules/dagit/src/PipelineExplorerRoot.tsx
92–93

Right now using the handleIDs is complicated because solid's input and output references and composite solid mappings link to another Solid not another SolidHandle, so to update the reference to that target solid's handleID we'd have to look up what it is by finding the solid with the right name at that layer in the graph.

This algorithm should generate + assign names that ARE the handleIDs though so I'll update the comment to explain this better!

101

Yeah people get all fancy with Utils.deepClone, etc, but this is actually substantially faster because all the work is done in the browser's native C++ and not in a recursive function on the JS side :-/ We might already have this wrapped in a helper elsewhere though I'll see if I can make it look cleaner :)

bengotow updated this revision to Diff 9022.Jan 28 2020, 6:02 PM
  • Rename to “explode” composites and only show when it’d do something
  • Update tests
This revision is now accepted and ready to land.Jan 28 2020, 6:02 PM
schrockn accepted this revision.Jan 28 2020, 6:09 PM
bengotow updated this revision to Diff 9133.Jan 29 2020, 10:57 PM
  • Merge branch 'master' of github.com:dagster-io/dagster into sfbg/expanded-composites-less-fun