RFC: Add register_run_asset event #7098

clairelin135 · 2022-03-16T23:51:46Z

Adds a new Dagster event type REGISTER_RUN_ASSET.

This event is yielded after an asset job run begins for every single asset that is selected in the job. The goal of this feature is to enable querying for runs that intended to generate certain assets (e.g. job that intends to materialize 3 assets fails during materialization of second asset).

This will enable warnings in Dagit on the asset details page and the asset graph nodes. In the future, we can enable this for partitioned assets to generate views of all runs across a certain asset's partition. If this event becomes too noisy in run logs, we can consider ways to condense the logs.

vercel · 2022-03-16T23:51:49Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

dagster – ./docs/next

🔍 Inspect: https://vercel.com/elementl/dagster/2wk6U5ghVUNcnJ3pFNAPaVicBnNs
✅ Preview: https://dagster-git-claire-intent-to-materialize-event-elementl.vercel.app

[Deployment for 394c3c1 canceled]

dagit-storybook – ./js_modules/dagit/packages/ui

🔍 Inspect: https://vercel.com/elementl/dagit-storybook/6L7guFy7j9Uxq4qQZpaKR8mFb3aj
✅ Preview: Canceled

[Deployment for 394c3c1 canceled]

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

python_modules/dagster/dagster/core/definitions/pipeline_definition.py

prha · 2022-03-17T17:37:52Z

I think my preference is to not show these events at all in the event log (run view).

I also think we should reconsider putting this code in the executor. One advantage of inserting events upon run creation as opposed to run execution is that you have insight into pre-start runs (e.g. queued runs, run launcher failures, etc).

…laire/intent-to-materialize-event

sryza · 2022-03-17T17:57:44Z

+1 to both things @prha said

clairelin135 · 2022-03-24T18:53:32Z

@alangenfeld I've changed the event name to asset_materialization_planned and updated the logs to display in Dagit when debug is selected. I think this is ready for you to take another look!

alangenfeld · 2022-03-24T19:07:27Z

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

 ASSET_EVENTS = {
    DagsterEventType.ASSET_MATERIALIZATION,


No, this is responsible for populating the assets tag on the job page. The list of assets is generated from the asset keys attached to these event types, and adding this event type enables fetching assets that were not successfully materialized.

ah I was referring to a code comment, i think i was commenting on a stale version of the PR

alangenfeld · 2022-03-24T19:08:40Z

python_modules/dagster/dagster/core/instance/__init__.py

+                        event = DagsterEvent.asset_materialization_planned(pipeline_name, asset_key)
+                        event_record = EventLogEntry(
+                            user_message="",
+                            level=logging.DEBUG,
+                            pipeline_name=pipeline_name,
+                            run_id=pipeline_run.run_id,
+                            error_info=None,
+                            timestamp=time.time(),
+                            dagster_event=event,
+                        )
+                        self.handle_new_event(event_record)


thoughts on putting this in line with the other DagsterEvent static constructors and logging there? Its a weird pattern but I think there is value in consistency.

Sure--I've updated the code now to log the dagster event in a static constructor.

alangenfeld · 2022-03-24T19:09:06Z

python_modules/dagster/dagster/core/storage/event_log/sqlite/sqlite_event_log.py

            )
+
            # mirror the event in the cross-run index database
            with self.index_connection() as conn:
                conn.execute(insert_event_statement)


[Re: lines 235 to 239]

why remove instead of update?

No strong reason here--I removed it earlier because I didn't feel that the check was necessary. I've updated it now though to contain the new event type

alangenfeld · 2022-03-24T19:17:03Z

python_modules/dagster/dagster/core/instance/__init__.py

