Page MenuHomePhabricator

Initial pass at custom modification of the Dagre layout for wide pipelines
ClosedPublic

Authored by bengotow on Tue, Aug 20, 3:56 PM.

Details

Summary

This diff adds some post-layout logic that adjusts the output provided by the graph
visualization library Dagre. Dagre does not support specifying max width/height on
it's output, so if we detect that the graph contains a row of more than 25 nodes, we
manually "split" it into multiple rows, shifting the rest of the graph down to make
space.

This normally works great, but it has a few caveats I've found:

  • If the really long line of nodes are connected to a lot of subsequent nodes, reflowing the row essentially breaks the "shortest path" logic that Dagre used. Eg: "A1 : A2, B1 : B2" x 50 may become "A1 : [A-Z]?2", which doesn't look as nice.
  • If the long line is two chunks of nodes with horizontal whitespace in between, we still apply our logic and also reflow the nodes together into a single block. We could turn this off, but in the interest of bounding the graph width it seemed OK.

Before:

After: Note this is 25 nodes per row, but we can choose any max. I'd suggest keeping it high enough it's 100% better than the alternative.

EDIT: Here's what this looks like on Nick's other pipeline:

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.Tue, Aug 20, 3:56 PM
bengotow edited the summary of this revision. (Show Details)Tue, Aug 20, 3:57 PM
bengotow updated this revision to Diff 3848.Tue, Aug 20, 5:13 PM
bengotow edited the summary of this revision. (Show Details)

Rebase

do we have any tests for dagre?

schrockn requested changes to this revision.Tue, Aug 20, 8:59 PM
schrockn added a subscriber: schrockn.

I'd also love some screenshots from pipelines generated with https://dagster.phacility.com/D846

js_modules/dagit/src/graph/getFullSolidLayout.ts
181

this seems like the type of code we should start unit testing

This revision now requires changes to proceed.Tue, Aug 20, 8:59 PM
bengotow edited the summary of this revision. (Show Details)Wed, Aug 21, 7:08 PM

Hmm, this is /sorta/ incorporated into the snapshot tests, but I think we could definitely create some tests around dagre and this overall layout logic. Actually, we may be able to render out SVGs of given DAGs and then compare the SVGs, which would allow you to see WHAT changed when the code is modified / broken (I think otherwise this is just going to be comparing 200kb of JSON full of numbers and they'll just get updated blindly.) Will see if I can set this up today.

schrockn accepted this revision.Thu, Aug 22, 9:44 PM

accepting because of fun tests in other diff

This revision is now accepted and ready to land.Thu, Aug 22, 9:44 PM