Checks on AssetSelection #16610

johannkm · 2023-09-19T00:58:54Z

Implementation of #16185, with some modifications

Use AssetSelection, rather than introducing AssetCheckSelection
externally use Tuple[AssetKey, str] rather than making AssetCheckHandle public
internally use the existing resolve method for assets, and resolve_checks for checks

This means that most of the existing callsites that only care about selected assets (e.g. https://github.com/dagster-io/dagster/blob/johann/09-18-checks_on_AssetSelection/python_modules/dagster/dagster/_core/definitions/asset_graph.py#L118, https://github.com/dagster-io/dagster/blob/johann/09-18-checks_on_AssetSelection/python_modules/dagster/dagster/_core/definitions/data_time.py#L164) can keep doing what they're doing, while the callsites that care can get the checks.

We'll expand the number of callsites to resolve_checks in the future- e.g. for sensors that return run requests with lists of asset keys.

Question:
Should we default to including or excluding checks? E.g. in AssetSelection.all() and AssetSelection.assets(...)

we've gone with including

johannkm · 2023-09-19T00:59:05Z

Current dependencies on/for this PR:

master
- PR Checks on AssetSelection #16610 👈
  - PR Add a checks only job to toys #16638

This comment was auto-generated by Graphite.

schrockn · 2023-09-19T17:51:56Z

Strongly feel that we should includes checks by default. Not including them will be very surprising behavior.

schrockn · 2023-09-19T17:54:56Z

python_modules/dagster/dagster_tests/definitions_tests/test_asset_check_selection.py

+
+
+def test_job_with_asset_and_all_its_checks():
+    job_def = define_asset_job("job1", selection=AssetSelection.assets(asset1) & AssetSelection.asset_checks_for_assets(asset1))


Using set operators is super confusing since this doesn't act like a set '&'. This really is an OR/UNION isn't it. Why don't we just do that instead of being cute.

Meaning that if this really following set semantics this wolud be a null job

schrockn · 2023-09-19T17:57:44Z

python_modules/dagster/dagster_tests/definitions_tests/test_asset_check_selection.py

+    job_def = define_asset_job(
+        "job1", selection=AssetSelection.all_assets() - AssetSelection.all_asset_checks(asset1_check1)
+    )


as above I'm skeptical of using set operations here as it claims a level for formalism that just isn't there. This is a builder API, not a proper algebra.

Strongly recommend AssetSelection.all_assets().without_checks() and let the set operations to the sets.

Meaning if someone really wants to use set operations then can create sets of AssetKeys and go nuts.

johannkm · 2023-09-19T18:54:17Z

@schrockn these test cases were wrong, they're fixed now. Under the hood selection1 & selection2 does a set operation on the asset keys, plus now the asset check handles.

johannkm · 2023-09-19T19:01:21Z

How do you feel about AssetSelection.all() - AssetSelection.all_asset_checks instead of AssetSelection.all_assets().without_checks()?

Agreed these set operations are maybe too fancy, they leave a lot of ways to do things. But they're already public so I'm not sure we can avoid supporting them if we're building in to AssetSelection

updated

schrockn · 2023-09-19T19:36:32Z

Yeah I guess we already do this so we can continue to. However in our successor selection API I do not think we should claim to support a formal set algebra.

github-actions · 2023-09-19T20:58:17Z

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-fh9ivon3e-elementl.vercel.app
https://johann-09-18-checks-on-AssetSelection.components-storybook.dagster-docs.io

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

github-actions · 2023-09-19T20:58:33Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-19tuogqoq-elementl.vercel.app
https://johann-09-18-checks-on-AssetSelection.core-storybook.dagster-docs.io

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

github-actions · 2023-09-19T21:01:13Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-oe8kailyf-elementl.vercel.app
https://johann-09-18-checks-on-AssetSelection.dagster.dagster-docs.io

Direct link to changed pages:

schrockn

Ok I think we move forward here. I've formally registered my concern with relying on set operators, but that decision predates this PR, so we must press on. I look forward to reassessing selection.

schrockn · 2023-09-20T01:44:38Z

python_modules/dagster/dagster_tests/definitions_tests/test_asset_check_selection.py

+    job_def = define_asset_job(
+        "job1", selection=AssetSelection.assets(asset1) - AssetSelection.all_asset_checks()
+    )


Bit concerned about the discoverability of this one. I'm not sure how we are going to expect users to discover this. We may have to add additional helper methods for common use cases like this.

## 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. #16638) - Existing pytest - Stack on dbt implementation --------- Co-authored-by: Johann Miller <[email protected]>

johannkm force-pushed the johann/09-18-checks_on_AssetSelection branch 2 times, most recently from 79e0bc8 to 42ad214 Compare September 19, 2023 02:47

johannkm changed the title ~~checks on AssetSelection~~ [rfc] checks on AssetSelection Sep 19, 2023

johannkm requested review from rexledesma and schrockn September 19, 2023 04:06

schrockn previously requested changes Sep 19, 2023

View reviewed changes

johannkm force-pushed the johann/09-18-checks_on_AssetSelection branch from 42ad214 to db9ad33 Compare September 19, 2023 18:52

johannkm requested a review from schrockn September 19, 2023 19:24

johannkm changed the title ~~[rfc] checks on AssetSelection~~ Checks on AssetSelection Sep 19, 2023

johannkm added 4 commits September 19, 2023 16:46

checks on AssetSelection

0767fa0

default to include

ab78f10

remove .all_assets()

b14b99c

api docs

4cd8a4c

johannkm force-pushed the johann/09-18-checks_on_AssetSelection branch from 1ccc7b2 to f0dc53c Compare September 19, 2023 20:54

johannkm requested a review from erinkcochran87 as a code owner September 19, 2023 20:54

types

b00675a

johannkm force-pushed the johann/09-18-checks_on_AssetSelection branch from f0dc53c to b00675a Compare September 20, 2023 01:36

schrockn approved these changes Sep 20, 2023

View reviewed changes

johannkm mentioned this pull request Sep 20, 2023

Add a checks only job to toys #16638

Merged

add check spec test

603a84d

johannkm force-pushed the johann/09-18-checks_on_AssetSelection branch from 0f22746 to 603a84d Compare September 20, 2023 02:55

johannkm merged commit 190fb99 into master Sep 20, 2023
1 check failed

johannkm deleted the johann/09-18-checks_on_AssetSelection branch September 20, 2023 02:55

rexledesma mentioned this pull request Sep 21, 2023

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

Merged

johannkm mentioned this pull request Sep 25, 2023

[RFC] tests to specify APIs for constructing jobs with check selections #16185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checks on AssetSelection #16610

Checks on AssetSelection #16610

johannkm commented Sep 19, 2023 •

edited

Loading

johannkm commented Sep 19, 2023 •

edited

Loading

schrockn commented Sep 19, 2023

schrockn Sep 19, 2023

schrockn Sep 19, 2023

schrockn Sep 19, 2023

johannkm commented Sep 19, 2023

johannkm commented Sep 19, 2023

schrockn commented Sep 19, 2023

github-actions bot commented Sep 19, 2023

github-actions bot commented Sep 19, 2023

github-actions bot commented Sep 19, 2023 •

edited

Loading

schrockn left a comment

schrockn Sep 20, 2023



		def test_job_with_asset_and_all_its_checks():
		job_def = define_asset_job("job1", selection=AssetSelection.assets(asset1) & AssetSelection.asset_checks_for_assets(asset1))

Checks on AssetSelection #16610

Checks on AssetSelection #16610

Conversation

johannkm commented Sep 19, 2023 • edited Loading

johannkm commented Sep 19, 2023 • edited Loading

schrockn commented Sep 19, 2023

schrockn Sep 19, 2023

Choose a reason for hiding this comment

schrockn Sep 19, 2023

Choose a reason for hiding this comment

schrockn Sep 19, 2023

Choose a reason for hiding this comment

johannkm commented Sep 19, 2023

johannkm commented Sep 19, 2023

schrockn commented Sep 19, 2023

github-actions bot commented Sep 19, 2023

github-actions bot commented Sep 19, 2023

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

schrockn left a comment

Choose a reason for hiding this comment

schrockn Sep 20, 2023

Choose a reason for hiding this comment

johannkm commented Sep 19, 2023 •

edited

Loading

johannkm commented Sep 19, 2023 •

edited

Loading

github-actions bot commented Sep 19, 2023 •

edited

Loading