Skip to content

Commit

Permalink
Add longest-path algorithm feature flag + revert flipping of horizont…
Browse files Browse the repository at this point in the history
…al/vertical logic. (#17241)

## Summary & Motivation

1. Add the longest path algorithm
2. Revert the flipping of horizontal/vertical logic since it doesn't
seem to work correctly for tight-tree. We'll need to mess around with
the options passed to dagre to figure out a robust solution for tight
tree. The current settings don't seem to consistently make the asset
graph horizontal or vertical 🤷🏾

## How I Tested These Changes
Locally
  • Loading branch information
salazarm authored Oct 16, 2023
1 parent 34ae5eb commit 67c0cbf
Show file tree
Hide file tree
Showing 7 changed files with 91 additions and 54 deletions.
1 change: 1 addition & 0 deletions js_modules/dagster-ui/packages/ui-core/src/app/Flags.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export const FeatureFlag = {
flagDisableDAGCache: 'flagDisableDAGCache' as const,
flagEnableAMPTimeline: 'flagEnableAMPTimeline' as const,
flagTightTreeDag: 'flagTightTreeDag' as const,
flagLongestPathDag: 'flagLongestPathDag' as const,
};
export type FeatureFlagType = keyof typeof FeatureFlag;

Expand Down
14 changes: 7 additions & 7 deletions js_modules/dagster-ui/packages/ui-core/src/app/GraphQueryImpl.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import {isPlannedDynamicStep, dynamicKeyWithoutIndex} from '../gantt/DynamicStepSupport';

import {featureEnabled, FeatureFlag} from './Flags';

const MAX_RENDERED_FOR_EMPTY_QUERY = 100;

export interface GraphQueryItem {
Expand Down Expand Up @@ -84,18 +86,16 @@ function expansionDepthForClause(clause: string) {
return clause.includes('*') ? Number.MAX_SAFE_INTEGER : clause.length;
}

export function filterByQuery<T extends GraphQueryItem>(
items: T[],
query: string,
flagTightTreeDag?: boolean,
) {
export function filterByQuery<T extends GraphQueryItem>(items: T[], query: string) {
if (query === '*') {
return {all: items, applyingEmptyDefault: false, focus: []};
}
const fastDag =
featureEnabled(FeatureFlag.flagTightTreeDag) || featureEnabled(FeatureFlag.flagLongestPathDag);
if (query === '') {
return {
all: !flagTightTreeDag && items.length >= MAX_RENDERED_FOR_EMPTY_QUERY ? [] : items,
applyingEmptyDefault: !flagTightTreeDag && items.length >= MAX_RENDERED_FOR_EMPTY_QUERY,
all: !fastDag && items.length >= MAX_RENDERED_FOR_EMPTY_QUERY ? [] : items,
applyingEmptyDefault: !fastDag && items.length >= MAX_RENDERED_FOR_EMPTY_QUERY,
focus: [],
};
}
Expand Down
13 changes: 9 additions & 4 deletions js_modules/dagster-ui/packages/ui-core/src/app/Util.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,15 @@ export function indexedDBAsyncMemoize<T, R, U extends (arg: T, ...rest: any[]) =
const hash = hashFn ? hashFn(arg, ...rest) : arg;

const encoder = new TextEncoder();
const data = encoder.encode(hash.toString());
const hashBuffer = await crypto.subtle.digest('SHA-1', data);
const hashArray = Array.from(new Uint8Array(hashBuffer)); // convert buffer to byte array
return hashArray.map((b) => b.toString(16).padStart(2, '0')).join(''); // convert bytes to hex string
// Crypto.subtle isn't defined in insecure contexts... fallback to using the full string as a key
// https://stackoverflow.com/questions/46468104/how-to-use-subtlecrypto-in-chrome-window-crypto-subtle-is-undefined
if (crypto.subtle?.digest) {
const data = encoder.encode(hash.toString());
const hashBuffer = await crypto.subtle.digest('SHA-1', data);
const hashArray = Array.from(new Uint8Array(hashBuffer)); // convert buffer to byte array
return hashArray.map((b) => b.toString(16).padStart(2, '0')).join(''); // convert bytes to hex string
}
return hash.toString();
}

const ret = (async (arg: T, ...rest: any[]) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,39 @@ export const getVisibleFeatureFlagRows = () => [
flagType: FeatureFlag.flagEnableAMPTimeline,
},
{
key: 'Experimental fast tight tree DAG algorithm',
key: 'Experimental tight tree DAG algorithm (Faster)',
flagType: FeatureFlag.flagTightTreeDag,
label: (
<Box flex={{direction: 'column'}}>
Experimental tight tree asset graph algorithm (Faster).
<div>
<a
href="https://github.com/dagster-io/dagster/discussions/17240"
target="_blank"
rel="noreferrer"
>
Learn more
</a>
</div>
</Box>
),
},
{
key: 'Experimental longest path DAG algorithm (Faster)',
flagType: FeatureFlag.flagLongestPathDag,
label: (
<Box flex={{direction: 'column'}}>
Experimental longest path asset graph algorithm (Faster).
<div>
<a
href="https://github.com/dagster-io/dagster/discussions/17240"
target="_blank"
rel="noreferrer"
>
Learn more
</a>
</div>
</Box>
),
},
];
58 changes: 32 additions & 26 deletions js_modules/dagster-ui/packages/ui-core/src/asset-graph/layout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,39 +39,45 @@ const GROUP_NODE_PREFIX = 'group__';

const MARGIN = 100;

export type LayoutAssetGraphOptions = {horizontalDAGs: boolean; tightTree: boolean};
export type LayoutAssetGraphOptions = {
horizontalDAGs: boolean;
tightTree: boolean;
longestPath: boolean;
};

export const layoutAssetGraph = (
graphData: GraphData,
opts: LayoutAssetGraphOptions,
): AssetGraphLayout => {
const g = new dagre.graphlib.Graph({compound: true});

const option1 = {
rankdir: 'LR',
marginx: MARGIN,
marginy: MARGIN,
nodesep: -10,
edgesep: 90,
ranksep: 60,
ranker: opts.tightTree ? 'tight-tree' : 'network-simplex',
};
const option2 = {
rankdir: 'TB',
marginx: MARGIN,
marginy: MARGIN,
nodesep: 40,
edgesep: 10,
ranksep: 10,
ranker: opts.tightTree ? 'tight-tree' : 'network-simplex',
};
let horizontal = option1;
let vertical = option2;
if (opts.tightTree) {
horizontal = option2;
vertical = option1;
}
g.setGraph(opts.horizontalDAGs ? horizontal : vertical);
const ranker = opts.tightTree
? 'tight-tree'
: opts.longestPath
? 'longest-path'
: 'network-simplex';

g.setGraph(
opts.horizontalDAGs
? {
rankdir: 'LR',
marginx: MARGIN,
marginy: MARGIN,
nodesep: -10,
edgesep: 90,
ranksep: 60,
ranker,
}
: {
rankdir: 'TB',
marginx: MARGIN,
marginy: MARGIN,
nodesep: 40,
edgesep: 10,
ranksep: 10,
ranker,
},
);
g.setDefaultEdgeLabel(() => ({}));

const parentNodeIdForNode = (node: GraphNode) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import keyBy from 'lodash/keyBy';
import reject from 'lodash/reject';
import React from 'react';

import {useFeatureFlags} from '../app/Flags';
import {filterByQuery, GraphQueryItem} from '../app/GraphQueryImpl';
import {AssetKey} from '../assets/types';
import {asyncGetFullAssetLayoutIndexDB} from '../graph/asyncGraphLayout';
Expand Down Expand Up @@ -73,8 +72,6 @@ export function useAssetGraphData(opsQuery: string, options: AssetGraphFetchScop
[fullGraphQueryItems],
);

const {flagTightTreeDag} = useFeatureFlags();

const {assetGraphData, graphAssetKeys, allAssetKeys, applyingEmptyDefault} = React.useMemo(() => {
if (repoFilteredNodes === undefined || graphQueryItems === undefined) {
return {
Expand All @@ -89,7 +86,7 @@ export function useAssetGraphData(opsQuery: string, options: AssetGraphFetchScop
// In the future it might be ideal to move this server-side, but we currently
// get to leverage the useQuery cache almost 100% of the time above, making this
// super fast after the first load vs a network fetch on every page view.
const {all, applyingEmptyDefault} = filterByQuery(graphQueryItems, opsQuery, flagTightTreeDag);
const {all, applyingEmptyDefault} = filterByQuery(graphQueryItems, opsQuery);

// Assemble the response into the data structure used for layout, traversal, etc.
const assetGraphData = buildGraphData(all.map((n) => n.node));
Expand All @@ -104,13 +101,7 @@ export function useAssetGraphData(opsQuery: string, options: AssetGraphFetchScop
graphQueryItems,
applyingEmptyDefault,
};
}, [
repoFilteredNodes,
graphQueryItems,
opsQuery,
options.hideEdgesToNodesOutsideQuery,
flagTightTreeDag,
]);
}, [repoFilteredNodes, graphQueryItems, opsQuery, options.hideEdgesToNodesOutsideQuery]);

// Used to avoid showing "query error"
const [isCalculating, setIsCalculating] = React.useState<boolean>(true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,21 +179,23 @@ export function useAssetLayout(graphData: GraphData) {
const flags = useFeatureFlags();

const opts = React.useMemo(
() => ({horizontalDAGs: flags.flagHorizontalDAGs, tightTree: flags.flagTightTreeDag}),
() => ({
horizontalDAGs: flags.flagHorizontalDAGs,
tightTree: flags.flagTightTreeDag,
longestPath: flags.flagLongestPathDag,
}),
[flags],
);

const cacheKey = _assetLayoutCacheKey(graphData, opts);
const nodeCount = Object.keys(graphData.nodes).length;
const runAsync = nodeCount >= ASYNC_LAYOUT_SOLID_COUNT;

const {flagDisableDAGCache} = useFeatureFlags();

React.useEffect(() => {
async function runAsyncLayout() {
dispatch({type: 'loading'});
let layout;
if (!flagDisableDAGCache) {
if (!flags.flagDisableDAGCache) {
layout = await asyncGetFullAssetLayoutIndexDB(graphData, opts);
} else {
layout = await asyncGetFullAssetLayout(graphData, opts);
Expand All @@ -207,7 +209,7 @@ export function useAssetLayout(graphData: GraphData) {
} else {
void runAsyncLayout();
}
}, [cacheKey, graphData, runAsync, flags, flagDisableDAGCache, opts]);
}, [cacheKey, graphData, runAsync, flags, opts]);

return {
loading: state.loading || !state.layout || state.cacheKey !== cacheKey,
Expand Down

1 comment on commit 67c0cbf

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-qhzvgo3ue-elementl.vercel.app

Built with commit 67c0cbf.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.