Page MenuHomePhabricator

Rework Gaant for performant rendering on large DAGs, add sidebar, polish
ClosedPublic

Authored by bengotow on Tue, Feb 11, 3:32 PM.

Details

Summary

This diff includes a ton of fixes and improvements to the Gaant viz. Most importantly it now zooms-to-fit by default, and a new sidebar shows what's executing and what's failed. Clicking a step puts it in a distinct "selection" state and the View Raw Logs and Re-Execute Step options are back.

Internally I had to almost entirely re-write the visualization to make it performant on our largest test DAG, but I'm really satisfied with the results. It's /almost/ smooth on my machine, even while the logs are streaming in. This was achieved primarily by:

  • Breaking layout into a two step process, in which we do the heavy lifting once and memoize it, and we can re-apply run metadata to adjust box sizes / colors in a fast second pass.
  • Passively observing the visible region (viewport) and clipping rendering to just boxes / lines that intersect the region.
  • Memoizing line and box rendering.

Changelog:

Add right hand status panel

Remove support for old EP

Colorize all three gaant viz options when metadata is available

Add single step re-execution button, persistent selection in sync with log selection

Bump blueprint and forcibly fix padding issue

Add little dots, remove labels

Split layout logic in half so metadata can be re-applied very fast

Remove scroll wheel zoom, because graph may pan both horizontally and vertically

Improve timescale label thresholds, switch slider to log scale

Adding a loading state to the logs provider / table

Fix slider cascading render, fix presets bar re-render

Observe viewport, clip offscreen boxes and make it fast 🍟

Change scale slider to β€œzoom” (0-100), map to actual px-per-ms scale in realtime for β€œfit to view” min zoom

Use DOM resizeObserver to sync viewport to panel size

Carry yellow focus into the log search box?

Put back View Raw Output button

Fix QueryInput placeholder on gaant (β€œShow Step Subset”)

Test Plan

Run snapshot 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.Tue, Feb 11, 3:32 PM
bengotow edited the summary of this revision. (Show Details)Tue, Feb 11, 3:39 PM
bengotow edited the summary of this revision. (Show Details)Tue, Feb 11, 3:47 PM
bengotow updated this revision to Diff 9503.Tue, Feb 11, 4:07 PM

Update types

alangenfeld added inline comments.Tue, Feb 11, 4:24 PM
js_modules/dagit/src/FeatureFlagsRoot.tsx
2–32

I'm team "just nuke it and bring it back out of this diff if we need it later"

burn-it-down

js_modules/dagit/src/PipelineExplorer.tsx
227

same as other comment

js_modules/dagit/src/execute/SolidSelector.tsx
160

we should figure out consistent language for this - maybe its solid subset but im inclined to find a name we can use in both pipeline explorer and execution monitor

js_modules/dagit/src/gaant/GaantChart.tsx
215

lets use more common english here

hide pending steps
hide not started steps
hide not yet started steps

263

[1]

282

[1]

297–425

wouldn't hurt to sprinkle this with some comments in case a mortal has to edit this file in the future

304

[1]

js_modules/dagit/src/gaant/GaantChartTimescale.tsx
11–52

this deserves a comment

92

[1] nit: I prefer consistent { } + new lines to make it hard to miss continue/return when skimming. Definitely a personal bias so up to on changing or not

156

πŸ˜ƒ

js_modules/dagit/src/gaant/ZoomSlider.tsx
6–7

oof

js_modules/dagit/src/runs/RunActionButtons.tsx
110–112

does this full label play well with small browser windows?

figure out consistent language for this

this doesn't block this diff - just something we should discuss. Theres a good chance we need to give the selector syntax / subset syntax A Nameβ„’ to make it easier to talk about and find documentation for

thoughts after playing with it:

  • are we ignoring pipeline level events? seems like things are stalled until we get the first step level events - we maybe should move the timing window along in some sort of "projected" state but definitely should as we get engine events / pipeline level events
  • the single step re-execution button doesn't feel great - the text is likely to get truncated all the time and having the same prefix as the button next to it feels off to me personally. Not sure what to do here.

This is pretty slick. A few product things:

  • The table of contents is not showing for non-time-based Gaant chart views. It definitely should.
  • We should have an easy way to navigate directly from table of contents to a filtered view of the dag. e,g. if foo.compute is on the right perhaps a double click (@alangenfeld has thoughts here) should filter the dag down to *foo.compute
  • I had a crash with a big dag running while doing a bunch of typing in the subselector.

  • I'm very confused about the current behavior of the slowest path at this point. Seemingly gives some nonsense.
  • I'm finding I frequently have to type "*" again when I don't feel like I need to. At a minimum I think we need a visual indicator when there are no steps selected so that the user knows what is going on.

The core animation and viewer feels pretty amazing. great work.

alangenfeld accepted this revision.Tue, Feb 11, 7:30 PM

pending a pass over existing feedback

werenotworthy

This revision is now accepted and ready to land.Tue, Feb 11, 7:30 PM
prha added a comment.Tue, Feb 11, 7:35 PM

Some polish feedback:

  • The dep lines are sometimes drawn over the solid (many_events pipeline)

  • Getting some graphql client errors for some historical runs:

  • Scroll area for the event log view seems cut off

  • This isn't new, but it's accentuated now, but the single step execution is a bit confusing... once I have started a single step reexecution, the option to perform it again is gone, even though there's only one step. I have to perform another click to apply the single step filter before the option is available.

bengotow planned changes to this revision.Tue, Feb 11, 7:57 PM
bengotow added inline comments.
js_modules/dagit/src/execute/SolidSelector.tsx
160

Yeah - I'd be up for Type a filter or Filter using query or something like that... might have to think about this more broadly.

js_modules/dagit/src/gaant/GaantChart.tsx
215

Sounds good! Hide not started steps seems reasonable.

297–425

Will do - I'm sure I'll forget what's going on here as well.

js_modules/dagit/src/gaant/GaantChartTimescale.tsx
11–52

Sounds good!

92

Ahh good call - I usually do that, not sure why I was on the one-line-statement train here. Will fix these.

js_modules/dagit/src/runs/RunActionButtons.tsx
110–112

Not terribly well... it's actually surprisingly hard to make a blueprint button "compress" as space runs out, so I did this as a stopgap. I think when we make a proper selection bar this will be resolved so going to leave it as-is for now.

bengotow updated this revision to Diff 9560.Wed, Feb 12, 12:27 AM
  • Add comments, address diff feedback
  • Conditionally add preset queries, don’t add any that are β€œβ€
  • Fix inifinite traversal - algo is visiting nodes repeatedly
  • Clearing a preset should reset filter to β€œ*”
  • Add basic double-click-to-query behavior
  • Pass the current time through layout adjustment, tick continuously for smooth effect
This revision is now accepted and ready to land.Wed, Feb 12, 12:27 AM

Test failures appear spurious?

Test failures appear spurious?

I think you need to rebase

So I think a reasonable resolution to "undo" the selection of *foo*: If you double click again it toggles back to "*"

My only other feedback would be that it would be great to vertically scroll to the node that is currently executing when you single click on it in the table of contents

So I think a reasonable resolution to "undo" the selection of *foo*: If you double click again it toggles back to "*"

Ahh cool yeah this seems reasonable for now, esp given that the feature isn't super discoverable. Will add this!

bengotow updated this revision to Diff 9575.Wed, Feb 12, 6:47 AM
  • Merge branch 'master' of github.com:dagster-io/dagster into sfbg/gaant-v2
  • Toggle query when double clicking
  • Slowest path should only consider finished nodes