+                        event = DagsterEvent.asset_materialization_planned(pipeline_name, asset_key)
+                        event_record = EventLogEntry(
+                            user_message="",
+                            level=logging.DEBUG,


might be good to leave reasoning next to the debug log level setting

Okay, added a comment next to the setting

alangenfeld

@prha what are your thoughts on this debug log level hide the event thing

alangenfeld · 2022-03-24T20:20:27Z

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

 ASSET_EVENTS = {
    DagsterEventType.ASSET_MATERIALIZATION,


ah I was referring to a code comment, i think i was commenting on a stale version of the PR

alangenfeld · 2022-03-25T15:58:07Z

js_modules/dagit/packages/core/src/runs/LogsScrollingTable.tsx

+    const l =
+      node.__typename === 'LogMessageEvent' ||
+      node.__typename === 'AssetMaterializationPlannedEvent'
+        ? node.level
+        : 'EVENT';


eesh - lets definitely leave a comment here explaining whats going on. I forgot that we classified events differently.

alangenfeld · 2022-03-25T16:00:17Z

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

+def log_asset_materialization_planned_event(
+    log_manager: DagsterLogManager, event: "DagsterEvent"
+) -> None:
+    # asset_materialization_planned events have a log level "DEBUG" in order to hide these
+    # events by default in Dagit. Modifying filtering to select DEBUG events will show these events
+    # in Dagit run logs.
+    log_level = logging.DEBUG
+    log_manager.log_dagster_event(level=log_level, msg=event.message or "", dagster_event=event)


no need to cargo-cult the pattern here and refactor when single use - this can just be inlined

alangenfeld · 2022-03-25T16:06:01Z

python_modules/dagster/dagster/core/instance/__init__.py

+        if execution_plan_snapshot:
+            self._log_asset_materialization_planned_events(pipeline_run, execution_plan_snapshot)
+
        return self._run_storage.add_run(pipeline_run)


might be good to sequence the event writes after the run gets added to the db? incase the run add fails

prha · 2022-03-25T16:15:12Z

I'm more inclined to just not show the events, regardless of level. We're only putting this in the event log so that we can query runs more efficiently by asset key. If there were a better implementation option that didn't result in visible events, we would pick that instead.

I think we should log them in debug mode but just not display them at all in the frontend. And we could change this later without major repercussions.

clairelin135 · 2022-03-25T17:59:57Z

@alangenfeld I don't feel strongly either way about whether to make these events viewable in Dagit or not, though I don't think users will find them very useful (the run will already show which assets intend to be materialized, and these events are only responsible for populating asset run information in Dagit). Defer to you to make the final call though

alangenfeld

Defer to you to make the final call though

I don't think I'm the best person to make final call on asset product experience questions. I don't have broader context, so things like

the run will already show which assets intend to be materialized

I don't know how that currently is presented.

As prha pointed out, this last bit is very easy to change so I will accept the PR and you can land it with the behavior you feel best about.

alangenfeld

[1] I believe filtering here is going to cause problems with the offset based pagination we currently have

alangenfeld · 2022-04-11T21:02:24Z

python_modules/dagster-graphql/dagster_graphql/schema/pipelines/pipeline.py

+        events = [
+            event
+            for event in events
+            if event.dagster_event_type != DagsterEventType.ASSET_MATERIALIZATION_PLANNED


alangenfeld · 2022-04-11T21:02:27Z

python_modules/dagster-graphql/dagster_graphql/implementation/execution/__init__.py

+        events = [
+            event
+            for event in events
+            if event.dagster_event_type != DagsterEventType.ASSET_MATERIALIZATION_PLANNED


I forgot about the offset cursors... I guess easiest thing to do is to just filter these events client-side?

ya, that or fix pagination to not be offset based 🙂

clairelin135 · 2022-04-11T22:02:36Z

@alangenfeld got it. I can adjust the filtering to filter client-side instead of here

add ASSET_INTENT_TO_MATERIALIZE event

5bc3304

clairelin135 commented Mar 16, 2022

View reviewed changes

python_modules/dagster/dagster/core/events/__init__.py Outdated Show resolved Hide resolved

mypy and formatting

d17f099

vercel bot temporarily deployed to Preview – dagit-storybook March 17, 2022 00:02 Inactive

vercel bot temporarily deployed to Preview – dagster March 17, 2022 00:02 Inactive

clairelin135 marked this pull request as ready for review March 17, 2022 00:14

clairelin135 requested review from prha, sryza and OwenKephart March 17, 2022 00:14

clairelin135 commented Mar 17, 2022

View reviewed changes

python_modules/dagster/dagster/core/definitions/pipeline_definition.py Outdated Show resolved Hide resolved

clairelin135 removed request for sryza, prha and OwenKephart March 17, 2022 00:49

clairelin135 added 2 commits March 17, 2022 10:32

add asset key property to step output

93750e6

fix backcompat and mypy

483fe24

vercel bot temporarily deployed to Preview – dagster March 17, 2022 17:32 Inactive

vercel bot temporarily deployed to Preview – dagit-storybook March 17, 2022 17:32 Inactive

Merge branch 'master' of https://github.com/dagster-io/dagster into c…

c066f0f

…laire/intent-to-materialize-event

vercel bot deployed to Preview – dagster March 17, 2022 17:43 View deployment

vercel bot temporarily deployed to Preview – dagit-storybook March 17, 2022 17:43 Inactive

clairelin135 added 3 commits March 17, 2022 15:47

create event upon run creation

8d7fe8f

filter out intent_to_materialize events from logs

f34aa3a

remove log from ts files

627c483

vercel bot temporarily deployed to Preview – dagster March 17, 2022 22:49 Inactive

vercel bot temporarily deployed to Preview – dagit-storybook March 17, 2022 22:49 Inactive

vercel bot temporarily deployed to Preview – dagit-storybook March 17, 2022 22:53 Inactive

vercel bot temporarily deployed to Preview – dagster March 17, 2022 22:53 Inactive

merge js types

de12cef

vercel bot deployed to Preview – dagster March 24, 2022 18:38 View deployment

vercel bot temporarily deployed to Preview – dagit-storybook March 24, 2022 18:38 Inactive

re add check

47b70ab

vercel bot temporarily deployed to Preview – dagster March 24, 2022 18:51 Inactive

vercel bot temporarily deployed to Preview – dagit-storybook March 24, 2022 18:51 Inactive

alangenfeld reviewed Mar 24, 2022

View reviewed changes

modify storing events in static method

9508bbb

vercel bot temporarily deployed to Preview – dagster March 24, 2022 21:39 Inactive

vercel bot temporarily deployed to Preview – dagit-storybook March 24, 2022 21:39 Inactive

isort

b25b36f

vercel bot temporarily deployed to Preview – dagster March 24, 2022 21:40 Inactive

vercel bot temporarily deployed to Preview – dagit-storybook March 24, 2022 21:41 Inactive

alangenfeld reviewed Mar 25, 2022

View reviewed changes

fix nits

feec198

vercel bot temporarily deployed to Preview – dagster March 25, 2022 17:47 Inactive

vercel bot temporarily deployed to Preview – dagit-storybook March 25, 2022 17:47 Inactive

alangenfeld approved these changes Mar 29, 2022

View reviewed changes

remove asset_materialization_planned from dagit run logs

394c3c1

vercel bot temporarily deployed to Preview – dagster March 31, 2022 22:40 Inactive

vercel bot temporarily deployed to Preview – dagit-storybook March 31, 2022 22:40 Inactive

clairelin135 merged commit 60059f9 into master Apr 1, 2022

clairelin135 deleted the claire/intent-to-materialize-event branch April 1, 2022 16:54

clairelin135 mentioned this pull request Apr 1, 2022

Update Materialization Warning #7265

Merged

alangenfeld reviewed Apr 11, 2022

View reviewed changes

clairelin135 mentioned this pull request Apr 12, 2022

Filter out asset_materialization_planned event logs client-side #7397

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Add register_run_asset event #7098

RFC: Add register_run_asset event #7098

clairelin135 commented Mar 16, 2022 •

edited

vercel bot commented Mar 16, 2022 •

edited

prha commented Mar 17, 2022

sryza commented Mar 17, 2022

clairelin135 commented Mar 24, 2022

alangenfeld Mar 24, 2022

clairelin135 Mar 24, 2022

alangenfeld Mar 24, 2022

alangenfeld Mar 24, 2022

clairelin135 Mar 24, 2022

alangenfeld Mar 24, 2022

clairelin135 Mar 24, 2022

alangenfeld Mar 24, 2022

clairelin135 Mar 24, 2022

alangenfeld left a comment

alangenfeld Mar 24, 2022

alangenfeld Mar 25, 2022

alangenfeld Mar 25, 2022

clairelin135 Mar 25, 2022

alangenfeld Mar 25, 2022

clairelin135 Mar 25, 2022

prha commented Mar 25, 2022

clairelin135 commented Mar 25, 2022

alangenfeld left a comment

alangenfeld left a comment

alangenfeld Apr 11, 2022

alangenfeld Apr 11, 2022

prha Apr 11, 2022

alangenfeld Apr 11, 2022

clairelin135 commented Apr 11, 2022

RFC: Add register_run_asset event #7098

RFC: Add register_run_asset event #7098

Conversation

clairelin135 commented Mar 16, 2022 • edited

vercel bot commented Mar 16, 2022 • edited

dagster – ./docs/next

dagit-storybook – ./js_modules/dagit/packages/ui

prha commented Mar 17, 2022

sryza commented Mar 17, 2022

clairelin135 commented Mar 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alangenfeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prha commented Mar 25, 2022

clairelin135 commented Mar 25, 2022

alangenfeld left a comment

Choose a reason for hiding this comment

alangenfeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairelin135 commented Apr 11, 2022

clairelin135 commented Mar 16, 2022 •

edited

vercel bot commented Mar 16, 2022 •

edited