Skip to content

Commit

Permalink
replace GrapheneAssetNode isSource with isMaterializable (#23401)
Browse files Browse the repository at this point in the history 
## Summary & Motivation

We are eliminating "source asset" as a concept in Dagster. The way we
use `isSource` in the UI is synonymous with "the asset is not
materializable". This PR removes `isSource` from `GrapheneAssetNode`,
adds `isMaterializable`, and updates the downstream frontend code.

The one user-facing change is that the asset details page will now say
"External Asset" instead of "Source Asset" when an asset is not
materializable:
![image](https://github.com/user-attachments/assets/a21cf63c-fbbc-4000-a19f-6b649cfd1f21)


## How I Tested These Changes
  • Loading branch information
@sryza
sryza authored Aug 7, 2024
1 parent 05f9414 commit 994f771
Show file tree
Hide file tree
Showing 36 changed files with 98 additions and 106 deletions.
33 changes: 16 additions & 17 deletions js_modules/dagster-ui/packages/ui-core/src/asset-graph/AssetNode.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ interface Props {
}

export const AssetNode = React.memo(({definition, selected, computeKindTagsFilter}: Props) => {
const isSource = definition.isSource;

const {liveData} = useAssetLiveData(definition.assetKey);
return (
<AssetInsetForHoverEffect>
Expand All @@ -45,7 +43,7 @@ export const AssetNode = React.memo(({definition, selected, computeKindTagsFilte
/>
</Box>
<AssetNodeContainer $selected={selected}>
<AssetNodeBox $selected={selected} $isSource={isSource}>
<AssetNodeBox $selected={selected} $isMaterializable={definition.isMaterializable}>
<AssetNameRow definition={definition} />
<Box style={{padding: '6px 8px'}} flex={{direction: 'column', gap: 4}} border="top">
{definition.description ? (
Expand All @@ -55,7 +53,7 @@ export const AssetNode = React.memo(({definition, selected, computeKindTagsFilte
) : (
<AssetDescription $color={Colors.textLight()}>No description</AssetDescription>
)}
{definition.isPartitioned && !definition.isSource && (
{definition.isPartitioned && definition.isMaterializable && (
<PartitionCountTags definition={definition} liveData={liveData} />
)}
</Box>
Expand All @@ -81,13 +79,13 @@ export const AssetNameRow = ({definition}: {definition: AssetNodeFragment}) => {
const displayName = definition.assetKey.path[definition.assetKey.path.length - 1]!;

return (
<AssetName $isSource={definition.isSource}>
<AssetName $isMaterializable={definition.isMaterializable}>
<span style={{marginTop: 1}}>
<Icon name={definition.isSource ? 'source_asset' : 'asset'} />
<Icon name={definition.isMaterializable ? 'asset' : 'source_asset'} />
</span>
<div
data-tooltip={displayName}
data-tooltip-style={definition.isSource ? NameTooltipStyleSource : NameTooltipStyle}
data-tooltip-style={definition.isMaterializable ? NameTooltipStyle : NameTooltipStyleSource}
style={{overflow: 'hidden', textOverflow: 'ellipsis'}}
>
{withMiddleTruncation(displayName, {
Expand Down Expand Up @@ -191,7 +189,7 @@ export const AssetNodeMinimal = ({
definition: AssetNodeFragment;
height: number;
}) => {
const {isSource, assetKey} = definition;
const {isMaterializable, assetKey} = definition;
const {liveData} = useAssetLiveData(assetKey);

const {border, background} = buildAssetNodeStatusContent({assetKey, definition, liveData});
Expand All @@ -214,7 +212,7 @@ export const AssetNodeMinimal = ({
>
<MinimalAssetNodeBox
$selected={selected}
$isSource={isSource}
$isMaterializable={isMaterializable}
$background={background}
$border={border}
$inProgress={!!inProgressRuns}
Expand All @@ -227,7 +225,7 @@ export const AssetNodeMinimal = ({
/>
) : null}
{isStale ? <MinimalNodeStaleDot assetKey={assetKey} liveData={liveData} /> : null}
<MinimalName style={{fontSize: 24}} $isSource={isSource}>
<MinimalName style={{fontSize: 24}} $isMaterializable={isMaterializable}>
{withMiddleTruncation(displayName, {maxLength: 18})}
</MinimalName>
</MinimalAssetNodeBox>
Expand All @@ -254,7 +252,7 @@ export const ASSET_NODE_FRAGMENT = gql`
computeKind
isPartitioned
isObservable
isSource
isMaterializable
assetKey {
...AssetNodeKey
}
Expand Down Expand Up @@ -290,12 +288,12 @@ const AssetNodeShowOnHover = styled.span`
`;

export const AssetNodeBox = styled.div<{
$isSource: boolean;
$isMaterializable: boolean;
$selected: boolean;
$noScale?: boolean;
}>`
${(p) =>
p.$isSource
!p.$isMaterializable
? `border: 2px dashed ${p.$selected ? Colors.accentGrayHover() : Colors.accentGray()}`
: `border: 2px solid ${
p.$selected ? Colors.lineageNodeBorderSelected() : Colors.lineageNodeBorder()
Expand Down Expand Up @@ -343,11 +341,12 @@ const NameTooltipStyleSource = JSON.stringify({
border: `none`,
});

const AssetName = styled.div<{$isSource: boolean}>`
const AssetName = styled.div<{$isMaterializable: boolean}>`
${NameCSS};
display: flex;
gap: 4px;
background: ${(p) => (p.$isSource ? Colors.backgroundLight() : Colors.lineageNodeBackground())};
background: ${(p) =>
p.$isMaterializable ? Colors.lineageNodeBackground() : Colors.backgroundLight()};
border-top-left-radius: 8px;
border-top-right-radius: 8px;
`;
Expand All @@ -357,7 +356,7 @@ const MinimalAssetNodeContainer = styled(AssetNodeContainer)`
`;

const MinimalAssetNodeBox = styled.div<{
$isSource: boolean;
$isMaterializable: boolean;
$selected: boolean;
$background: string;
$border: string;
Expand All @@ -367,7 +366,7 @@ const MinimalAssetNodeBox = styled.div<{
background: ${(p) => p.$background};
overflow: hidden;
${(p) =>
p.$isSource
!p.$isMaterializable
? `border: 4px dashed ${p.$selected ? Colors.accentGray() : p.$border}`
: `border: 4px solid ${p.$selected ? Colors.lineageNodeBorderSelected() : p.$border}`};
${(p) =>
Expand Down
2 changes: 1 addition & 1 deletion js_modules/dagster-ui/packages/ui-core/src/asset-graph/AssetNodeMenu.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export type AssetNodeMenuNode = {
id: string;
assetKey: AssetKeyInput;
definition: {
isSource: boolean;
isMaterializable: boolean;
isObservable: boolean;
isExecutable: boolean;
hasMaterializePermission: boolean;
Expand Down
4 changes: 2 additions & 2 deletions js_modules/dagster-ui/packages/ui-core/src/asset-graph/AssetNodeStatusContent.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const LOADING_STATUS_CONTENT = {

export type StatusContentArgs = {
assetKey: AssetKeyInput;
definition: {opNames: string[]; isSource: boolean; isObservable: boolean};
definition: {opNames: string[]; isMaterializable: boolean; isObservable: boolean};
liveData: LiveDataForNode | null | undefined;
expanded?: boolean;
};
Expand Down Expand Up @@ -400,7 +400,7 @@ export function _buildAssetNodeStatusContent({
};
}

if (!lastMaterialization && definition.isSource) {
if (!lastMaterialization && !definition.isMaterializable) {
return {
case: StatusCase.SOURCE_NO_STATE as const,
background: Colors.backgroundLight(),
Expand Down
2 changes: 1 addition & 1 deletion js_modules/dagster-ui/packages/ui-core/src/asset-graph/SidebarAssetInfo.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ export const SidebarAssetInfo = ({graphNode}: {graphNode: GraphNode}) => {

{assetType && <TypeSidebarSection assetType={assetType} />}

{asset.partitionDefinition && !definition.isSource && (
{asset.partitionDefinition && definition.isMaterializable && (
<SidebarSection title="Partitions">
<Box padding={{vertical: 16, horizontal: 24}} flex={{direction: 'column', gap: 16}}>
<p>{asset.partitionDefinition.description}</p>
Expand Down
6 changes: 3 additions & 3 deletions js_modules/dagster-ui/packages/ui-core/src/asset-graph/__fixtures__/AssetNode.fixtures.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export const AssetNodeFragmentBasic: AssetNodeFragment = buildAssetNode({
id: '["asset1"]',
isObservable: false,
isPartitioned: false,
isSource: false,
isMaterializable: true,
jobNames: ['job1'],
opNames: ['asset1'],
opVersion: '1',
Expand All @@ -82,13 +82,13 @@ export const AssetNodeFragmentSource = buildAssetNode({
description: 'This is a test source asset',
id: '["source_asset"]',
isObservable: true,
isSource: true,
isMaterializable: false,
jobNames: [],
opNames: [],
});

export const AssetNodeFragmentSourceOverdue = buildAssetNode({
isSource: true,
isMaterializable: false,
isObservable: false,
freshnessInfo: buildAssetFreshnessInfo({
currentMinutesLate: 12,
Expand Down
6 changes: 4 additions & 2 deletions ...ules/dagster-ui/packages/ui-core/src/asset-graph/__stories__/SidebarAssetInfo.stories.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const buildGraphNodeMock = (definitionOverrides: Partial<AssetNode>): GraphNode
graphName: null,
isPartitioned: false,
isObservable: false,
isSource: false,
isMaterializable: true,
...definitionOverrides,
}),
});
Expand Down Expand Up @@ -329,7 +329,9 @@ export const AssetWithDifferentOpName = () => {
export const ObservableSourceAsset = () => {
return (
<TestContainer>
<SidebarAssetInfo graphNode={buildGraphNodeMock({isObservable: true, isSource: true})} />
<SidebarAssetInfo
graphNode={buildGraphNodeMock({isObservable: true, isMaterializable: false})}
/>
</TestContainer>
);
};
4 changes: 2 additions & 2 deletions js_modules/dagster-ui/packages/ui-core/src/asset-graph/layout.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ export const ASSET_NODE_NAME_MAX_LENGTH = 38;
export const getAssetNodeDimensions = (def: {
assetKey: {path: string[]};
opNames: string[];
isSource: boolean;
isMaterializable: boolean;
isObservable: boolean;
isPartitioned: boolean;
graphName: string | null;
Expand All @@ -355,7 +355,7 @@ export const getAssetNodeDimensions = (def: {

let height = 106; // top tags area + name + description

if (def.isSource && def.isObservable) {
if (!def.isMaterializable && def.isObservable) {
height += 30; // status row
} else {
height += 28; // status row
Expand Down
2 changes: 1 addition & 1 deletion js_modules/dagster-ui/packages/ui-core/src/asset-graph/types/AssetNode.types.ts
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions js_modules/dagster-ui/packages/ui-core/src/asset-graph/types/useAssetGraphData.types.ts
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion js_modules/dagster-ui/packages/ui-core/src/asset-graph/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {AssetKeyInput} from '../graphql/types';
export function groupAssetsByStatus<
T extends {
key: AssetKeyInput;
definition?: {opNames: string[]; isSource: boolean; isObservable: boolean} | null;
definition?: {opNames: string[]; isMaterializable: boolean; isObservable: boolean} | null;
},
>(assets: T[], liveDataByNode: Record<string, LiveDataForNode>) {
type StatusesType = {
Expand Down
8 changes: 4 additions & 4 deletions js_modules/dagster-ui/packages/ui-core/src/assets/AssetEvents.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,10 @@ export const AssetEvents = ({
(json) => ({types: json?.types || ALL_EVENT_TYPES}),
);

// Source assets never have materializations, so we don't want to show the type filter
const hideFilters = assetNode?.isSource;
// Source assets never have a partitions tab, so we shouldn't allow links to it
const hidePartitionLinks = assetNode?.isSource;
// No need to show the type filter for assets without materializations
const hideFilters = !assetNode?.isMaterializable;
// Non-materializable assets never have a partitions tab, so we shouldn't allow links to it
const hidePartitionLinks = !assetNode?.isMaterializable;

const grouped = useGroupedEvents(
xAxis,
Expand Down
6 changes: 2 additions & 4 deletions js_modules/dagster-ui/packages/ui-core/src/assets/AssetNodeDefinition.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -296,9 +296,7 @@ const DescriptionAnnotations = ({
</Mono>
))}
<UnderlyingOpsOrGraph assetNode={assetNode} repoAddress={repoAddress} />
{assetNode.isSource ? (
<Caption style={{lineHeight: '16px'}}>Source Asset</Caption>
) : !assetNode.isExecutable ? (
{!assetNode.isMaterializable ? (
<Caption style={{lineHeight: '16px'}}>External Asset</Caption>
) : undefined}
</Box>
Expand All @@ -313,7 +311,7 @@ export const ASSET_NODE_DEFINITION_FRAGMENT = gql`
opNames
opVersion
jobNames
isSource
isMaterializable
isExecutable
tags {
key
Expand Down
2 changes: 1 addition & 1 deletion js_modules/dagster-ui/packages/ui-core/src/assets/AssetTableFragment.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export const ASSET_TABLE_DEFINITION_FRAGMENT = gql`
changedReasons
groupName
opNames
isSource
isMaterializable
isObservable
isExecutable
computeKind
Expand Down
2 changes: 1 addition & 1 deletion js_modules/dagster-ui/packages/ui-core/src/assets/AssetTabs.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export const buildAssetTabMap = (input: AssetTabConfigInput) => {
id: 'partitions',
title: 'Partitions',
to: buildAssetViewParams({...params, view: 'partitions'}),
hidden: !definition?.partitionDefinition || definition?.isSource,
hidden: !definition?.partitionDefinition || !definition?.isMaterializable,
} as AssetTabConfig,
checks: {
id: 'checks',
Expand Down
8 changes: 2 additions & 6 deletions js_modules/dagster-ui/packages/ui-core/src/assets/AssetView.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ export const AssetView = ({assetKey, headerBreadcrumbs, writeAssetVisit, current
};

const renderPartitionsTab = () => {
if (definition?.isSource) {
if (!definition?.isMaterializable) {
return <Redirect to={assetDetailsPathForKey(assetKey, {view: 'events'})} />;
}

Expand Down Expand Up @@ -553,11 +553,7 @@ const AssetViewPageHeaderTags = ({
/>
</>
) : null}
{definition?.isSource ? (
<Tag>Source Asset</Tag>
) : !definition?.isExecutable ? (
<Tag>External Asset</Tag>
) : undefined}
{!definition?.isMaterializable ? <Tag>External Asset</Tag> : undefined}
</>
);
};
4 changes: 2 additions & 2 deletions js_modules/dagster-ui/packages/ui-core/src/assets/CalculateUnsyncedDialog.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export const CalculateUnsyncedDialog = React.memo(
const unsynced = React.useMemo(
() =>
(data?.assetNodes || [])
.filter((node) => !node.isSource && (isAssetStale(node) || isAssetMissing(node)))
.filter((node) => node.isMaterializable && (isAssetStale(node) || isAssetMissing(node)))
.map(asAssetKeyInput),
[data],
);
Expand Down Expand Up @@ -208,7 +208,7 @@ const ASSET_STALE_STATUS_QUERY = gql`
assetKey {
path
}
isSource
isMaterializable
staleStatus
partitionStats {
numMaterialized
Expand Down
4 changes: 2 additions & 2 deletions js_modules/dagster-ui/packages/ui-core/src/assets/LaunchAssetChoosePartitionsDialog.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ export const LAUNCH_ASSET_WARNINGS_QUERY = gql`
query LaunchAssetWarningsQuery($upstreamAssetKeys: [AssetKeyInput!]!) {
assetNodes(assetKeys: $upstreamAssetKeys) {
id
isSource
isMaterializable
assetKey {
path
}
Expand Down Expand Up @@ -755,7 +755,7 @@ const Warnings = ({
(upstreamAssets || [])
.filter(
(a) =>
!a.isSource &&
a.isMaterializable &&
a.partitionDefinition &&
displayedPartitionDefinition &&
partitionDefinitionsEqual(a.partitionDefinition, displayedPartitionDefinition),
Expand Down
Loading

1 comment on commit 994f771

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

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

Please sign in to comment.