Page MenuHomePhabricator
Differential D4038

displaying asset types in the asset menu
AbandonedPublic
Actions

Authored by max on Jul 29 2020, 9:21 PM.

Details

Reviewers
prha
schrockn
• leoeer
sandyryza
Summary

adding a column in the asset view to display asset types

Test Plan

Unit

Diff Detail

Repository
R1 dagster
Branch
assetdisplay (branched from master)
Lint
Lint Skipped
Unit
No Unit Test Coverage

Event Timeline

• leoeer created this revision.Jul 29 2020, 9:21 PM
• leoeer updated this revision to Diff 19759.Jul 29 2020, 9:34 PM

flake

Harbormaster failed remote builds in B16178: Diff 19757!Jul 29 2020, 9:36 PM
• leoeer published this revision for review.Jul 29 2020, 9:36 PM

the build failure is due to a legacy example having issues

Harbormaster failed remote builds in B16180: Diff 19759!Jul 29 2020, 9:48 PM
• leoeer added a comment.Jul 29 2020, 9:56 PM

image in dagit

• leoeer added a reviewer: max.Jul 29 2020, 9:58 PM
prha added inline comments.Jul 29 2020, 11:53 PM
js_modules/dagit/src/assets/AssetsRoot.tsx
241

missed this in the original review.... should this be dagster-type-name?

if we're moving towards having this be used in more places, we may want to consider storing this as a the top-level materialization field instead of jammed in with metadata.

287

It seems pretty heavy to have this be populated for every single asset. Sometimes you just want to record an asset materialization without a type.

• leoeer updated this revision to Diff 19802.Jul 30 2020, 3:03 PM

phil's changes

• leoeer updated this revision to Diff 19803.Jul 30 2020, 3:16 PM

rebase

Harbormaster failed remote builds in B16218: Diff 19802!Jul 30 2020, 3:17 PM
Harbormaster completed remote builds in B16219: Diff 19803.Jul 30 2020, 3:31 PM
prha requested changes to this revision.Jul 30 2020, 8:16 PM
prha added inline comments.
js_modules/dagit/src/assets/AssetsRoot.tsx
283

I think we should hide this column if it's not present for any of the assets. We're fetching all the data anyway so it shouldn't be a problem to check.

287

We should default to this being blank

This revision now requires changes to proceed.Jul 30 2020, 8:16 PM
schrockn added a comment.Jul 30 2020, 9:15 PM

Yeah the more I think about the more I think this is really heavy and doesn't really take the asset manager in a direction we want to

sandyryza resigned from this revision.Sep 9 2020, 6:12 AM
max commandeered this revision.Tue, Jan 5, 8:34 PM
max abandoned this revision.
max edited reviewers, added: • leoeer; removed: max.

Revision Contents
Changeset List

PathSize
js_modules/
dagit/
src/
assets/
26 lines
types/
python_modules/
dagster/
dagster/
core/
definitions/
4 lines
dagster_tests/
core_tests/
10 lines

Diff 19803

View Options

js_modules/dagit/src/assets/AssetsRoot.tsx

Loading...
View Options

js_modules/dagit/src/assets/types/AssetsRootQuery.ts

Loading...
View Options

python_modules/dagster/dagster/core/definitions/events.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/test_output_materialization.py

Loading...