[external-assets] Make execution_type an ExternalAssetNode property #19684

smackesey · 2024-02-08T18:59:16Z

Summary & Motivation

This adds execution_type as a property on ExternalAssetsNode and defines execution-type related helper properties on AssetsDefinition.

Logic is in place on ExternalAssetNode to ensure that execution_type specified as a param agrees with metadata.

How I Tested These Changes

Existing test suite.

smackesey · 2024-02-08T18:59:28Z

[external-assets] Remove SourceAsset from asset jobs #19702 : 2 dependent PRs (#18217 , #19741 )
[external-assets] Add support for all SourceAsset functionality on externals #19685
[external-assets] Make execution_type an ExternalAssetNode property #19684 👈
Pin duckdb <0.10.0 #19774
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @smackesey and the rest of your teammates on Graphite

schrockn

Can we please do this without changing the public API for asset and multi_asset and friends? The _ prefixed arguments will pollute typeaheads, we'll need documentation, it will add confusion etc.

smackesey · 2024-02-14T15:14:09Z

python_modules/dagster/dagster/_core/host_representation/external_data.py

+                )
+                or AssetExecutionType.MATERIALIZATION
+            )
+


Even though we are sticking with metadata for now to specify AssetExecutionType on AssetsDefinition, We still need this check because when an ExternalAssetNode represents a source asset there is no such metadata.

smackesey · 2024-02-14T15:19:34Z

I tried to do this by directly constructing AssetsDefinition but found it would require too much duplicative logic-- there is way more logic in the asset decorator code than I would expect. Makes it so that it's quite impractical to build AssetsDefinition directly with any confidence, as one would intuitively expect to do in a factory. Maybe something we should address in the future.

For the time being I reverted to using metadata per your suggestion in the upstack PR. This PR does still add the property on ExternalAssetNode and various helpers on AssetsDefinition.

schrockn

Great

schrockn · 2024-02-14T18:15:32Z

python_modules/dagster/dagster/_core/host_representation/external_data.py

+            val = metadata[SYSTEM_METADATA_KEY_ASSET_EXECUTION_TYPE]
+            if not isinstance(val, TextMetadataValue):
+                check.failed(
+                    f"Expected metadata value for key {SYSTEM_METADATA_KEY_ASSET_EXECUTION_TYPE} to be a TextMetadataValue, got {val}"
+                )
+            metadata_execution_type = AssetExecutionType.str_to_enum(val.value)
+            if execution_type is not None:
+                check.invariant(
+                    execution_type == metadata_execution_type,
+                    f"Execution type {execution_type} in metadata does not match type inferred from metadata {metadata_execution_type}",
+                )
+            execution_type = metadata_execution_type


consider extracting out body of if into the helper function to compartmentalize the backward compat code

the reason I didn't are that there are subsequent commented backcompat blocks in the constructor, so it was more consistent to go inline

cool makes sense.

…19684) ## Summary & Motivation This adds `execution_type` as a property on `ExternalAssetsNode` and defines execution-type related helper properties on `AssetsDefinition`. Logic is in place on `ExternalAssetNode` to ensure that `execution_type` specified as a param agrees with metadata. ## How I Tested These Changes Existing test suite.

smackesey mentioned this pull request Feb 8, 2024

[external assets] source assets -> external assets #18217

Closed

smackesey mentioned this pull request Feb 8, 2024

[external-assets] Allow asset jobs to combine materializations and observations #19667

Merged

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from 9d22a35 to d881aa6 Compare February 8, 2024 19:35

smackesey force-pushed the sean/make-asset-execution-type-property branch from a5a00c6 to 1a4ff9b Compare February 8, 2024 19:35

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from d881aa6 to a1490ca Compare February 8, 2024 20:12

smackesey force-pushed the sean/make-asset-execution-type-property branch from 1a4ff9b to cfd2894 Compare February 8, 2024 20:12

smackesey mentioned this pull request Feb 8, 2024

[external-assets] Add support for all SourceAsset functionality on externals #19685

Merged

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from a1490ca to 36d76b2 Compare February 8, 2024 22:55

smackesey force-pushed the sean/make-asset-execution-type-property branch from cfd2894 to acd306f Compare February 8, 2024 22:55

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from 36d76b2 to be3a82e Compare February 9, 2024 10:20

smackesey force-pushed the sean/make-asset-execution-type-property branch from acd306f to 69d7b3e Compare February 9, 2024 10:20

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from be3a82e to 33f844e Compare February 9, 2024 14:13

smackesey force-pushed the sean/make-asset-execution-type-property branch from 69d7b3e to b507aba Compare February 9, 2024 14:13

smackesey mentioned this pull request Feb 9, 2024

[external-assets] Remove SourceAsset from asset jobs #19702

Closed

