[DS][29/n] Create ParentNewerCondition

[OwenKephart]

Summary & Motivation

Originally, this was conceived as a condition that would be used as part of an any_deps_match() sort of context. There are a few problems with this:

1. All other conditions are simply true or false for a given asset partition, regardless of which child_partition we consider to be the "downstream". e.g. an asset partition is unambiguously missing or not missing. However, if you have A -> B with A unpartitioned and B partitioned, then it's possible for A to be newer than some partitions of B but older than others. This means that if we wanted to evaluate something like: any_deps_match(missing() | newer_than_child()), then we would have to evaluate the inner expression of missing | newer_than_child on a per-child-partition basis, which is both expensive and extremely challenging to visualize
2. We want to eventually implement this via the StaleStatusResolver, which fits way better with the mental model of "a child partition can have the status (parent is newer)" rather than "a child partition can have the status (any parent has the status (newer than child partition))"

The downside is that this does not compose as nicely with other conditions, but I think this is actually ok. For example, we don't support "all parents newer" out of the box, but I think this is a good thing, as that sort of behavior is actually a bit of a footgun, which leads to weird synchronization issues if you (e.g.) materialize one of the two parents, then manually materialize the child. Now you're on a weird new cadence in which your child will not materialize after the second parent updates.

For most cases in which you want to wait for more than one parent to update, you're either filling in time partitions as they come in (and so you can just wait for no parent partitions to be missing, rather than all parent partitions to update), or you want to materialize an unpartitioned asset regularly (in which you can wait for all parent partitions to be updated since a given cron tick, rather than waiting for them all to be updated more recently than you have updated).

How I Tested These Changes

[OwenKephart]

• [DS][29/n] Create ParentNewerCondition #21641 : 2 dependent PRs (#21670 , #21671 ) 👈
• [DS][28/n] Create WillBeRequestedCondition #21640
• [DS][27/n] Make candidate_slice and true_slice Optional #21648
• [DS][26/n] Update internal representation of InLatestTimeWindow #21615
• [DS][25/n] Narrow extra_state type #21613
• [DS][24/n] Remove all AssetSubset references from the context and result objects #21612
• [DS][23/n] Update SchedulingContext object #21573
• [DS][22/n] Convert SchedulingResult to DagsterModel #21546
• [DS][21/n] Rename AssetConditionResult to SchedulingResult #21545
• [DS][20/n] Port static constructors from AssetCondition to SchedulingCondition #21541
• [DS][19/n] Create SchedulingCondition object to replace AssetCondition #21540
• [DS][18/n] Move serialized objects into separate file #21539
• [DS][17/n] Move legacy files to separate legacy folder #21538
• [DS][16/n] Rename AssetConditionEvaluationContext to LegacyRuleEvaluationContext #21537
• [DS][15/n] Move operators to separate folder #21536
• [DS][14/n] Remove unused methods and properties #21535
• [DS][13/n] Convert SchedulingContext to DagsterModel #21521
• [DS][12/n] Rename SchedulingContext object #21520
• [DS][11/n] Create InProgressCondition #21511 : 1 other dependent PR (#21512 )
• [DS][10/n] Create UpdatedSinceCronCondition #21510
• [DS][9/n] Add lookback_timedelta to InLatestTimeWindow condition #21508
• [DS][8/n] Create InLatestTimeWindow condition #21507
• [DS][7/n] Create MaterializedSchedulingCondition #21505
• [DS][6/n] Create Any/AllDepsMatchSchedulingConditions #21504
• [DS][5/n] Use AssetSlice in AssetConditionResult #21503
• [DS][4/n] Rename asset_condition folder and add README #21502
• [DS][3/n] Update unique_id creation logic #21501
• [DS][2/n] Update signature of AutoMaterializeRule #21500
• [DS][1/n] Create SchedulingConditionEvaluationContext #21499
• master

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

Join @OwenKephart and the rest of your teammates on Graphite

[schrockn]

I think we should consider a scarier name for this to indicate that you should really understand the constraints of the callsite before calling it, as this will materialize the entire partition range in memory.

[OwenKephart]

Is that functionally all that different from the potential impact of other types of compute_*() functions? Many of those involve actual round trips to the database, which is comparatively expensive to iterating through a ton of time window partitions.

[schrockn]

I basically think operation should be illegal except in very specific contexts, and it should be named accordingly.

[OwenKephart]

hm I disagree -- I think this is a fairly standard operation that is a sub routine of plenty of other similarly-named operations (e.g. intersecting two AssetSlices requires computing all the keys in each slice), and so if we were to make this illegal, we'd need to make intersections and unions etc. illegal as well

[schrockn]

I feel pretty strongly this. We can change this later, but for now I would prefer the name

expensively_materialize_partition_key_range_in_memory or something ridiculous like that is very easily greppable. This is not a public API so we can codemod it later.

But that will make people think about this in the short-term and make sure that we are in a context where there aren't huge partition ranges.

@gibsondan keeps on running into this in py-spy runs so I consider this "on fire" at the moment.

[OwenKephart]

Ok I updated the name to compute_asset_partitions_EXPENSIVE, which I think is a sufficient deterrent for now, and we can move this discussion to a future diff

[schrockn]

what is the relationship between newer parents and our new language around "unsynced"?

[OwenKephart]

Right now, the main differences are:

1. this does not consider data versions for materialized assets, meaning a parent update which does not modify the data version will still result in the asset being marked as having a parent updated (it does consider data versions for asset observations, however)

Note that currently, code-versioned assets with no parents will only get new data versions when their code versions are updated. This means that for dbt assets (which are assigned code versions by default), parents would almost never be considered updated if we didn't have this weird definition.

2. this definition handles a slight edge case bug with observable source asset versioning, wherein if you make an observation with a data version, then another observation without any data version, then that counts as a "new data version".

[schrockn]

Super supportive of the direction and thanks for the clear PR summary. It looks very good overall.

I think this is worth a brief in-person convo to talk through the subtleties, if nothing else so that I fully understand the semantics here. Getting this as right as possible upfront is important as this is such a critical rule.

[schrockn]

All caps too aggressive aesthetically imo

…

On Tue, May 14 2024 at 9:43 AM, OwenKephart < ***@***.*** > wrote: ***@***.**** commented on this pull request. In python_modules/dagster/dagster/_core/asset_graph_view/asset_graph_view.py ( #21641 (comment) ) : > + def compute_asset_partitions(self) -> AbstractSet[AssetKeyPartitionKey]: + return self._compatible_subset.asset_partitions + Ok I updated the name to compute_asset_partitions_EXPENSIVE , which I think is a sufficient deterrent for now, and we can move this discussion to a future diff — Reply to this email directly, view it on GitHub ( #21641 (comment) ) , or unsubscribe ( https://github.com/notifications/unsubscribe-auth/AG3IK6KLIK7SC35FNPNQAY3ZCIIH7AVCNFSM6AAAAABHF7TBCOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJVGQZDIMJXGY ). You are receiving this because your review was requested. Message ID: <dagster-io/dagster/pull/21641/review/2055424176 @ github. com>

[OwenKephart]

Merge activity

• May 14, 10:38 AM EDT: @OwenKephart started a stack merge that includes this pull request via Graphite.
• May 14, 10:47 AM EDT: Graphite rebased this pull request as part of a merge.
• May 14, 10:49 AM EDT: Graphite rebased this pull request as part of a merge.
• May 14, 10:50 AM EDT: @OwenKephart merged this pull request with Graphite.