feat(asset-checks): allow asset check subsetting #16672

rexledesma · 2023-09-20T22:43:05Z

Summary & Motivation

Build on #16610 and #16219 to enable asset check subselection. This change was painful and a doozy.

To wrangle this complexity in the future we need to:

Encompass a succinct and interpretable version of XXXSelection to seamlessly enable the asset selection and asset check selection use case
Standardize our usage of None, [], {}, when indicating that a selection is defaulting to select all the objects (assets and asset checks).
- Rather than using None, an explicit sentinel value should be used (e.g. XXXSelection.ALL, so that the value indicates its usage. Likewise, [] and {} should be replaced with XXXSelection.EMPTY).

How I Tested These Changes

When materializing a subsetted asset, there are four cases:
- Materialize without a selection
- Materialize with a selection of assets and checks
- Materialize with only a selection of assets
- Materialize with only a selection of checks
Remove any usages of AssetSelection that break the new subsetting invariants (e.g. Add a checks only job to toys #16638)
Existing pytest
Stack on dbt implementation

rexledesma · 2023-09-20T22:43:16Z

Current dependencies on/for this PR:

master
- PR feat(asset-checks): allow asset check subsetting #16672 👈
  - PR feat(dbt): model dbt tests as asset checks only on their attached dbt node #16727
    - PR feat(dbt): implement subsetting to execute dbt tests #16801
  - PR subsetting fun #16721

This comment was auto-generated by Graphite.

python_modules/dagster-test/dagster_test/toys/asset_checks.py

...er/dagster_tests/definitions_tests/decorators_tests/test_asset_decorator_with_check_specs.py

python_modules/dagster/dagster/_core/execution/context/compute.py

johannkm · 2023-09-21T02:08:39Z

python_modules/dagster/dagster/_core/definitions/assets.py

@@ -254,18 +257,25 @@ def __init__(
            backfill_policy, "backfill_policy", BackfillPolicy
        )

-        if selected_asset_keys is None:


context to this change for Nick and Sandy: currently for multi assets we just filter asset checks down to the ones that target selected_asset_keys. That's still the default because AssetSelection includes checks for assets by default, but it will be passed in over selected_asset_check_handles now

it will be passed in over selected_asset_check_handles now

What precisely do you mean by this?

ben is going to start sending explicit check selections from the front end, instead of defaulting to None

johannkm

tysm for putting this together. Let's get a look from @schrockn or @sryza then merge

python_modules/dagster/dagster/_core/definitions/assets.py

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

python_modules/dagster/dagster/_core/definitions/assets.py

sryza · 2023-09-22T16:01:57Z

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

@@ -794,21 +807,25 @@ def build_asset_selection_job(
        build_source_asset_observation_job,
    )

+    included_assets = []


It's a little more verbose, but I think it would make this code more bug-proof to explicitly setting each of these in each of the branches below, instead of setting them up here and overwriting them below.

I think the most bulletproof thing here is to make a NamedTuple with these properties.

Then you can also extract out functions that return that object as appopriate

I took Sandy's suggestion, I think it's a reasonable improvement. I think we just need to prioritize removing the special handling for None

python_modules/dagster/dagster/_core/definitions/assets.py

sryza · 2023-09-22T16:05:11Z

python_modules/dagster/dagster/_core/definitions/assets.py

+
+    @public
+    @property
+    def check_handles(self) -> AbstractSet[Tuple[AssetKey, str]]:


I think we should expose AssetCheckHandle publicly rather than tuples. Also, I don't think we need both a check_handles and an asset_check_handles property.

Yeah I pushed back on a type since I didn't want to intoroduce a new concept, "handles", to the public API. I think we should keep that nomenclature internal. However a public property called check_handles does the same thing.

How about we do selected_check_specs and return a list of AssetCheckSpec?

I don't feel strongly about the name at all, but I feel pretty strongly that we should expose a type for this. By the fact that a check is globally identifiable by its (asset key, check name), the concept is inherently present in the ontology. Hiding it will just require a set of contortions.

Do we have any other analogous concepts? Searching for a name here.

It is our first instance of an asset-scoped identifier, although PartitionKey might count.

What about AssetCheckKey? This stays consistent with the naming that a "Key" is a globally unique identifier for a spec in this system.

I like AssetCheckKey.

We use AssetKeyPartitionKey internally, which would correspond to something like AssetKeyCheckName, though a bit of a mouthful.

key + check name are guaranteed unique across checks. We concat them to create Op names which is where you can hit a conflict

I am still confused. Asset keys should be globally unique within a deployment. Can you provide a concrete example of a collision?

asset_key = "a", check_name = "b_c"

asset_key = "a_b", check_name = "c"

We should mangle the name at least a little bit to make that exceeding unlikely.

I'll pick this conversation back up in a thread

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

python_modules/dagster/dagster/_core/execution/context/compute.py

github-actions · 2023-09-22T19:44:05Z

Deploy preview for dagster-university ready!

✅ Preview
https://dagster-university-ijbw93eps-elementl.vercel.app
https://rl-allow-subsetted-asset-checks.dagster-university.dagster-docs.io

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

github-actions · 2023-09-22T19:45:04Z

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-bimn05r5g-elementl.vercel.app
https://rl-allow-subsetted-asset-checks.components-storybook.dagster-docs.io

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

schrockn

Mostly about the "handles" question

schrockn · 2023-09-23T20:08:34Z

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

@@ -794,21 +807,25 @@ def build_asset_selection_job(
        build_source_asset_observation_job,
    )

+    included_assets = []


I think the most bulletproof thing here is to make a NamedTuple with these properties.

schrockn · 2023-09-23T20:09:04Z

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

@@ -794,21 +807,25 @@ def build_asset_selection_job(
        build_source_asset_observation_job,
    )

+    included_assets = []


Then you can also extract out functions that return that object as appopriate

schrockn · 2023-09-23T20:10:31Z

python_modules/dagster/dagster/_core/definitions/assets.py

@@ -254,18 +257,25 @@ def __init__(
            backfill_policy, "backfill_policy", BackfillPolicy
        )

-        if selected_asset_keys is None:


it will be passed in over selected_asset_check_handles now

What precisely do you mean by this?

schrockn · 2023-09-23T20:13:16Z

python_modules/dagster/dagster/_core/definitions/assets.py

+
+    @public
+    @property
+    def check_handles(self) -> AbstractSet[Tuple[AssetKey, str]]:


Yeah I pushed back on a type since I didn't want to intoroduce a new concept, "handles", to the public API. I think we should keep that nomenclature internal. However a public property called check_handles does the same thing.

How about we do selected_check_specs and return a list of AssetCheckSpec?

github-actions · 2023-09-25T13:24:23Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-bb31ie9cm-elementl.vercel.app
https://rl-allow-subsetted-asset-checks.dagster.dagster-docs.io

Direct link to changed pages:

Context #16672 (comment)

updated

johannkm · 2023-09-26T20:18:25Z

Updated to use AssetCheckKeys, and cleaned up some of the branching/added comments

schrockn · 2023-09-27T00:48:57Z

python_modules/dagster/dagster/_core/definitions/assets.py

+    def subset_for(
+        self,
+        selected_asset_keys: AbstractSet[AssetKey],
+        selected_asset_check_keys: Optional[AbstractSet[AssetCheckKey]],
+    ) -> "AssetsDefinition":
+        """Create a subset of this AssetsDefinition that will only materialize the assets and checks
+        in the selected set.


While outside the scope of this PR, and we need to move forward, this function definitely makes me think we need a unified concept of some sort of "keyed entity" (spec?) in the asset graph so that we don't have to thread another parallel set of arguments around.

schrockn · 2023-09-27T01:00:15Z

So we need to move forward here, but it definitely concerns me that we increasing the complexity of AssetsDefinition and also adding another surface area for selection bugs and surface area.

We should do a fast-follow refactor once things have calmed down a bit, especially to consolidate asset selection and asset check selection into a single object to thread around. However time is short, and we have a release cut to make.

rexledesma force-pushed the rl/allow-subsetted-asset-checks branch 6 times, most recently from cf5a9fa to c2e5215 Compare September 21, 2023 01:34

rexledesma requested review from schrockn and johannkm and removed request for schrockn September 21, 2023 01:39

rexledesma marked this pull request as ready for review September 21, 2023 01:39

rexledesma requested a review from schrockn September 21, 2023 01:43

rexledesma self-assigned this Sep 21, 2023

johannkm reviewed Sep 21, 2023

View reviewed changes

python_modules/dagster-test/dagster_test/toys/asset_checks.py Show resolved Hide resolved

johannkm reviewed Sep 21, 2023

View reviewed changes

johannkm requested a review from sryza September 21, 2023 02:06

johannkm reviewed Sep 21, 2023

View reviewed changes

rexledesma force-pushed the rl/allow-subsetted-asset-checks branch from c2e5215 to 1d634c7 Compare September 21, 2023 20:19

sryza reviewed Sep 22, 2023

View reviewed changes

johannkm mentioned this pull request Sep 22, 2023

subsetting fun #16721

Closed

rexledesma force-pushed the rl/allow-subsetted-asset-checks branch from 1d634c7 to db87ca0 Compare September 22, 2023 19:41

rexledesma mentioned this pull request Sep 22, 2023

feat(dbt): model dbt tests as asset checks only on their attached dbt node #16727

Merged

schrockn previously requested changes Sep 23, 2023

View reviewed changes

rexledesma force-pushed the rl/allow-subsetted-asset-checks branch from db87ca0 to 13dfb5f Compare September 25, 2023 13:20

rexledesma force-pushed the rl/allow-subsetted-asset-checks branch from 13dfb5f to ea29644 Compare September 25, 2023 13:28

johannkm force-pushed the rl/allow-subsetted-asset-checks branch from ea29644 to 0d3830a Compare September 26, 2023 03:55

johannkm mentioned this pull request Sep 26, 2023

rename AssetCheckHandle -> AssetCheckKey #16790

Merged

johannkm added a commit that referenced this pull request Sep 26, 2023

rename AssetCheckHandle -> AssetCheckKey (#16790)

1994712

Context #16672 (comment)

johannkm force-pushed the rl/allow-subsetted-asset-checks branch 2 times, most recently from f4d0c7e to 12b4e44 Compare September 26, 2023 17:19

johannkm assigned johannkm and unassigned rexledesma Sep 26, 2023

rexledesma mentioned this pull request Sep 26, 2023

feat(dbt): implement subsetting to execute dbt tests #16801

Merged

rexledesma and others added 2 commits September 26, 2023 14:23

feat(asset-checks): allow asset check subsetting

da40865

rename

cbaf515

johannkm force-pushed the rl/allow-subsetted-asset-checks branch 8 times, most recently from dc2c0f5 to 6770d5d Compare September 26, 2023 20:16

johannkm requested review from schrockn and sryza September 26, 2023 20:16

rexledesma force-pushed the rl/allow-subsetted-asset-checks branch from 6770d5d to d405c22 Compare September 26, 2023 22:08

schrockn reviewed Sep 27, 2023

View reviewed changes

schrockn self-requested a review September 27, 2023 00:57

schrockn approved these changes Sep 27, 2023

View reviewed changes

surface check keys

dda4b01

johannkm force-pushed the rl/allow-subsetted-asset-checks branch from d405c22 to dda4b01 Compare September 27, 2023 01:42

johannkm merged commit e69cd3f into master Sep 27, 2023

johannkm deleted the rl/allow-subsetted-asset-checks branch September 27, 2023 01:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(asset-checks): allow asset check subsetting #16672

feat(asset-checks): allow asset check subsetting #16672

rexledesma commented Sep 20, 2023 •

edited

Loading

rexledesma commented Sep 20, 2023 •

edited

Loading

johannkm Sep 21, 2023

schrockn Sep 23, 2023

johannkm Sep 26, 2023

johannkm left a comment

sryza Sep 22, 2023

schrockn Sep 23, 2023

schrockn Sep 23, 2023

johannkm Sep 26, 2023

sryza Sep 22, 2023

schrockn Sep 23, 2023 •

edited

Loading

sryza Sep 25, 2023

schrockn Sep 25, 2023

sryza Sep 25, 2023

johannkm Sep 26, 2023

schrockn Sep 26, 2023

sryza Sep 26, 2023

schrockn Sep 26, 2023 •

edited

Loading

johannkm Sep 26, 2023

github-actions bot commented Sep 22, 2023 •

edited

Loading

github-actions bot commented Sep 22, 2023 •

edited

Loading

schrockn left a comment

schrockn Sep 23, 2023

schrockn Sep 23, 2023

schrockn Sep 23, 2023

schrockn Sep 23, 2023 •

edited

Loading

github-actions bot commented Sep 25, 2023 •

edited

Loading

johannkm commented Sep 26, 2023

schrockn Sep 27, 2023

schrockn commented Sep 27, 2023

feat(asset-checks): allow asset check subsetting #16672

feat(asset-checks): allow asset check subsetting #16672

Conversation

rexledesma commented Sep 20, 2023 • edited Loading

Summary & Motivation

How I Tested These Changes

rexledesma commented Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannkm 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

schrockn Sep 23, 2023 • edited Loading

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 Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 22, 2023 • edited Loading

github-actions bot commented Sep 22, 2023 • 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

schrockn Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 25, 2023 • edited Loading

johannkm commented Sep 26, 2023

Choose a reason for hiding this comment

schrockn commented Sep 27, 2023

rexledesma commented Sep 20, 2023 •

edited

Loading

rexledesma commented Sep 20, 2023 •

edited

Loading

schrockn Sep 23, 2023 •

edited

Loading

schrockn Sep 26, 2023 •

edited

Loading

github-actions bot commented Sep 22, 2023 •

edited

Loading

github-actions bot commented Sep 22, 2023 •

edited

Loading

schrockn Sep 23, 2023 •

edited

Loading

github-actions bot commented Sep 25, 2023 •

edited

Loading