Page MenuHomePhabricator

asset-store-dagit-1 show asset store event in run's Logs
ClosedPublic

Authored by yuhan on Oct 29 2020, 5:41 PM.

Details

Summary

show AssetStoreOperation events in Run Viewer's logs

Asset page

Test Plan

dgait

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

yuhan added reviewers: dish, schrockn.
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 29 2020, 5:57 PM
Harbormaster failed remote builds in B20429: Diff 24772!
yuhan requested review of this revision.Oct 29 2020, 6:20 PM

So expanding on my comment: perhaps the other way to think about it how this is distinguished (if at all) from an AssetMaterialization event that comes from the asset store. I think most of the value will be in the user-defined information in the event (e.g. a generated s3 path or whatever) so i'm not sure how much value this event, unmodified, adds

python_modules/dagster/dagster/core/events/__init__.py
842–850

I think it's very important that this is user-pluggable in the asset store. There is a lot of store-specific metadata that someone would want (e.g. an s3 path)

gonna allow set_asset to yield AssetMaterialization first

python_modules/dagster/dagster/core/events/__init__.py
842–850

good call. im working on https://github.com/dagster-io/dagster/issues/3138 now and will make this diff stacked on it

so i'm not sure how much value this event, unmodified, adds

@schrockn My thinking had been that not all calls to set_asset should result in an entry in the Asset Catalog, because an entry for every intermediate would result in a lot of clutter. One way of achieving that is to expect users to yield AssetMaterializations when they want entries to appear in the Asset Catalog. If we go that route, then the value of having an AssetStoreOperation event is that it's there even when we're not adding entries to the Asset Catalog.

I had a suggestion on the message text - otherwise this looks great.

python_modules/dagster/dagster/core/events/__init__.py
840

The messages for other events appear to include a past tense verb, e.g. "Materialized value" or "Yielded output". I think it could aid readability to aim for consistency with that.

E.g. for SET_ASSET operations, the message could be 'Stored output {output_name} using asset store "{asset_store_key}'", and, for GET_ASSET operations, the message could be 'Retrieved output {output_name} from step {step_key} using asset store "{asset_store_key}"'.

This revision is now accepted and ready to land.Fri, Nov 6, 4:46 AM
yuhan marked an inline comment as done.

event message

python_modules/dagster/dagster/core/events/__init__.py
840

good catch! updated: