Skip to content

Commit

Permalink
[Asset graph] Make tight-tree the default layout algorithm (#17723)
Browse files Browse the repository at this point in the history
## Summary & Motivation

In _most_ cases this algorithm behaves the same as network-simplex while
being much much faster so we're making this the default.


## How I Tested These Changes
Local version of dagit, made sure the asset graph still loads
  • Loading branch information
salazarm authored Nov 7, 2023
1 parent cba181b commit c72d9cb
Show file tree
Hide file tree
Showing 10 changed files with 16 additions and 205 deletions.
1 change: 0 additions & 1 deletion js_modules/dagster-ui/packages/ui-core/src/app/Flags.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ export const FeatureFlag = {
flagDisableAutoLoadDefaults: 'flagDisableAutoLoadDefaults' as const,
flagDAGSidebar: 'flagDAGSidebar' as const,
flagDisableDAGCache: 'flagDisableDAGCache' as const,
flagTightTreeDag: 'flagTightTreeDag' as const,
flagLongestPathDag: 'flagLongestPathDag' as const,
};
export type FeatureFlagType = keyof typeof FeatureFlag;
Expand Down
18 changes: 2 additions & 16 deletions js_modules/dagster-ui/packages/ui-core/src/app/GraphQueryImpl.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
import {isPlannedDynamicStep, dynamicKeyWithoutIndex} from '../gantt/DynamicStepSupport';

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

const MAX_RENDERED_FOR_EMPTY_QUERY = 100;

export interface GraphQueryItem {
name: string;
inputs: {
Expand Down Expand Up @@ -87,17 +83,8 @@ function expansionDepthForClause(clause: string) {
}

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: !fastDag && items.length >= MAX_RENDERED_FOR_EMPTY_QUERY ? [] : items,
applyingEmptyDefault: !fastDag && items.length >= MAX_RENDERED_FOR_EMPTY_QUERY,
focus: [],
};
if (query === '*' || query === '') {
return {all: items, focus: []};
}

const traverser = new GraphTraverser<T>(items);
Expand Down Expand Up @@ -137,6 +124,5 @@ export function filterByQuery<T extends GraphQueryItem>(items: T[], query: strin
return {
all: Array.from(results),
focus: Array.from(focus),
applyingEmptyDefault: false,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -57,24 +57,6 @@ export const getVisibleFeatureFlagRows = () => [
key: 'Disable Asset Graph caching',
flagType: FeatureFlag.flagDisableDAGCache,
},
{
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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,10 @@ import {
RightInfoPanel,
RightInfoPanelContent,
} from '../pipelines/GraphExplorer';
import {
EmptyDAGNotice,
EntirelyFilteredDAGNotice,
LargeDAGNotice,
LoadingNotice,
} from '../pipelines/GraphNotices';
import {EmptyDAGNotice, EntirelyFilteredDAGNotice, LoadingNotice} from '../pipelines/GraphNotices';
import {ExplorerPath} from '../pipelines/PipelinePathUtils';
import {GraphQueryInput} from '../ui/GraphQueryInput';
import {Loading, LoadingSpinner} from '../ui/Loading';
import {Loading} from '../ui/Loading';

import {AssetEdges} from './AssetEdges';
import {AssetGraphJobSidebar} from './AssetGraphJobSidebar';
Expand Down Expand Up @@ -71,19 +66,8 @@ export const MINIMAL_SCALE = 0.6;
export const GROUPS_ONLY_SCALE = 0.15;

export const AssetGraphExplorer = (props: Props) => {
const {
fetchResult,
assetGraphData,
fullAssetGraphData,
graphQueryItems,
allAssetKeys,
applyingEmptyDefault,
isCalculating,
} = useAssetGraphData(props.explorerPath.opsQuery, props.fetchOptions);

if (isCalculating) {
return <LoadingSpinner purpose="page" />;
}
const {fetchResult, assetGraphData, fullAssetGraphData, graphQueryItems, allAssetKeys} =
useAssetGraphData(props.explorerPath.opsQuery, props.fetchOptions);

return (
<Loading allowStaleData queryResult={fetchResult}>
Expand All @@ -110,7 +94,6 @@ export const AssetGraphExplorer = (props: Props) => {
fullAssetGraphData={fullAssetGraphData}
allAssetKeys={allAssetKeys}
graphQueryItems={graphQueryItems}
applyingEmptyDefault={applyingEmptyDefault}
{...props}
/>
);
Expand All @@ -124,7 +107,6 @@ interface WithDataProps extends Props {
assetGraphData: GraphData;
fullAssetGraphData: GraphData;
graphQueryItems: AssetGraphQueryItem[];
applyingEmptyDefault: boolean;
}

const AssetGraphExplorerWithData = ({
Expand All @@ -136,7 +118,6 @@ const AssetGraphExplorerWithData = ({
assetGraphData,
fullAssetGraphData,
graphQueryItems,
applyingEmptyDefault,
fetchOptions,
fetchOptionFilters,
allAssetKeys,
Expand All @@ -155,9 +136,7 @@ const AssetGraphExplorerWithData = ({
const lastSelectedNode = selectedGraphNodes[selectedGraphNodes.length - 1]!;

const selectedDefinitions = selectedGraphNodes.map((a) => a.definition);
const allDefinitionsForMaterialize = applyingEmptyDefault
? graphQueryItems.map((a) => a.node)
: Object.values(assetGraphData.nodes).map((a) => a.definition);
const allDefinitionsForMaterialize = Object.values(assetGraphData.nodes).map((a) => a.definition);

const onSelectNode = React.useCallback(
async (
Expand Down Expand Up @@ -301,8 +280,6 @@ const AssetGraphExplorerWithData = ({
<ErrorBoundary region="graph">
{graphQueryItems.length === 0 ? (
<EmptyDAGNotice nodeType="asset" isGraph />
) : applyingEmptyDefault ? (
<LargeDAGNotice nodeType="asset" anchorLeft="40px" />
) : Object.keys(assetGraphData.nodes).length === 0 ? (
<EntirelyFilteredDAGNotice nodeType="asset" />
) : undefined}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ const MARGIN = 100;

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

Expand All @@ -51,11 +50,7 @@ export const layoutAssetGraph = (
): AssetGraphLayout => {
const g = new dagre.graphlib.Graph({compound: true});

const ranker = opts.tightTree
? 'tight-tree'
: opts.longestPath
? 'longest-path'
: 'network-simplex';
const ranker = opts.longestPath ? 'longest-path' : 'tight-tree';

g.setGraph(
opts.horizontalDAGs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,8 @@ 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';
import {AssetGroupSelector, PipelineSelector} from '../graphql/types';

import {ASSET_NODE_FRAGMENT} from './AssetNode';
Expand Down Expand Up @@ -73,21 +71,20 @@ export function useAssetGraphData(opsQuery: string, options: AssetGraphFetchScop
[fullGraphQueryItems],
);

const {assetGraphData, graphAssetKeys, allAssetKeys, applyingEmptyDefault} = React.useMemo(() => {
const {assetGraphData, graphAssetKeys, allAssetKeys} = React.useMemo(() => {
if (repoFilteredNodes === undefined || graphQueryItems === undefined) {
return {
graphAssetKeys: [],
graphQueryItems: [],
assetGraphData: null,
applyingEmptyDefault: false,
};
}

// Filter the set of all AssetNodes down to those matching the `opsQuery`.
// 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);
const {all} = 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 @@ -100,71 +97,16 @@ export function useAssetGraphData(opsQuery: string, options: AssetGraphFetchScop
graphAssetKeys: all.map((n) => ({path: n.node.assetKey.path})),
assetGraphData,
graphQueryItems,
applyingEmptyDefault,
};
}, [repoFilteredNodes, graphQueryItems, opsQuery, options.hideEdgesToNodesOutsideQuery]);

// Used to avoid showing "query error"
const [isCalculating, setIsCalculating] = React.useState<boolean>(true);
const [isCached, setIsCached] = React.useState<boolean>(false);
const [assetGraphDataMaybeCached, setAssetGraphDataMaybeCached] =
React.useState<GraphData | null>(null);

const {flagDisableDAGCache} = useFeatureFlags();

React.useLayoutEffect(() => {
setAssetGraphDataMaybeCached(null);
setIsCached(false);
setIsCalculating(true);
let cancel = false;
(async () => {
let fullAssetGraphData = null;
if (applyingEmptyDefault && repoFilteredNodes && !flagDisableDAGCache) {
// build the graph data anyways to check if it's cached
const {all} = filterByQuery(graphQueryItems, '*');
fullAssetGraphData = buildGraphData(all.map((n) => n.node));
if (options.hideEdgesToNodesOutsideQuery) {
removeEdgesToHiddenAssets(fullAssetGraphData, repoFilteredNodes);
}
if (fullAssetGraphData) {
const isCached = await asyncGetFullAssetLayoutIndexDB.isCached(fullAssetGraphData);
if (cancel) {
return;
}
if (isCached) {
setAssetGraphDataMaybeCached(fullAssetGraphData);
setIsCached(true);
setIsCalculating(false);
return;
}
}
}
setAssetGraphDataMaybeCached(assetGraphData);
setIsCached(false);
setIsCalculating(false);
})();

return () => {
cancel = true;
};
}, [
applyingEmptyDefault,
graphQueryItems,
options.hideEdgesToNodesOutsideQuery,
repoFilteredNodes,
assetGraphData,
flagDisableDAGCache,
]);

return {
fetchResult,
assetGraphData: assetGraphDataMaybeCached,
assetGraphData,
fullAssetGraphData,
graphQueryItems,
graphAssetKeys,
allAssetKeys,
applyingEmptyDefault: applyingEmptyDefault && !isCached,
isCalculating,
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ const _assetLayoutCacheKey = (graphData: GraphData, opts: LayoutAssetGraphOption
return newObj;
}

return `${opts?.horizontalDAGs ? 'horizontal:' : ''}${opts?.tightTree ? 'tight-tree:' : ''}${
return `${opts?.horizontalDAGs ? 'horizontal:' : ''}${
opts?.longestPath ? 'longest-path' : ''
}${JSON.stringify({
downstream: recreateObjectWithKeysSorted(graphData.downstream),
Expand Down Expand Up @@ -181,7 +181,6 @@ export function useAssetLayout(graphData: GraphData) {
const opts = React.useMemo(
() => ({
horizontalDAGs: flags.flagHorizontalDAGs,
tightTree: flags.flagTightTreeDag,
longestPath: flags.flagLongestPathDag,
}),
[flags],
Expand Down
2 changes: 1 addition & 1 deletion js_modules/dagster-ui/packages/ui-core/src/graph/layout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ export function layoutOpGraph(pipelineOps: ILayoutOp[], opts: LayoutOpGraphOptio
}

// Define a new top-down, left to right graph layout
g.setGraph({rankdir: 'TB', marginx, marginy});
g.setGraph({rankdir: 'TB', marginx, marginy, ranker: 'tight-tree'});
g.setDefaultEdgeLabel(() => ({}));

const edges: OpLayoutEdge[] = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,7 @@ import {OpNameOrPath} from '../ops/OpNameOrPath';
import {GraphQueryInput} from '../ui/GraphQueryInput';
import {RepoAddress} from '../workspace/types';

import {
EmptyDAGNotice,
EntirelyFilteredDAGNotice,
LargeDAGNotice,
LoadingNotice,
} from './GraphNotices';
import {EmptyDAGNotice, EntirelyFilteredDAGNotice, LoadingNotice} from './GraphNotices';
import {ExplorerPath} from './PipelinePathUtils';
import {SIDEBAR_ROOT_CONTAINER_FRAGMENT} from './SidebarContainerOverview';
import {SidebarRoot} from './SidebarRoot';
Expand Down Expand Up @@ -164,10 +159,7 @@ export const GraphExplorer = (props: GraphExplorerProps) => {
solids.some((f) => f.definition.__typename === 'CompositeSolidDefinition'));

const queryResultOps = React.useMemo(
() =>
solidsQueryEnabled
? filterByQuery(solids, opsQuery)
: {all: solids, focus: [], applyingEmptyDefault: false},
() => (solidsQueryEnabled ? filterByQuery(solids, opsQuery) : {all: solids, focus: []}),
[opsQuery, solids, solidsQueryEnabled],
);

Expand Down Expand Up @@ -262,8 +254,6 @@ export const GraphExplorer = (props: GraphExplorerProps) => {

{solids.length === 0 ? (
<EmptyDAGNotice nodeType="op" isGraph={isGraph} />
) : queryResultOps.applyingEmptyDefault ? (
<LargeDAGNotice nodeType="op" />
) : Object.keys(queryResultOps.all).length === 0 ? (
<EntirelyFilteredDAGNotice nodeType="op" />
) : undefined}
Expand Down
Loading

1 comment on commit c72d9cb

@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-enosh9rke-elementl.vercel.app

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

Please sign in to comment.