Skip to content

Commit

Permalink
[dagit] Remove asset graph bundling based on path prefixes, experimen…
Browse files Browse the repository at this point in the history 
…tal flag
  • Loading branch information
bengotow committed May 31, 2022
1 parent 79a86ec commit 33a5946
Show file tree
Hide file tree
Showing 5 changed files with 9 additions and 241 deletions.
1 change: 0 additions & 1 deletion js_modules/dagit/packages/core/src/app/Flags.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {getJSONForKey} from '../hooks/useStateWithStorage';
export const DAGIT_FLAGS_KEY = 'DAGIT_FLAGS';

export enum FeatureFlag {
flagExperimentalAssetDAG = 'flagExperimentalAssetDAG',
flagDebugConsoleLogging = 'flagDebugConsoleLogging',
flagAlwaysCollapseNavigation = 'flagAlwaysCollapseNavigation',
flagDisableWebsockets = 'flagDisableWebsockets',
Expand Down
10 changes: 0 additions & 10 deletions js_modules/dagit/packages/core/src/app/UserSettingsRoot.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,16 +124,6 @@ const UserSettingsRoot: React.FC<SettingsRootProps> = ({tabs}) => {
/>
),
},
{
key: 'Experimental asset graph display',
value: (
<Checkbox
format="switch"
checked={flags.includes(FeatureFlag.flagExperimentalAssetDAG)}
onChange={() => toggleFlag(FeatureFlag.flagExperimentalAssetDAG)}
/>
),
},
{
key: 'New partitions view (experimental)',
value: (
Expand Down
117 changes: 2 additions & 115 deletions js_modules/dagit/packages/core/src/asset-graph/AssetGraphExplorer.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,4 @@
import {
Body,
Box,
Checkbox,
Colors,
Icon,
Mono,
NonIdealState,
SplitPanelContainer,
} from '@dagster-io/ui';
import {Box, Checkbox, Colors, NonIdealState, SplitPanelContainer} from '@dagster-io/ui';
import flatMap from 'lodash/flatMap';
import isEqual from 'lodash/isEqual';
import pickBy from 'lodash/pickBy';
Expand All @@ -18,7 +9,6 @@ import React from 'react';
import {useHistory} from 'react-router-dom';
import styled from 'styled-components/macro';

import {useFeatureFlags} from '../app/Flags';
import {GraphQueryItem} from '../app/GraphQueryImpl';
import {
FIFTEEN_SECONDS,
Expand Down Expand Up @@ -168,7 +158,6 @@ const AssetGraphExplorerWithData: React.FC<

const history = useHistory();
const findJobForAsset = useFindJobForAsset();
const {flagExperimentalAssetDAG: experiments} = useFeatureFlags();

const [highlighted, setHighlighted] = React.useState<string | null>(null);

Expand Down Expand Up @@ -344,99 +333,6 @@ const AssetGraphExplorerWithData: React.FC<
<SVGContainer width={layout.width} height={layout.height}>
<AssetConnectedEdges highlighted={highlighted} edges={layout.edges} />

{Object.values(layout.bundles)
.sort((a, b) => a.id.length - b.id.length)
.map(({id, bounds}) => {
if (experiments && _scale < EXPERIMENTAL_MINI_SCALE) {
const path = JSON.parse(id);
return (
<foreignObject
x={bounds.x}
y={bounds.y}
width={bounds.width}
height={bounds.height + 10}
key={id}
onDoubleClick={(e) => {
viewportEl.current?.zoomToSVGBox(bounds, true, 1.2);
e.stopPropagation();
}}
>
<AssetNodeMinimal
style={{
border: `${2 / _scale}px solid ${Colors.Gray300}`,
background: Colors.White,
}}
selected={selectedGraphNodes.some((g) =>
hasPathPrefix(g.assetKey.path, path),
)}
>
<NameMinimal
style={{
fontSize: 18 / _scale,
display: 'flex',
flexDirection: 'column',
alignItems: 'center',
}}
>
<div
style={{display: 'flex', gap: 3 / _scale, alignItems: 'center'}}
>
<Icon
name="folder"
size={Math.round(16 / _scale) as any}
style={{marginTop: 4}}
/>
{withMiddleTruncation(displayNameForAssetKey({path}), {
maxLength: 17,
})}
</div>
<Body style={{fontSize: 10 / _scale, color: Colors.Gray500}}>
{layout.bundleMapping[id].length} items
</Body>
</NameMinimal>
</AssetNodeMinimal>
</foreignObject>
);
}
return (
<foreignObject
x={bounds.x}
y={bounds.y}
width={bounds.width}
height={bounds.height + 10}
style={{pointerEvents: 'none'}}
key={id}
>
<Mono
style={{
opacity:
_scale > EXPERIMENTAL_MINI_SCALE
? (_scale - EXPERIMENTAL_MINI_SCALE) / 0.2
: 0,
fontWeight: 600,
display: 'flex',
gap: 6,
}}
>
<Icon name="folder" size={20} />
{displayNameForAssetKey({path: JSON.parse(id)})}
</Mono>
<div
style={{
inset: 0,
top: 24,
position: 'absolute',
borderRadius: 10,
border: `${3 / _scale}px dashed ${Colors.Gray300}`,
background: `rgba(223, 223, 223, ${
0.4 - Math.max(0, _scale - EXPERIMENTAL_MINI_SCALE) * 0.3
})`,
}}
/>
</foreignObject>
);
})}

{Object.values(layout.nodes).map(({id, bounds}, index) => {
const graphNode = assetGraphData.nodes[id];
const path = JSON.parse(id);
Expand All @@ -451,15 +347,6 @@ const AssetGraphExplorerWithData: React.FC<
) : null;
}

if (experiments) {
const isWithinBundle = Object.keys(layout.bundles).some((bundleId) =>
hasPathPrefix(path, JSON.parse(bundleId)),
);
if (isWithinBundle && _scale < EXPERIMENTAL_MINI_SCALE) {
return null;
}
}

return (
<foreignObject
{...bounds}
Expand All @@ -475,7 +362,7 @@ const AssetGraphExplorerWithData: React.FC<
>
{!graphNode || !graphNode.definition.opNames.length ? (
<ForeignNode assetKey={{path}} />
) : experiments && _scale < EXPERIMENTAL_MINI_SCALE ? (
) : _scale < EXPERIMENTAL_MINI_SCALE ? (
<AssetNodeMinimal
style={{background: Colors.White}}
selected={selectedGraphNodes.includes(graphNode)}
Expand Down
57 changes: 0 additions & 57 deletions js_modules/dagit/packages/core/src/asset-graph/Utils.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import {pathVerticalDiagonal} from '@vx/shape';
import uniq from 'lodash/uniq';

import {AssetNodeDefinitionFragment} from '../assets/types/AssetNodeDefinitionFragment';

Expand Down Expand Up @@ -47,62 +46,6 @@ export const isSourceAsset = (node: {jobNames: string[]; opNames: string[]}) =>
return node.jobNames.length === 0 && !node.opNames.length;
};

export function identifyBundles(assetIds: string[]) {
const pathPrefixes: {[prefixId: string]: string[]} = {};

for (const assetId of assetIds) {
const assetKeyPath = JSON.parse(assetId);

for (let ii = 1; ii < assetKeyPath.length; ii++) {
const prefix = assetKeyPath.slice(0, ii);
const key = JSON.stringify(prefix);
pathPrefixes[key] = pathPrefixes[key] || [];
pathPrefixes[key].push(assetId);
}
}

for (const key of Object.keys(pathPrefixes)) {
if (pathPrefixes[key].length <= 1) {
delete pathPrefixes[key];
}
}

const finalBundlePrefixes: {[prefixId: string]: string[]} = {};
const finalBundleIdForNodeId: {[id: string]: string} = {};

// Sort the prefix keys by length descending and iterate from the deepest folders first.
// Dedupe asset keys and replace asset keys we've already seen with the (deeper) folder
// they are within. This gets us "multi layer folders" of nodes.

// Turn this:
// {
// "s3": [["s3", "collect"], ["s3", "prod", "a"], ["s3", "prod", "b"]],
// "s3/prod": ["s3", "prod", "a"], ["s3", "prod", "b"]
// }

// Into this:
// {
// "s3/prod": ["s3", "prod", "a"], ["s3", "prod", "b"]
// "s3": [["s3", "collect"], ["s3", "prod"]],
// }

for (const prefixId of Object.keys(pathPrefixes).sort((a, b) => b.length - a.length)) {
const contents = uniq(
pathPrefixes[prefixId].map((p) =>
finalBundleIdForNodeId[p] ? finalBundleIdForNodeId[p] : p,
),
);
if (contents.length === 1 && finalBundlePrefixes[contents[0]]) {
// If this bundle contains exactly one bundle, no need to show both outlines.
// Just show the inner one. eg: a > b > asset1, a > b > asset2, just show a > b.
continue;
}
finalBundlePrefixes[prefixId] = contents;
finalBundlePrefixes[prefixId].forEach((id) => (finalBundleIdForNodeId[id] = prefixId));
}
return finalBundlePrefixes;
}

export const buildGraphData = (assetNodes: AssetNode[]) => {
const data: GraphData = {
nodes: {},
Expand Down
65 changes: 7 additions & 58 deletions js_modules/dagit/packages/core/src/asset-graph/layout.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as dagre from 'dagre';

import {IBounds, IPoint} from '../graph/common';

import {GraphData, GraphNode, GraphId, displayNameForAssetKey, identifyBundles} from './Utils';
import {GraphData, GraphNode, GraphId, displayNameForAssetKey} from './Utils';

export interface AssetLayout {
id: GraphId;
Expand All @@ -23,12 +23,6 @@ export type AssetGraphLayout = {
height: number;
edges: AssetLayoutEdge[];
nodes: {[id: string]: AssetLayout};

bundles: {[id: string]: AssetLayout};
bundleEdges: AssetLayoutEdge[];
bundleMapping: {
[prefixId: string]: string[];
};
};

const opts: {margin: number; mini: boolean} = {
Expand Down Expand Up @@ -88,19 +82,6 @@ export const layoutAssetGraph = (graphData: GraphData): AssetGraphLayout => {
g.setNode(id, getForeignNodeDimensions(id));
});

// Create "parent" nodes for nodes with a shared ID (path) prefix (eg: s3>a, s3>b),
// and then place the children inside. Note that the bundles are identified in order
// and bundleMapping can reference bundles as children - this code can create multiple
// layers of parents!
const bundleMapping = identifyBundles(g.nodes());

for (const [parentId, nodeIds] of Object.entries(bundleMapping)) {
g.setNode(parentId, {});
for (const nodeId of nodeIds) {
g.setParent(nodeId, parentId);
}
}

dagre.layout(g);

const dagreNodesById: {[id: string]: dagre.Node} = {};
Expand All @@ -116,7 +97,6 @@ export const layoutAssetGraph = (graphData: GraphData): AssetGraphLayout => {
let maxHeight = 0;

const nodes: {[id: string]: AssetLayout} = {};
const bundles: {[id: string]: AssetLayout} = {};

Object.keys(dagreNodesById).forEach((id) => {
const dagreNode = dagreNodesById[id];
Expand All @@ -126,58 +106,27 @@ export const layoutAssetGraph = (graphData: GraphData): AssetGraphLayout => {
width: dagreNode.width,
height: dagreNode.height,
};
if (bundleMapping[id]) {
return;
}
nodes[id] = {id, bounds};

// If this node was inside one or more parent nodes, upsert the parent box
// into the bundles result set and expand it to include the child. Note:
// dagre does give us "parent" node dimensions, but sometimes they're randomly
// much larger than the contents.
let bundleId = g.parent(id);
while (bundleId) {
bundles[bundleId] = bundles[bundleId] || {id: bundleId, bounds};
bundles[bundleId].bounds = extendBounds(bundles[bundleId].bounds, {
x: bounds.x - opts.margin / 4,
y: bounds.y - opts.margin / 4,
width: bounds.width + opts.margin / 2,
height: bounds.height + opts.margin / 2,
});
bundleId = g.parent(bundleId);
}
maxWidth = Math.max(maxWidth, dagreNode.x + dagreNode.width / 2);
maxHeight = Math.max(maxHeight, dagreNode.y + dagreNode.height / 2);
});

const edges: AssetLayoutEdge[] = [];
const bundleEdges: AssetLayoutEdge[] = [];

g.edges().forEach((e) => {
const points = g.edge(e).points;
if (bundles[e.v] || bundles[e.w]) {
bundleEdges.push({
from: points[0],
fromId: e.v,
to: points[points.length - 1],
toId: e.w,
});
} else {
edges.push({
from: points[0],
fromId: e.v,
to: points[points.length - 1],
toId: e.w,
});
}
edges.push({
from: points[0],
fromId: e.v,
to: points[points.length - 1],
toId: e.w,
});
});

return {
nodes,
edges,
bundles,
bundleEdges,
bundleMapping,
width: maxWidth + opts.margin,
height: maxHeight + opts.margin,
};
Expand Down

0 comments on commit 33a5946

Please sign in to comment.