Skip to content

Commit c887188

Browse files
clnollDagster Devtools
authored andcommitted
fix: resolve asset check keys (#25686)
## Summary & Motivation Update the logic that selects which asset checks are run with DA. The logic is now to resolve to `asset_check_keys` that - target one of the keys in `asset_keys` - are in the same repo that we're targeting - do not have an automation condition defined on it - are explicitly requested I've also included the optimization: if no checks on the `asset_key` set have an automation condition, we pass None. This is all behind a `AUTOMATION_RESOLVE_ASSET_CHECK_KEYS` feature gate. Here's a summary of the behavior now when a DA run is kicked off (gate OFF is the current behavior in production). ``` case gate checks that EXIST checks that RUN pills on the run ----------------------- ---- -------------------------------------- -------------------- -------------------- unconditioned OFF row_count (u), not_null (u) row_count, not_null 2 (row_count, not_null) unconditioned ON row_count (u), not_null (u) row_count, not_null 2 (row_count, not_null) mixed OFF row_count (c), not_null (u) row_count row_count mixed ON row_count (c), not_null (u) row_count, not_null 2 (row_count, not_null) conditioned_not_firing OFF yearly (c), not_null (u) yearly, not_null 2 (yearly, not_null) conditioned_not_firing ON yearly (c), not_null (u) not_null not_null conditioned_only OFF row_count (c) row_count row_count conditioned_only ON row_count (c) row_count row_count (c) = conditioned (has its own automation_condition) (u) = unconditioned (rides along) ``` ## Test Plan Unit & manual (local). Internal-RevId: 421c76c3d19baa6cd46e9cac723c37ad7f8290b0
1 parent 5bd48c9 commit c887188

14 files changed

Lines changed: 589 additions & 6 deletions

python_modules/dagster/dagster/_core/definitions/automation_tick_evaluation_context.py

Lines changed: 79 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
from dagster._core.definitions.asset_selection import AssetSelection
1515
from dagster._core.definitions.assets.graph.asset_graph_subset import AssetGraphSubset
1616
from dagster._core.definitions.assets.graph.base_asset_graph import BaseAssetGraph
17+
from dagster._core.definitions.assets.graph.remote_asset_graph import RemoteWorkspaceAssetGraph
1718
from dagster._core.definitions.backfill_policy import BackfillPolicy, BackfillPolicyType
1819
from dagster._core.definitions.declarative_automation.automation_condition import (
1920
AutomationCondition,
@@ -30,6 +31,7 @@
3031
from dagster._core.definitions.partitions.context import use_partition_loading_context
3132
from dagster._core.definitions.partitions.definition import PartitionsDefinition
3233
from dagster._core.definitions.run_request import RunRequest
34+
from dagster._core.definitions.selector import RepositorySelector
3335
from dagster._core.instance import DynamicPartitionsStore
3436
from dagster._core.storage.tags import (
3537
ASSET_PARTITION_RANGE_END_TAG,
@@ -81,6 +83,8 @@ def __init__(
8183
self._auto_observe_asset_keys = auto_observe_asset_keys or set()
8284
self._partition_loading_context = self._evaluator.asset_graph_view.partition_loading_context
8385
self._logger = logger
86+
# Feature-gated: when off, run-request asset check resolution uses the legacy behavior.
87+
self._resolve_check_keys = instance.automation_resolve_asset_check_keys_enabled()
8488

8589
@property
8690
def cursor(self) -> AssetDaemonCursor:
@@ -137,6 +141,7 @@ def _build_run_requests(self, entity_subsets: Sequence[EntitySubset]) -> Sequenc
137141
asset_graph=self.asset_graph,
138142
run_tags=self._materialize_run_tags,
139143
emit_backfills=self._evaluator.emit_backfills,
144+
resolve_check_keys_enabled=self._resolve_check_keys,
140145
)
141146

142147
def _get_updated_cursor(
@@ -315,6 +320,8 @@ def build_run_requests(
315320
asset_graph: BaseAssetGraph,
316321
run_tags: Mapping[str, str] | None,
317322
emit_backfills: bool,
323+
*,
324+
resolve_check_keys_enabled: bool,
318325
) -> Sequence[RunRequest]:
319326
"""For a single asset in a given tick, the asset will only be part of a run or a backfill, not both.
320327
If the asset is targetd by a backfill, there will only be one backfill that targets the asset.
@@ -330,17 +337,65 @@ def build_run_requests(
330337
_get_mapping_from_entity_subsets(entity_subsets, asset_graph),
331338
asset_graph,
332339
run_tags,
340+
resolve_check_keys_enabled=resolve_check_keys_enabled,
333341
)
334342
if backfill_run_request:
335343
run_requests = [backfill_run_request, *run_requests]
336344

337345
return run_requests
338346

339347

348+
def _any_check_uses_automation_condition(
349+
asset_graph: BaseAssetGraph, asset_keys: AbstractSet[AssetKey]
350+
) -> bool:
351+
return any(
352+
asset_graph.get(check_key).automation_condition is not None
353+
for check_key in asset_graph.get_check_keys_for_assets(asset_keys)
354+
)
355+
356+
357+
def _ride_along_check_keys_for_assets(
358+
asset_graph: BaseAssetGraph, asset_keys: AbstractSet[AssetKey]
359+
) -> AbstractSet[AssetCheckKey]:
360+
"""The checks that ride along by default with a set of assets sharing a single repository.
361+
362+
These are the checks that target the selected assets, restricted to those that:
363+
364+
- do NOT own an automation condition: a check with its own automation condition is scheduled
365+
independently by declarative automation, so it only runs when its own condition fires (i.e.
366+
when it is explicitly requested), not whenever its asset is materialized; and
367+
- are defined in the same repository as the assets: a check can target an asset that lives in
368+
a different code location, but such a check is not part of this repository's implicit asset
369+
job, so it cannot be grouped into the run (otherwise downstream the run fails to build an
370+
execution plan for it).
371+
"""
372+
candidate_check_keys = {
373+
check_key
374+
for check_key in asset_graph.get_check_keys_for_assets(asset_keys)
375+
if asset_graph.get(check_key).automation_condition is None
376+
}
377+
if not asset_keys or not isinstance(asset_graph, RemoteWorkspaceAssetGraph):
378+
# A non-workspace graph only ever spans a single repository, so every candidate check
379+
# is necessarily defined alongside the assets.
380+
return candidate_check_keys
381+
382+
def repo_selector(entity_key: EntityKey) -> RepositorySelector:
383+
return asset_graph.get_repository_handle(entity_key).to_selector()
384+
385+
# All assets in `asset_keys` share a repository (the mapping is split per-repository before
386+
# this is called), so any of them identifies the target repository.
387+
target_repo = repo_selector(next(iter(asset_keys)))
388+
return {
389+
check_key for check_key in candidate_check_keys if repo_selector(check_key) == target_repo
390+
}
391+
392+
340393
def _build_run_requests_from_partitions_def_mapping(
341394
mapping: _PartitionsDefKeyMapping,
342395
asset_graph: BaseAssetGraph,
343396
run_tags: Mapping[str, str] | None,
397+
*,
398+
resolve_check_keys_enabled: bool,
344399
) -> Sequence[RunRequest]:
345400
run_requests = []
346401

@@ -352,19 +407,30 @@ def _build_run_requests_from_partitions_def_mapping(
352407
tags.update({**partitions_def.get_tags_for_partition_key(partition_key)})
353408

354409
for entity_keys_for_repo in asset_graph.split_entity_keys_by_repository(entity_keys):
355-
asset_check_keys = [k for k in entity_keys_for_repo if isinstance(k, AssetCheckKey)]
410+
asset_keys = [k for k in entity_keys_for_repo if isinstance(k, AssetKey)]
411+
requested_check_keys = [k for k in entity_keys_for_repo if isinstance(k, AssetCheckKey)]
412+
if resolve_check_keys_enabled and _any_check_uses_automation_condition(
413+
asset_graph, set(asset_keys)
414+
):
415+
ride_along_check_keys = _ride_along_check_keys_for_assets(
416+
asset_graph, set(asset_keys)
417+
)
418+
resolved_asset_check_keys = list({*requested_check_keys, *ride_along_check_keys})
419+
else:
420+
# Reached when the feature is gated off, or when no check on these assets uses DA:
421+
# pass the explicitly-requested checks, or `None` to let the code server expand to
422+
# all of the assets' checks at execution time.
423+
resolved_asset_check_keys = requested_check_keys or None
356424
run_requests.append(
357425
# Do not call run_request.with_resolved_tags_and_config as the partition key is
358426
# valid and there is no config.
359427
# Calling with_resolved_tags_and_config is costly in asset reconciliation as it
360428
# checks for valid partition keys.
361429
RunRequest(
362-
asset_selection=[k for k in entity_keys_for_repo if isinstance(k, AssetKey)],
430+
asset_selection=asset_keys,
363431
partition_key=partition_key,
364432
tags=tags,
365-
# if selecting no asset_check_keys, just pass in `None` to allow required
366-
# checks to be included
367-
asset_check_keys=asset_check_keys or None,
433+
asset_check_keys=resolved_asset_check_keys,
368434
)
369435
)
370436

@@ -380,6 +446,8 @@ def build_run_requests_with_backfill_policies(
380446
asset_partitions: Iterable[AssetKeyPartitionKey],
381447
asset_graph: BaseAssetGraph,
382448
dynamic_partitions_store: DynamicPartitionsStore,
449+
*,
450+
resolve_check_keys_enabled: bool,
383451
) -> Sequence[RunRequest]:
384452
"""Build run requests for a selection of asset partitions based on the associated BackfillPolicies."""
385453
run_requests = []
@@ -430,7 +498,12 @@ def build_run_requests_with_backfill_policies(
430498
(partitions_def, pk): entity_keys for pk in (partition_keys or [None])
431499
}
432500
run_requests.extend(
433-
_build_run_requests_from_partitions_def_mapping(mapping, asset_graph, run_tags={})
501+
_build_run_requests_from_partitions_def_mapping(
502+
mapping,
503+
asset_graph,
504+
run_tags={},
505+
resolve_check_keys_enabled=resolve_check_keys_enabled,
506+
)
434507
)
435508
else:
436509
run_requests.extend(

python_modules/dagster/dagster/_core/execution/asset_backfill.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1661,6 +1661,7 @@ def _execute_asset_backfill_iteration_inner(
16611661
asset_partitions=asset_partitions_to_request,
16621662
asset_graph=asset_graph,
16631663
dynamic_partitions_store=instance_queryer,
1664+
resolve_check_keys_enabled=instance_queryer.instance.automation_resolve_asset_check_keys_enabled(),
16641665
)
16651666
if run_config is not None:
16661667
run_requests = [rr._replace(run_config=run_config) for rr in run_requests]

python_modules/dagster/dagster/_core/instance/methods/asset_methods.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,10 @@ def internal_asset_freshness_enabled(self) -> bool:
456456
"""Check if internal asset freshness is enabled - moved from AssetMixin.internal_asset_freshness_enabled()."""
457457
return os.getenv("DAGSTER_ASSET_FRESHNESS_ENABLED", "").lower() != "false"
458458

459+
def automation_resolve_asset_check_keys_enabled(self) -> bool:
460+
"""Whether declarative automation explicitly resolves the asset checks recorded on its run requests."""
461+
return os.getenv("DAGSTER_AUTOMATION_RESOLVE_ASSET_CHECK_KEYS", "").lower() == "true"
462+
459463
def streamline_read_asset_health_supported(self, streamline_name: StreamlineName) -> bool:
460464
"""Check if streamline read asset health is supported - moved from AssetMixin.streamline_read_asset_health_supported()."""
461465
return False
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import dagster as dg
2+
3+
4+
@dg.asset
5+
def raw_files() -> None: ...
6+
7+
8+
@dg.asset(automation_condition=dg.AutomationCondition.eager(), deps=[raw_files])
9+
def processed_files() -> None: ...
10+
11+
12+
# Has an automation condition, but a time-gated one that will NOT fire during the test window
13+
# (the cron is far from the frozen test time). DA therefore never individually requests this
14+
# check on the ticks exercised by the test. It is also excluded from the "default" ride-along
15+
# set, because it owns an automation condition.
16+
@dg.asset_check(
17+
asset=processed_files, automation_condition=dg.AutomationCondition.on_cron("0 0 1 1 *")
18+
)
19+
def yearly_check() -> dg.AssetCheckResult:
20+
return dg.AssetCheckResult(passed=True)
21+
22+
23+
# No automation condition -> rides along whenever `processed_files` is materialized.
24+
@dg.asset_check(asset=processed_files)
25+
def non_null() -> dg.AssetCheckResult:
26+
return dg.AssetCheckResult(passed=True)
27+
28+
29+
defs = dg.Definitions(assets=[raw_files, processed_files], asset_checks=[yearly_check, non_null])
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import dagster as dg
2+
3+
any_dep_newly_updated = dg.AutomationCondition.any_deps_match(
4+
dg.AutomationCondition.newly_updated() | dg.AutomationCondition.will_be_requested()
5+
)
6+
7+
8+
@dg.asset
9+
def raw_files() -> None: ...
10+
11+
12+
@dg.asset(automation_condition=dg.AutomationCondition.eager(), deps=[raw_files])
13+
def processed_files() -> None: ...
14+
15+
16+
# A conditioned check on `processed_files` -- fires alongside it, so the run takes the DA
17+
# check-resolution path (rather than the no-DA-checks fallback).
18+
@dg.asset_check(asset=processed_files, automation_condition=any_dep_newly_updated)
19+
def row_count() -> dg.AssetCheckResult:
20+
return dg.AssetCheckResult(passed=True)
21+
22+
23+
# An unconditioned check on `processed_files` -- rides along.
24+
@dg.asset_check(asset=processed_files)
25+
def non_null() -> dg.AssetCheckResult:
26+
return dg.AssetCheckResult(passed=True)
27+
28+
29+
# A DIFFERENT asset with its own unconditioned check. This check targets `other_asset`, not
30+
# `processed_files`, so it must never be attached to a run that targets `processed_files`.
31+
@dg.asset
32+
def other_asset() -> None: ...
33+
34+
35+
@dg.asset_check(asset=other_asset)
36+
def other_check() -> dg.AssetCheckResult:
37+
return dg.AssetCheckResult(passed=True)
38+
39+
40+
defs = dg.Definitions(
41+
assets=[raw_files, processed_files, other_asset],
42+
asset_checks=[row_count, non_null, other_check],
43+
)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
import dagster as dg
2+
3+
any_dep_newly_updated = dg.AutomationCondition.any_deps_match(
4+
dg.AutomationCondition.newly_updated() | dg.AutomationCondition.will_be_requested()
5+
)
6+
7+
8+
@dg.asset
9+
def raw_files() -> None: ...
10+
11+
12+
@dg.asset(automation_condition=dg.AutomationCondition.eager(), deps=[raw_files])
13+
def processed_files() -> None: ...
14+
15+
16+
@dg.asset_check(asset=processed_files, automation_condition=any_dep_newly_updated)
17+
def row_count() -> dg.AssetCheckResult:
18+
return dg.AssetCheckResult(passed=True)
19+
20+
21+
@dg.asset_check(asset=processed_files)
22+
def non_null() -> dg.AssetCheckResult:
23+
return dg.AssetCheckResult(passed=True)
24+
25+
26+
defs = dg.Definitions(assets=[raw_files, processed_files], asset_checks=[row_count, non_null])
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
import dagster as dg
2+
3+
4+
@dg.asset
5+
def raw_files() -> None: ...
6+
7+
8+
@dg.asset(automation_condition=dg.AutomationCondition.eager(), deps=[raw_files])
9+
def processed_files() -> None: ...
10+
11+
12+
# These checks have NO automation condition of their own. They are not individually
13+
# requested by the automation tick -- they ride along with any run that materializes
14+
# `processed_files`. The run request should therefore record all of them explicitly.
15+
@dg.asset_check(asset=processed_files)
16+
def row_count() -> dg.AssetCheckResult:
17+
return dg.AssetCheckResult(passed=True)
18+
19+
20+
@dg.asset_check(asset=processed_files)
21+
def non_null() -> dg.AssetCheckResult:
22+
return dg.AssetCheckResult(passed=True)
23+
24+
25+
defs = dg.Definitions(assets=[raw_files, processed_files], asset_checks=[row_count, non_null])
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import dagster as dg
2+
3+
any_dep_newly_updated = dg.AutomationCondition.any_deps_match(
4+
dg.AutomationCondition.newly_updated() | dg.AutomationCondition.will_be_requested()
5+
)
6+
7+
8+
@dg.asset
9+
def raw_files() -> None: ...
10+
11+
12+
@dg.asset(automation_condition=dg.AutomationCondition.eager(), deps=[raw_files])
13+
def processed_files() -> None: ...
14+
15+
16+
# A SAME-location check that owns an automation condition and fires alongside `processed_files`.
17+
@dg.asset_check(asset=processed_files, automation_condition=any_dep_newly_updated)
18+
def row_count() -> dg.AssetCheckResult:
19+
return dg.AssetCheckResult(passed=True)
20+
21+
22+
defs = dg.Definitions(assets=[raw_files, processed_files], asset_checks=[row_count])
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import dagster as dg
2+
3+
4+
# `processed_files` is defined in a different code location (cross_location_da_check_asset.py).
5+
# This unconditioned check targets it but lives here, so it must be excluded from the ride-along
6+
# set resolved for `processed_files` in the other location (repo-scoping).
7+
@dg.asset_check(asset=dg.AssetKey("processed_files"), name="external_not_null")
8+
def external_not_null() -> dg.AssetCheckResult:
9+
return dg.AssetCheckResult(passed=True)
10+
11+
12+
defs = dg.Definitions(asset_checks=[external_not_null])
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import dagster as dg
2+
3+
4+
# `processed_files` is defined in a different code location
5+
@dg.asset_check(asset=dg.AssetKey("processed_files"))
6+
def external_non_null() -> dg.AssetCheckResult:
7+
return dg.AssetCheckResult(passed=True)
8+
9+
10+
defs = dg.Definitions(asset_checks=[external_non_null])

0 commit comments

Comments
 (0)