Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace GrapheneAssetNode isSource with isMaterializable #23401

Merged
merged 1 commit into from
Aug 7, 2024
Merged

replace GrapheneAssetNode isSource with isMaterializable #23401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 16 additions & 17 deletions js_modules/dagster-ui/packages/ui-core/src/asset-graph/AssetNode.tsx
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop

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
Open in desktop

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically this also hides it if the definition does not exist, I think in this case we may want to say (definition && !definition.isMaterializable) so that the !definition case doesn't also count

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gah I accidentally merged before fixing this. Here's a followup: #23490.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually – thinking about this more deeply after noticing a test failure on my followup PR branch, I think the expected behavior is actually that we do want to hide the tab if the definition does not exist. So I think this is right?

} 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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ export const AssetView = ({
};

const renderPartitionsTab = () => {
if (definition?.isSource) {
if (!definition?.isMaterializable) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same note as above, I think we want isMaterializable = false specifically. Maybe another way of writing this is definition?.isMaterializable === false, since the other falsy value is undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oo thanks for catching my ignoramus javascript, will fix

return <Redirect to={assetDetailsPathForKey(assetKey, {view: 'events'})} />;
}

Expand Down Expand Up @@ -567,11 +567,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
Open in desktop
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
Open in desktop
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