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

Jump to bottom

replace GrapheneAssetNode isSource with isMaterializable #23401

Merged
merged 1 commit into from
Aug 7, 2024
Merged

replace GrapheneAssetNode isSource with isMaterializable #23401

merged 1 commit into from
Aug 7, 2024

Conversation

sryza
Copy link
Contributor

@sryza sryza commented Aug 3, 2024 •
edited
Loading

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

How I Tested These Changes

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 3, 2024 •
edited
Loading

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-c35lovwti-elementl.vercel.app
https://is-source-is-materializable.core-storybook.dagster-docs.io

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

@sryza sryza changed the title replace GrapheneAssetNode isSource with isMaterializable replace GrapheneAssetNode isSource with isMaterializable Aug 3, 2024
@sryza sryza force-pushed the is-source-is-materializable branch from 05a154a to 3d10a43 Compare August 5, 2024 15:21
@sryza sryza requested a review from bengotow August 5, 2024 15:50
@sryza sryza force-pushed the is-source-is-materializable branch 2 times, most recently from 09958a5 to ce79b42 Compare August 5, 2024 16:50
@sryza sryza requested a review from salazarm August 5, 2024 16:51
@sryza sryza force-pushed the is-source-is-materializable branch from ce79b42 to e378057 Compare August 5, 2024 18:09
@bengotow
Copy link
Collaborator

bengotow commented Aug 5, 2024

Hey @sryza I think this makes sense, but how is isMaterializable different from isExecutable? maybe we merge those two as well?

bengotow
bengotow approved these changes Aug 5, 2024
View reviewed changes
js_modules/dagster-ui/packages/ui-core/src/assets/AssetTabs.tsx
@@ -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?

js_modules/dagster-ui/packages/ui-core/src/assets/AssetView.tsx
@@ -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

@sryza
Copy link
Contributor Author

sryza commented Aug 5, 2024

but how is isMaterializable different from isExecutable? maybe we merge those two as well?

isExecutable is equivalent to isObservable || isMaterializable. We could potentially remove it, though I think that it's possible we'll add other peers to "observe" and "materialize" in the future that would also be underneath the isExecutable umbrella. So isExecutable is useful in the situations where we just need to know whether there's a computation, but not what that computation is doing.

@sryza sryza merged commit 994f771 into master Aug 7, 2024
2 checks passed
@sryza sryza deleted the is-source-is-materializable branch August 7, 2024 21:01
sryza added a commit that referenced this pull request Aug 7, 2024
@sryza
Revert "replace GrapheneAssetNode isSource with isMaterializable (
a62919e 
#23401)"

This reverts commit 994f771.
@sryza sryza mentioned this pull request Aug 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bengotow bengotow bengotow approved these changes

@salazarm salazarm Awaiting requested review from salazarm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sryza @bengotow