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.

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

[ui] Move Observe Sources into the Materialize button #23764

Merged
merged 1 commit into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ import {
import {AssetLocation, useFindAssetLocation} from './useFindAssetLocation';
import {AssetLiveDataRefreshButton} from '../asset-data/AssetLiveDataProvider';
import {LaunchAssetExecutionButton} from '../assets/LaunchAssetExecutionButton';
import {LaunchAssetObservationButton} from '../assets/LaunchAssetObservationButton';
import {AssetKey} from '../assets/types';
import {DEFAULT_MAX_ZOOM, SVGViewport} from '../graph/SVGViewport';
import {useAssetLayout} from '../graph/asyncGraphLayout';
Expand Down Expand Up @@ -734,14 +733,6 @@ const AssetGraphExplorerWithData = ({
/>
</GraphQueryInputFlexWrap>
<AssetLiveDataRefreshButton />
<LaunchAssetObservationButton
preferredJobName={explorerPath.pipelineName}
scope={
selectedDefinitions.length
? {selected: selectedDefinitions.filter((a) => a.isObservable)}
: {all: allDefinitionsForMaterialize.filter((a) => a.isObservable)}
}
/>
<LaunchAssetExecutionButton
preferredJobName={explorerPath.pipelineName}
scope={
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export type AssetNodeMenuNode = {
isMaterializable: boolean;
isObservable: boolean;
isExecutable: boolean;
isPartitioned: boolean;
hasMaterializePermission: boolean;
};
};
Expand All @@ -45,10 +46,11 @@ export const useAssetNodeMenu = ({
const upstream = graphData ? Object.keys(graphData.upstream[node.id] ?? {}) : [];
const downstream = graphData ? Object.keys(graphData.downstream[node.id] ?? {}) : [];

const {executeItem, launchpadElement} = useExecuteAssetMenuItem(
node.assetKey.path,
node.definition,
const asset = React.useMemo(
() => ({assetKey: node.assetKey, ...node.definition}),
[node.definition, node.assetKey],
);
const {executeItem, launchpadElement} = useExecuteAssetMenuItem(asset);

const [showParents, setShowParents] = React.useState(false);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
import {Button, Icon, Menu, MenuItem, Popover, Spinner, Tooltip} from '@dagster-io/ui-components';
import {useContext} from 'react';
import {useContext, useMemo} from 'react';

import {
executionDisabledMessageForAssets,
useMaterializationAction,
} from './LaunchAssetExecutionButton';
import {useObserveAction} from './LaunchAssetObservationButton';
import {optionsForExecuteButton, useMaterializationAction} from './LaunchAssetExecutionButton';
import {assetDetailsPathForKey} from './assetDetailsPathForKey';
import {AssetTableDefinitionFragment} from './types/AssetTableFragment.types';
import {useDeleteDynamicPartitionsDialog} from './useDeleteDynamicPartitionsDialog';
import {useObserveAction} from './useObserveAction';
import {useReportEventsModal} from './useReportEventsModal';
import {useWipeModal} from './useWipeModal';
import {CloudOSSContext} from '../app/CloudOSSContext';
import {showSharedToaster} from '../app/DomUtils';
import {AssetKeyInput} from '../graphql/types';
import {MenuLink} from '../ui/MenuLink';
import {RepoAddress} from '../workspace/types';
import {workspacePathFromAddress} from '../workspace/workspacePath';
Expand All @@ -30,7 +28,17 @@ export const AssetActionMenu = (props: Props) => {
featureContext: {canSeeMaterializeAction},
} = useContext(CloudOSSContext);

const {executeItem, launchpadElement} = useExecuteAssetMenuItem(path, definition);
// Because the asset catalog loads a list of keys, and then definitions for SDAs,
// we don't re-fetch the key inside the definition of each asset. Re-attach the
// assetKey to the definition for the hook below.
const asset = useMemo(
() =>
definition
? {...definition, isPartitioned: !!definition?.partitionDefinition, assetKey: {path}}
: null,
[path, definition],
);
const {executeItem, launchpadElement} = useExecuteAssetMenuItem(asset);

const deletePartitions = useDeleteDynamicPartitionsDialog(
repoAddress && definition ? {repoAddress, assetKey: {path}, definition} : null,
Expand Down Expand Up @@ -120,49 +128,40 @@ export const AssetActionMenu = (props: Props) => {
};

export const useExecuteAssetMenuItem = (
path: string[],
definition: {
assetKey: AssetKeyInput;
isObservable: boolean;
isExecutable: boolean;
isPartitioned: boolean;
hasMaterializePermission: boolean;
} | null,
Comment on lines 130 to 137
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this type more closely resemble the Asset type so we can avoid needing to attach the asset key to the definition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like the inconsistencies are 1) isParittioned vs partitionDefinition and 2) assetKey in the object or not.

I think that we can update the AssetTableFragment to request these two fields and it'll align these. Going to do it in a quick separate PR bc it'll change a bunch of files.

) => {
const disabledMessage = definition
? executionDisabledMessageForAssets([definition])
: 'Asset definition not found in a code location';
const {materializeOption, observeOption} = optionsForExecuteButton(
definition ? [definition] : [],
{skipAllTerm: true},
);

const {
featureContext: {canSeeMaterializeAction},
} = useContext(CloudOSSContext);

const materialize = useMaterializationAction();
const observe = useObserveAction();
const loading = materialize.loading || observe.loading;

const [option, action] =
materializeOption.disabledReason && !observeOption.disabledReason
? [observeOption, observe.onClick]
: [materializeOption, materialize.onClick];

const disabledMessage = definition
? option.disabledReason
: 'Asset definition not found in a code location';

if (!canSeeMaterializeAction) {
return {executeItem: null, launchpadElement: null};
}

if (definition?.isExecutable && definition.isObservable && definition.hasMaterializePermission) {
return {
launchpadElement: null,
executeItem: (
<MenuItem
text="Observe"
icon={observe.loading ? <Spinner purpose="body-text" /> : 'observation'}
disabled={observe.loading}
onClick={(e) => {
void showSharedToaster({
intent: 'primary',
message: 'Initiating observation',
icon: 'observation',
});
observe.onClick([{path}], e);
}}
/>
),
};
}

return {
launchpadElement: materialize.launchpadElement,
executeItem: (
Expand All @@ -173,20 +172,19 @@ export const useExecuteAssetMenuItem = (
useDisabledButtonTooltipFix
>
<MenuItem
text="Materialize"
icon={materialize.loading ? <Spinner purpose="body-text" /> : 'materialization'}
disabled={!!disabledMessage || materialize.loading}
text={option.label}
icon={loading ? <Spinner purpose="body-text" /> : option.icon}
disabled={!!disabledMessage || loading}
onClick={(e) => {
if (!definition) {
return;
}
void showSharedToaster({
intent: 'primary',
message: 'Initiating materialization',
icon: 'materialization',
message: `Initiating...`,
});

materialize.onClick([{path}], e);
action([definition.assetKey], e);
}}
/>
</Tooltip>
Expand Down
31 changes: 11 additions & 20 deletions js_modules/dagster-ui/packages/ui-core/src/assets/AssetTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {
MenuItem,
NonIdealState,
Popover,
Tooltip,
} from '@dagster-io/ui-components';
import groupBy from 'lodash/groupBy';
import * as React from 'react';
Expand All @@ -23,12 +22,15 @@ import {CloudOSSContext} from '../app/CloudOSSContext';
import {useUnscopedPermissions} from '../app/Permissions';
import {QueryRefreshCountdown, RefreshState} from '../app/QueryRefresh';
import {useSelectionReducer} from '../hooks/useSelectionReducer';
import {testId} from '../testing/testId';
import {StaticSetFilter} from '../ui/BaseFilters/useStaticSetFilter';
import {VirtualizedAssetTable} from '../workspace/VirtualizedAssetTable';

type Asset = AssetTableFragment;

type AssetWithDefinition = AssetTableFragment & {
definition: NonNullable<AssetTableFragment['definition']>;
};

interface Props {
view: AssetViewType;
assets: Asset[];
Expand Down Expand Up @@ -158,24 +160,13 @@ export const AssetTable = ({
<div style={{flex: 1}} />
<QueryRefreshCountdown refreshState={refreshState} />
<Box flex={{alignItems: 'center', gap: 8}}>
{checkedAssets.some((c) => !c.definition) ? (
<Tooltip content="One or more selected assets are not software-defined and cannot be launched directly.">
<Button
intent="primary"
data-testid={testId('materialize-button')}
icon={<Icon name="materialization" />}
disabled
>
{checkedAssets.length > 1
? `Materialize (${checkedAssets.length.toLocaleString()})`
: 'Materialize'}
</Button>
</Tooltip>
) : (
<LaunchAssetExecutionButton
scope={{selected: checkedAssets.map((a) => ({...a.definition!, assetKey: a.key}))}}
/>
)}
<LaunchAssetExecutionButton
scope={{
selected: checkedAssets
.filter((a): a is AssetWithDefinition => !!a.definition)
.map((a) => ({...a.definition, assetKey: a.key})),
}}
/>
<MoreActionsDropdown
selected={checkedAssets}
clearSelection={() => onToggleAll(false)}
Expand Down
19 changes: 9 additions & 10 deletions js_modules/dagster-ui/packages/ui-core/src/assets/AssetView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {useAllAssets} from './AssetsCatalogTable';
import {AssetAutomaterializePolicyPage} from './AutoMaterializePolicyPage/AssetAutomaterializePolicyPage';
import {ChangedReasonsTag} from './ChangedReasons';
import {LaunchAssetExecutionButton} from './LaunchAssetExecutionButton';
import {LaunchAssetObservationButton} from './LaunchAssetObservationButton';
import {UNDERLYING_OPS_ASSET_NODE_FRAGMENT} from './UnderlyingOpsOrGraph';
import {AssetChecks} from './asset-checks/AssetChecks';
import {assetDetailsPathForKey} from './assetDetailsPathForKey';
Expand Down Expand Up @@ -279,7 +278,12 @@ export const AssetView = ({assetKey, headerBreadcrumbs, writeAssetVisit, current
}, [path, selectedTab, setCurrentPage]);

const wipe = useWipeModal(
definition ? {assetKey: definition.assetKey, repository: definition.repository} : null,
definition && !definition.isObservable
? {
assetKey: definition.assetKey,
repository: definition.repository,
}
: null,
refresh,
);

Expand All @@ -292,7 +296,7 @@ export const AssetView = ({assetKey, headerBreadcrumbs, writeAssetVisit, current
);

const reportEvents = useReportEventsModal(
definition && repoAddress
definition && !definition.isObservable && repoAddress
? {
assetKey: definition.assetKey,
isPartitioned: definition.isPartitioned,
Expand Down Expand Up @@ -336,13 +340,8 @@ export const AssetView = ({assetKey, headerBreadcrumbs, writeAssetVisit, current
</div>
}
right={
<Box flex={{direction: 'row'}}>
{cachedOrLiveDefinition && cachedOrLiveDefinition.isObservable ? (
<LaunchAssetObservationButton
primary
scope={{all: [cachedOrLiveDefinition], skipAllTerm: true}}
/>
) : cachedOrLiveDefinition && cachedOrLiveDefinition.jobNames.length > 0 && upstream ? (
<Box style={{margin: '-4px 0'}} flex={{direction: 'row', gap: 8}}>
{cachedOrLiveDefinition && cachedOrLiveDefinition.jobNames.length > 0 && upstream ? (
<LaunchAssetExecutionButton
scope={{all: [cachedOrLiveDefinition]}}
showChangedAndMissingOption={false}
Expand Down
Loading