Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DS][31/n] Create static constructor for eager condition #21737

Merged

Conversation

OwenKephart
Copy link
Contributor

Summary & Motivation

This creates a static constructor for an analog to the current-day eager condition. It is not identical, as it does not consider historical time partitions. It also manages state differently.

In the current world, the "parent_newer" analog actually encodes a lot of different information. From its perspective, an asset partition is True if:

A parent has been materialized more recently than the asset, and the run that materialized it does not target this asset,
AND this asset hasn't been requested for materialization by AMP since that run came in

This is a pretty confusing thing to wrap your head around, but it does have two good qualities:

it avoids repeatedly requesting the downstream asset for materialization while the asset computes (techincally, the parent will remain newer than the child until the child successfully materializes, which may in practice span a large number of scheduling evaluations)
it avoids repeatedly materializing the asset in the case that the requested materialization failed (similar to the above issue)
This policy handles issue (1) by explicitly avoiding materializing this asset if a run is currently in progress, but does not handle case (2).

This will need to be handled with a recently_requested condition, which can keep track of the asset partitions which have been requested within some timedelta.

How I Tested These Changes

Copy link
Contributor Author

OwenKephart commented May 8, 2024

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

Join @OwenKephart and the rest of your teammates on Graphite Graphite

@OwenKephart OwenKephart changed the title Create static constructor for eager condition [DS][31/n] Create static constructor for eager condition May 8, 2024
@OwenKephart OwenKephart requested a review from schrockn May 8, 2024 23:13
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super exciting to see this. Just want to make sure I understand 100% our default policy here and have it in the docblock

@OwenKephart OwenKephart force-pushed the 05-06-Static_constructor_for_eager_condition branch from b835cb7 to 8158240 Compare May 9, 2024 22:48
@OwenKephart OwenKephart force-pushed the 05-08-Create_static_constructor_for_eager_condition branch from fa586bf to 7216287 Compare May 9, 2024 22:48
@OwenKephart OwenKephart force-pushed the 05-06-Static_constructor_for_eager_condition branch from 8158240 to 3c56fea Compare May 10, 2024 22:44
@OwenKephart OwenKephart force-pushed the 05-08-Create_static_constructor_for_eager_condition branch from 7216287 to 43d01d9 Compare May 10, 2024 22:44
@OwenKephart OwenKephart requested a review from schrockn May 10, 2024 22:46
@OwenKephart OwenKephart force-pushed the 05-06-Static_constructor_for_eager_condition branch from 3c56fea to be7aa64 Compare May 13, 2024 17:30
@OwenKephart OwenKephart force-pushed the 05-08-Create_static_constructor_for_eager_condition branch from 43d01d9 to 0d2a7ad Compare May 13, 2024 17:30
@OwenKephart OwenKephart force-pushed the 05-06-Static_constructor_for_eager_condition branch from be7aa64 to b721c23 Compare May 13, 2024 17:51
@OwenKephart OwenKephart force-pushed the 05-08-Create_static_constructor_for_eager_condition branch from 0d2a7ad to 12677c0 Compare May 13, 2024 17:51
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add test that ensures this actually constructs.

All I am looking for is

assert isinstance(SchedulingCondition.eager(), SchedulingCondition)

@OwenKephart OwenKephart force-pushed the 05-06-Static_constructor_for_eager_condition branch from b721c23 to f64a604 Compare May 14, 2024 13:47
@OwenKephart OwenKephart force-pushed the 05-08-Create_static_constructor_for_eager_condition branch from 12677c0 to f4fe85b Compare May 14, 2024 13:47
@OwenKephart OwenKephart force-pushed the 05-06-Static_constructor_for_eager_condition branch from f64a604 to 5033665 Compare May 14, 2024 13:55
@OwenKephart OwenKephart force-pushed the 05-08-Create_static_constructor_for_eager_condition branch from f4fe85b to f854324 Compare May 14, 2024 13:55
@OwenKephart OwenKephart force-pushed the 05-06-Static_constructor_for_eager_condition branch from 5033665 to d09af0c Compare May 14, 2024 13:59
@OwenKephart OwenKephart force-pushed the 05-08-Create_static_constructor_for_eager_condition branch from f854324 to 26e5013 Compare May 14, 2024 13:59
Copy link
Contributor Author

a test gets added upstack, which also tests the behavior of the composite condition

Copy link
Contributor Author

OwenKephart commented May 14, 2024

Merge activity

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

@OwenKephart OwenKephart force-pushed the 05-06-Static_constructor_for_eager_condition branch from d09af0c to 80939db Compare May 14, 2024 14:51
Base automatically changed from 05-06-Static_constructor_for_eager_condition to master May 14, 2024 14:52
@OwenKephart OwenKephart force-pushed the 05-08-Create_static_constructor_for_eager_condition branch from 26e5013 to 3353519 Compare May 14, 2024 14:52
@OwenKephart OwenKephart merged commit 6526093 into master May 14, 2024
1 check was pending
@OwenKephart OwenKephart deleted the 05-08-Create_static_constructor_for_eager_condition branch May 14, 2024 14:54
danielgafni pushed a commit to danielgafni/dagster that referenced this pull request Jun 18, 2024
…21737)

## Summary & Motivation

This creates a static constructor for an analog to the current-day eager condition. It is not identical, as it does not consider historical time partitions. It also manages state differently.

In the current world, the "parent_newer" analog actually encodes a lot of different information. From its perspective, an asset partition is True if:

A parent has been materialized more recently than the asset, and the run that materialized it does not target this asset,
AND this asset hasn't been requested for materialization by AMP since that run came in

This is a pretty confusing thing to wrap your head around, but it does have two good qualities:

it avoids repeatedly requesting the downstream asset for materialization while the asset computes (techincally, the parent will remain newer than the child until the child successfully materializes, which may in practice span a large number of scheduling evaluations)
it avoids repeatedly materializing the asset in the case that the requested materialization failed (similar to the above issue)
This policy handles issue (1) by explicitly avoiding materializing this asset if a run is currently in progress, but does not handle case (2).

This will need to be handled with a recently_requested condition, which can keep track of the asset partitions which have been requested within some timedelta.



## How I Tested These Changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants