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

smackesey · 2024-02-07T23:40:15Z

Summary & Motivation

Lift the restriction that asset jobs cannot contain both observations and materializations. This does not change existing user-constructed jobs using public define_asset_job-- it just lifts the restriction so that it is now possible to construct jobs that contain both observations and materializations. This will facilitate ongoing asset job refactoring. I don't think we should encourage/advertise this functionality yet because the result of an observation still cannot be used to determine whether to skip downstream steps within a run.

How I Tested These Changes

Updated "mixed asset job" test which previously checked for an error, now checks that it successfully executes with correct results.

smackesey · 2024-02-07T23:40:27Z

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

Join @smackesey and the rest of your teammates on Graphite

smackesey · 2024-02-08T12:07:24Z

python_modules/dagster/dagster_tests/definitions_tests/test_external_assets.py

-    ] == {AssetKey("upstream_asset")}
+    assert defs.get_asset_graph().asset_dep_graph["upstream"][AssetKey("downstream_asset")] == {
+        AssetKey("upstream_asset")
+    }


Cleaner to test dependencies in the definitions asset graph than the global asset job, which is an implementation detail.

github-actions · 2024-02-12T16:19:25Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-gowd7vpd4-elementl.vercel.app
https://sean-allow-mixed-asset-jobs.dagster.dagster-docs.io

Direct link to changed pages:

https://dagster-docs-gowd7vpd4-elementl.vercel.app
https://sean-allow-mixed-asset-jobs.dagster.dagster-docs.io/dagster-cloud/managing-deployments/setting-up-alerts

schrockn

Some intermediate comments while I process this more. As I stated in slack, I consider this diff extremely risky as written and I implore you to consider approaches that break this up into smaller, more incremental, lower risk changes. I can grab some time with you later.

schrockn · 2024-02-09T18:43:23Z

python_modules/dagster/dagster/_core/definitions/asset_layer.py

-        observable_source_assets_by_node_handle: Mapping[NodeHandle, "SourceAsset"],
-        source_assets: Sequence["SourceAsset"],
+        assets_to_execute_by_node_handle: Mapping[
+            NodeHandle, Union["AssetsDefinition", "SourceAsset"]


Why is this a Union now? I thought the point was to consolidate internal code paths

Per our discussion, I think it would be useful to add a temporary TypeAlias for these Unions so there is a place in the code base to document what we are doing. You can explain how this is a temporary measure. Looking at this without context it makes very little sense.

The number of instanceofs embedded within this function is terrifying. It is extremely difficult to review with any confidence. We need a more coherent model to understand when it is AssetsDefinition and when it is SourceAsset

schrockn · 2024-02-12T20:40:48Z

python_modules/dagster/dagster/_core/definitions/assets_job.py

        if isinstance(asset, AssetsDefinition):
-            resolved_source_assets += asset.to_source_assets()
+            resolved_other_assets += asset.to_source_assets()


Wow I never saw to_source_assets before. My brain is melting

schrockn · 2024-02-12T20:51:28Z

python_modules/dagster/dagster/_core/definitions/assets_job.py

-    assets: Sequence[AssetsDefinition],
-    source_assets: Optional[Sequence[Union[SourceAsset, AssetsDefinition]]] = None,
+    assets_to_execute: Sequence[Union[AssetsDefinition, SourceAsset]],
+    other_assets: Optional[Sequence[Union[SourceAsset, AssetsDefinition]]] = None,


This comment might have gotten lost so if my be repetive.

other_assets is not a great name. What it means, as far as I understand, is that it is an additional set of assets that is available to source i/o manager information from. I think we should name it something more specific accordingly.

smackesey · 2024-02-12T21:43:42Z

python_modules/dagster/dagster_tests/definitions_tests/test_external_assets.py


    assert isinstance(defs_with_shim.get_assets_def("source_asset"), AssetsDefinition)

-    job_def_with_shim = get_job_for_assets(defs_with_shim, an_asset, shimmed_source_asset)
+    job_def_with_shim = get_job_for_assets(defs_with_shim, an_asset)


We no longer need to provide the external asset when getting the job that uses it purely for loading.

smackesey · 2024-02-12T21:45:45Z

python_modules/dagster/dagster/_core/definitions/observe.py

 def observe(
-    source_assets: Sequence[SourceAsset],
+    assets: Optional[Sequence[Union[AssetsDefinition, SourceAsset]]] = None,


This is now a union because externals can be observed too.

schrockn

Per offline discussion, @smackesey is going to do a series of pre-refactors to build_assets_job and from_graph_and_assets_node_mapping to make incremental changes to those functions more palatable.

schrockn

Very exciting!

…servations (#19667) ## Summary & Motivation Lift the restriction that asset jobs cannot contain both observations and materializations. This does _not_ change existing user-constructed jobs using public `define_asset_job`-- it just lifts the restriction so that it is now _possible_ to construct jobs that contain both observations and materializations. This will facilitate ongoing asset job refactoring. I don't think we should encourage/advertise this functionality yet because the result of an observation still cannot be used to determine whether to skip downstream steps within a run. ## How I Tested These Changes Updated "mixed asset job" test which previously checked for an error, now checks that it successfully executes with correct results.

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from 1098e02 to 9a78a94 Compare February 8, 2024 11:52

smackesey commented Feb 8, 2024

View reviewed changes

smackesey force-pushed the sean/allow-mixed-asset-jobs branch 2 times, most recently from c30c846 to e01be6f Compare February 8, 2024 13:16

smackesey mentioned this pull request Feb 8, 2024

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

Closed

smackesey force-pushed the sean/allow-mixed-asset-jobs branch 2 times, most recently from 62f4e96 to 9d22a35 Compare February 8, 2024 18:59

smackesey mentioned this pull request Feb 8, 2024

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

Merged

smackesey force-pushed the sean/allow-mixed-asset-jobs branch 2 times, most recently from d881aa6 to a1490ca 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 3 times, most recently from be3a82e to 33f844e 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 4 times, most recently from 58c9a32 to 0e3c817 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

schrockn reviewed Feb 12, 2024

View reviewed changes

smackesey commented Feb 12, 2024

View reviewed changes

schrockn requested changes Feb 13, 2024

View reviewed changes

smackesey changed the base branch from master to sean/external-assets-assets-to-execute February 13, 2024 14:30

smackesey force-pushed the sean/external-assets-asset-graph-callsites branch from b7a9ffc to d5de819 Compare March 5, 2024 19:29

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from a178fec to 8a81206 Compare March 5, 2024 19:29

smackesey force-pushed the sean/external-assets-asset-graph-callsites branch from d5de819 to 83293c9 Compare March 5, 2024 22:09

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from 8a81206 to 35bdc6a Compare March 5, 2024 22:09

smackesey force-pushed the sean/external-assets-asset-graph-callsites branch from 83293c9 to 6596798 Compare March 5, 2024 23:22

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from 35bdc6a to ec160de Compare March 5, 2024 23:22

This was referenced Mar 5, 2024

[external-assets] AssetGraph class renames #20276

Merged

[external-assets] Make external_asset_nodes_from_defs take list #20298

Merged

smackesey force-pushed the sean/external-assets-asset-graph-callsites branch 3 times, most recently from 620d0c2 to c1ee471 Compare March 7, 2024 12:39

smackesey mentioned this pull request Mar 7, 2024

[external-rename] host_representation -> remote_representation #20323

Merged

smackesey force-pushed the sean/external-assets-asset-graph-callsites branch 3 times, most recently from c7142ad to 94186ae Compare March 7, 2024 14:50

Base automatically changed from sean/external-assets-asset-graph-callsites to master March 7, 2024 15:26

smackesey mentioned this pull request Mar 7, 2024

[external-assets] Update callsites for partition property asset graph accessors #20330

Merged

smackesey force-pushed the sean/allow-mixed-asset-jobs branch 3 times, most recently from 86ebf75 to 06e3dba Compare March 8, 2024 14:10

smackesey mentioned this pull request Mar 8, 2024

[external-assets] asset checks dup check #20361

Merged

smackesey force-pushed the sean/allow-mixed-asset-jobs branch 2 times, most recently from 4a3c5b2 to 46c50a5 Compare March 11, 2024 13:10

smackesey mentioned this pull request Mar 11, 2024

[external-assets] Add AssetGraph subsetting, make build_asset_job use AssetGraph #20405

Merged

[external-assets] Allow mixed asset jobs

5deee20

smackesey force-pushed the sean/allow-mixed-asset-jobs branch from 46c50a5 to 5deee20 Compare March 11, 2024 14:49

schrockn approved these changes Mar 11, 2024

View reviewed changes

smackesey merged commit 94324c3 into master Mar 11, 2024
2 checks passed

smackesey deleted the sean/allow-mixed-asset-jobs branch March 11, 2024 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

smackesey commented Feb 7, 2024 •

edited

Loading

smackesey commented Feb 7, 2024 •

edited

Loading

smackesey Feb 8, 2024

github-actions bot commented Feb 12, 2024 •

edited

Loading

schrockn left a comment

schrockn Feb 9, 2024

schrockn Feb 12, 2024

schrockn Feb 12, 2024

schrockn Feb 12, 2024

schrockn Feb 12, 2024

smackesey Feb 12, 2024

smackesey Feb 12, 2024

schrockn left a comment

schrockn left a comment

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

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

Conversation

smackesey commented Feb 7, 2024 • edited Loading

Summary & Motivation

How I Tested These Changes

smackesey commented Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 12, 2024 • edited Loading

schrockn 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

Choose a reason for hiding this comment

schrockn left a comment

Choose a reason for hiding this comment

schrockn left a comment

Choose a reason for hiding this comment

smackesey commented Feb 7, 2024 •

edited

Loading

smackesey commented Feb 7, 2024 •

edited

Loading

github-actions bot commented Feb 12, 2024 •

edited

Loading