smackesey marked this pull request as ready for review February 9, 2024 17:34

smackesey requested a review from schrockn February 9, 2024 17:34

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from 33f844e to 3268b72 Compare February 10, 2024 17:43

smackesey force-pushed the sean/make-asset-execution-type-property branch from b507aba to da4cd8d Compare February 10, 2024 17:43

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from 3268b72 to b6eb4c3 Compare February 10, 2024 17:45

smackesey force-pushed the sean/make-asset-execution-type-property branch from da4cd8d to a956af4 Compare February 10, 2024 17:45

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from b6eb4c3 to 58c9a32 Compare February 11, 2024 14:15

smackesey force-pushed the sean/make-asset-execution-type-property branch from a956af4 to 5cceb48 Compare February 11, 2024 14:15

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from 58c9a32 to 0e3c817 Compare February 12, 2024 16:15

smackesey force-pushed the sean/make-asset-execution-type-property branch from 5cceb48 to 71ef97e Compare February 12, 2024 16:15

This was referenced Feb 12, 2024

[external-assets] External asset renames #19741

Closed

[external-assets] remove old SourceAsset code #19742

Closed

smackesey force-pushed the sean/make-asset-execution-type-property branch from 71ef97e to 046bc7f Compare February 13, 2024 14:30

smackesey changed the base branch from sean/allow-mixed-asset-jobs to master February 13, 2024 14:30

smackesey force-pushed the sean/make-asset-execution-type-property branch from 046bc7f to 46799c0 Compare February 13, 2024 15:45

smackesey force-pushed the sean/make-asset-execution-type-property branch 3 times, most recently from 461f3a7 to 74a61c6 Compare February 13, 2024 16:49

smackesey changed the base branch from master to sean/duckdb-pin February 13, 2024 17:09

smackesey force-pushed the sean/make-asset-execution-type-property branch from 74a61c6 to 078c860 Compare February 13, 2024 17:09

This was referenced Feb 13, 2024

[external-assets] Change build_assets_job interface to use executable_assets and loadable_assets #19765

Merged

Pin duckdb <0.10.0 #19774

Merged

Base automatically changed from sean/duckdb-pin to master February 13, 2024 17:34

smackesey force-pushed the sean/make-asset-execution-type-property branch from 078c860 to f035484 Compare February 13, 2024 17:41

schrockn requested changes Feb 13, 2024

View reviewed changes

smackesey force-pushed the sean/make-asset-execution-type-property branch 2 times, most recently from bee736f to 0ed5804 Compare February 14, 2024 15:07

[external-assets] Make execution type an AssetsDefinition property

05f5a4c

smackesey force-pushed the sean/make-asset-execution-type-property branch from 0ed5804 to 05f5a4c Compare February 14, 2024 15:11

smackesey commented Feb 14, 2024

View reviewed changes

smackesey requested a review from schrockn February 14, 2024 16:07

smackesey changed the title ~~[external-assets] Make execution_type an AssetsDefinition property~~ [external-assets] Make execution_type an ExternalAssetNode Feb 14, 2024

schrockn approved these changes Feb 14, 2024

View reviewed changes

smackesey changed the title ~~[external-assets] Make execution_type an ExternalAssetNode~~ [external-assets] Make execution_type an ExternalAssetNode property Feb 14, 2024

smackesey merged commit 6788bb8 into master Feb 14, 2024
1 check passed

smackesey deleted the sean/make-asset-execution-type-property branch February 14, 2024 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[external-assets] Make execution_type an ExternalAssetNode property #19684

[external-assets] Make execution_type an ExternalAssetNode property #19684

smackesey commented Feb 8, 2024 •

edited

Loading

smackesey commented Feb 8, 2024 •

edited

Loading

schrockn left a comment

smackesey Feb 14, 2024

smackesey commented Feb 14, 2024

schrockn left a comment

schrockn Feb 14, 2024

smackesey Feb 14, 2024

schrockn Feb 14, 2024

[external-assets] Make execution_type an ExternalAssetNode property #19684

[external-assets] Make execution_type an ExternalAssetNode property #19684

Conversation

smackesey commented Feb 8, 2024 • edited Loading

Summary & Motivation

How I Tested These Changes

smackesey commented Feb 8, 2024 • edited Loading

schrockn left a comment

Choose a reason for hiding this comment

smackesey Feb 14, 2024

Choose a reason for hiding this comment

smackesey commented Feb 14, 2024

schrockn left a comment

Choose a reason for hiding this comment

schrockn Feb 14, 2024

Choose a reason for hiding this comment

smackesey Feb 14, 2024

Choose a reason for hiding this comment

schrockn Feb 14, 2024

Choose a reason for hiding this comment

smackesey commented Feb 8, 2024 •

edited

Loading

smackesey commented Feb 8, 2024 •

edited

Loading