Add a new AssetExecutionType (Observable) and use it to omit materializations for observable source asset wrapping #16621

schrockn · 2023-09-19T13:56:56Z

Summary & Motivation

This adds a new execution type (Observation) which does not automatically produce a materialization on execution. This allows us to drive observation from a common execution pathway, rather than a completely parallel system.

How I Tested These Changes

BK

schrockn · 2023-09-19T13:57:12Z

Current dependencies on/for this PR:

master
- PR Source Asset -> Unexecutable Asset Adapter #16617
  - PR Extract _get_op_def_compute_fn into wrap_source_asset_observe_fn_in_op_compute_fn #16618
    - PR Wrap observable source assets in AssetsDefinition #16620
      - PR Add Playground Example folder and Observable source asset test #17063
      - PR Add a new AssetExecutionType (Observable) and use it to omit materializations for observable source asset wrapping #16621 👈
        
        PR @observable_source_asset adapter to generate and SensorDefinition #16716
        
        PR Make observable source adapter layer compatibility with AMP #16718
        
        PR RFC: @observable_source_asset adapter to generate AssetsDefinition and SensorDefinition #16712
        
        PR RFC: @observable_source_asset adapter to generate AssetsDefinition and SensorDefinition #16623
    - PR Wrap observable source assets in AssetsDefinition #16619

This comment was auto-generated by Graphite.

alangenfeld

I think this is fine - think we need these metadata bits to be able to set the right bools on external asset node

alangenfeld · 2023-09-20T17:02:07Z

python_modules/dagster/dagster/_core/execution/plan/execute_step.py

+        asset_layer = step_context.job_def.asset_layer
+        execution_type = (
+            asset_layer.assets_def_for_asset(asset_key).asset_execution_type_for_asset(asset_key)
+            if asset_layer.has_assets_def_for_asset(asset_key)
+            else AssetExecutionType.MATERIALIZATION
+        )
+
+        check.invariant(
+            execution_type != AssetExecutionType.UNEXECUTABLE,
+            "There should never be unexecutable assets here",
+        )
+        if execution_type == AssetExecutionType.MATERIALIZATION:
+            for materialization in _get_output_asset_materializations(
+                asset_key,
+                partitions,
+                output,
+                output_def,
+                manager_metadata,
+                step_context,
+            ):
+                yield DagsterEvent.asset_materialization(step_context, materialization)
+        elif execution_type == AssetExecutionType.OBSERVATION:
+            # if this is an observation execution type, yield no materializations
+            pass
+        else:
+            check.failed(f"Unexpected asset execution type {execution_type}")


hmm not sure if theres a way to make this feel cleaner, take another second to stare at it and see if you can come up with something

schrockn · 2023-09-22T02:55:19Z

python_modules/dagster/dagster/_core/execution/plan/execute_step.py

+        yield from (
+            (
+                DagsterEvent.asset_materialization(step_context, materialization)
+                for materialization in _get_output_asset_materializations(
+                    asset_key,
+                    partitions,
+                    output,
+                    output_def,
+                    manager_metadata,
+                    step_context,
+                )
+            )
+            if execution_type == AssetExecutionType.MATERIALIZATION
+            else ()
+        )


@alangenfeld I think this is nicer put arguably a little overly clever as it's a little obscured that it is making a lazily computed geneator 🤷

schrockn · 2023-10-06T18:19:33Z

@alangenfeld just confirming that I am planning on landing this after #16620 goes in

## Summary & Motivation This makes an observable source asset wrappable in an AssetsDefinition. This is an imperfect intermediate state, as as spurious asset materialization is produced as a result of execution (verified in test case but fixed in #16621) but this verifies that the core execution machinery works. Makes the following code possible: ```python @observable_source_asset def an_observable_source_asset() -> DataVersion: return DataVersion("foo") defs = Definitions(assets=[ create_external_asset_from_source_asset(an_observable_source_asset) ]) ``` This `@observable_source_asset` now plugs into the rest of our infrastructure: 1) You can execute it from `dagster-webserver` using the "Materialize" button, which will be renamed to "Execute". 2) You can schedule this from schedules and sensors. AMP support has to wait because auto observation is hard-coded in the AMP logic. ## How I Tested These Changes BK

…izations for observable source asset wrapping more bulletproof codepath defend against non-existent asset_defs better better still f-string

github-actions · 2023-10-09T08:02:16Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-jb90f0ubm-elementl.vercel.app
https://source-asset-wrapping-4-new-obserable-asset-execution-type.core-storybook.dagster-docs.io

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

schrockn mentioned this pull request Sep 19, 2023

RFC: @observable_source_asset adapter to generate AssetsDefinition and SensorDefinition #16623

Closed

schrockn force-pushed the source-asset-wrapping-3-executable-observable-asset branch from 061cda4 to d01b7bc Compare September 20, 2023 03:42

schrockn force-pushed the source-asset-wrapping-4-new-obserable-asset-execution-type branch from c57f687 to 78ff1b8 Compare September 20, 2023 03:42

schrockn marked this pull request as ready for review September 20, 2023 03:45

alangenfeld approved these changes Sep 20, 2023

View reviewed changes

schrockn force-pushed the source-asset-wrapping-3-executable-observable-asset branch 3 times, most recently from 7ef9fad to a790171 Compare September 22, 2023 02:45

schrockn force-pushed the source-asset-wrapping-4-new-obserable-asset-execution-type branch from 78ff1b8 to 02b6c10 Compare September 22, 2023 02:54

schrockn commented Sep 22, 2023

View reviewed changes

schrockn force-pushed the source-asset-wrapping-4-new-obserable-asset-execution-type branch from 02b6c10 to 9bb377c Compare September 22, 2023 02:56

This was referenced Sep 22, 2023

RFC: @observable_source_asset adapter to generate AssetsDefinition and SensorDefinition #16712

Closed

@observable_source_asset adapter to generate and SensorDefinition #16716

Closed

schrockn force-pushed the source-asset-wrapping-4-new-obserable-asset-execution-type branch from 9bb377c to b03b357 Compare September 22, 2023 14:18

schrockn force-pushed the source-asset-wrapping-3-executable-observable-asset branch from a790171 to b5aeae1 Compare September 22, 2023 14:19

schrockn force-pushed the source-asset-wrapping-4-new-obserable-asset-execution-type branch from b03b357 to ee9c797 Compare September 22, 2023 14:20

schrockn mentioned this pull request Sep 22, 2023

Make observable source adapter layer compatibility with AMP #16718

Closed

schrockn force-pushed the source-asset-wrapping-3-executable-observable-asset branch from b5aeae1 to 73e8bf2 Compare October 6, 2023 12:22

schrockn force-pushed the source-asset-wrapping-4-new-obserable-asset-execution-type branch 2 times, most recently from b7d2e1b to e66d383 Compare October 6, 2023 12:46

schrockn force-pushed the source-asset-wrapping-3-executable-observable-asset branch from 9798d5d to 1ecc61e Compare October 6, 2023 17:10

schrockn mentioned this pull request Oct 6, 2023

Add Playground Example folder and Observable source asset test #17063

Closed

schrockn force-pushed the source-asset-wrapping-4-new-obserable-asset-execution-type branch from e66d383 to d8a6069 Compare October 6, 2023 18:18

Base automatically changed from source-asset-wrapping-3-executable-observable-asset to master October 6, 2023 19:20

schrockn force-pushed the source-asset-wrapping-4-new-obserable-asset-execution-type branch from d8a6069 to 3211da4 Compare October 6, 2023 21:27

Add a new AssetExecutionType (Observable) and use it to omit material…

c429cc0

…izations for observable source asset wrapping more bulletproof codepath defend against non-existent asset_defs better better still f-string

schrockn force-pushed the source-asset-wrapping-4-new-obserable-asset-execution-type branch from 3211da4 to c429cc0 Compare October 9, 2023 07:58

schrockn merged commit 03584cc into master Oct 9, 2023
1 check passed

schrockn deleted the source-asset-wrapping-4-new-obserable-asset-execution-type branch October 9, 2023 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new AssetExecutionType (Observable) and use it to omit materializations for observable source asset wrapping #16621

Add a new AssetExecutionType (Observable) and use it to omit materializations for observable source asset wrapping #16621

schrockn commented Sep 19, 2023 •

edited

Loading

schrockn commented Sep 19, 2023 •

edited

Loading

alangenfeld left a comment

alangenfeld Sep 20, 2023

schrockn Sep 22, 2023 •

edited

Loading

schrockn commented Oct 6, 2023

github-actions bot commented Oct 9, 2023

Add a new AssetExecutionType (Observable) and use it to omit materializations for observable source asset wrapping #16621

Add a new AssetExecutionType (Observable) and use it to omit materializations for observable source asset wrapping #16621

Conversation

schrockn commented Sep 19, 2023 • edited Loading

Summary & Motivation

How I Tested These Changes

schrockn commented Sep 19, 2023 • edited Loading

alangenfeld left a comment

Choose a reason for hiding this comment

alangenfeld Sep 20, 2023

Choose a reason for hiding this comment

schrockn Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

schrockn commented Oct 6, 2023

github-actions bot commented Oct 9, 2023

schrockn commented Sep 19, 2023 •

edited

Loading

schrockn commented Sep 19, 2023 •

edited

Loading

schrockn Sep 22, 2023 •

edited

Loading