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][49/n] Update static constructors to use Since operator #21975

Open
wants to merge 2 commits into
base: 05-23-Allow_AllPartitionsSubsets_in_TimeWindowPartitionMapping
Choose a base branch
from

Conversation

OwenKephart
Copy link
Contributor

@OwenKephart OwenKephart commented May 20, 2024

Summary & Motivation

As title -- now we can take advantage of the "since" operator in a variety of different ways to simplify and improve our on_cron and eager "composite conditions".

On the on_cron side, note that the previous definition had a bad property, which is that if the requested materialization failed, a new one would be immediately re-requested (as nothing would have changed in the eyes of the system -- the parents would still be newer than the cron tick, the child would still be older than the cron tick, and there'd no long be an in-progress run). eager had handling for this case (temporarily removed downstack), but it was ugly (basically just a fixed retry interval, which is conceptually confusing).

This shows off the value of the new since operator -- you can just make sure that you've requested a materialization of an asset since the newest time a condition became true, which means that you can guarantee that the cron condition will only become true once per cron tick, because the CronTickPassed condition only becomes true once per cron tick. Similarly, the any_dep_matches(newly_updated) condition will only be true at the instant a parent is updated, so you can guarantee a maximum of one downstream run per parent update, regardless of if that downstream run fails etc.

This basically reimplements the current AMP eager behavior, but in a much more principled way, as we assign actual operators and names to these bookkeeping operations, rather than relying on some rather arcane codepaths to magically do a similar thing.

How I Tested These Changes

Copy link
Contributor Author

OwenKephart commented May 20, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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 force-pushed the 05-20-Create_CronTickPassedCondition branch from 6432e5e to 3d2806a Compare May 20, 2024 21:18
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch from d4b4d48 to 6cf445c Compare May 20, 2024 21:18
@OwenKephart OwenKephart force-pushed the 05-20-Create_CronTickPassedCondition branch from 3d2806a to 3b9c102 Compare May 20, 2024 21:52
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch from 6cf445c to 09fbc35 Compare May 20, 2024 21:52
@OwenKephart OwenKephart changed the title [ds][x/n] Replace UpdatedSinceCronCondition [DS][48/n] Replace UpdatedSinceCronCondition May 20, 2024
@OwenKephart OwenKephart requested a review from schrockn May 20, 2024 22:23
@OwenKephart OwenKephart marked this pull request as ready for review May 20, 2024 22:24
@OwenKephart OwenKephart force-pushed the 05-20-Create_CronTickPassedCondition branch from 3b9c102 to 8ddcdff Compare May 20, 2024 23:22
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch from 09fbc35 to c4d4898 Compare May 20, 2024 23:22
@OwenKephart OwenKephart force-pushed the 05-20-Create_CronTickPassedCondition branch from 8ddcdff to 4de29a2 Compare May 21, 2024 13:06
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch from c4d4898 to 0e4b4b8 Compare May 21, 2024 13:07
@OwenKephart OwenKephart force-pushed the 05-20-Create_CronTickPassedCondition branch from 4de29a2 to c5335df Compare May 22, 2024 22:51
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch from 0e4b4b8 to 384a80c Compare May 22, 2024 22:51
@OwenKephart OwenKephart force-pushed the 05-20-Create_CronTickPassedCondition branch from c5335df to d8978d0 Compare May 22, 2024 23:07
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch 2 times, most recently from ff920de to 1da4642 Compare May 22, 2024 23:24
@OwenKephart OwenKephart changed the title [DS][48/n] Replace UpdatedSinceCronCondition [DS][48/n] Update static constructors to use Since operator May 22, 2024
@OwenKephart OwenKephart force-pushed the 05-20-Create_CronTickPassedCondition branch from d8978d0 to 8f54ed6 Compare May 22, 2024 23:54
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch 2 times, most recently from 303b5c7 to d08e907 Compare May 22, 2024 23:56
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch from d08e907 to 4802b31 Compare May 23, 2024 14:56
@OwenKephart OwenKephart changed the base branch from 05-20-Create_CronTickPassedCondition to 05-23-Allow_AllPartitionsSubsets_in_TimeWindowPartitionMapping May 23, 2024 14:56
flame.svg Outdated Show resolved Hide resolved
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.

This is all quite slick. I think we need to do a naming pass on all the downstack concepts, but the core structure here is compelling.

@OwenKephart OwenKephart force-pushed the 05-23-Allow_AllPartitionsSubsets_in_TimeWindowPartitionMapping branch from bdbaf33 to 068c772 Compare May 30, 2024 19:42
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch from 844091e to e121cf7 Compare May 30, 2024 19:42
@OwenKephart OwenKephart force-pushed the 05-23-Allow_AllPartitionsSubsets_in_TimeWindowPartitionMapping branch from 068c772 to c80942d Compare May 30, 2024 23:34
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch from e121cf7 to e5f3e6b Compare May 30, 2024 23:34
@OwenKephart OwenKephart force-pushed the 05-23-Allow_AllPartitionsSubsets_in_TimeWindowPartitionMapping branch from c80942d to 6ea95c7 Compare May 30, 2024 23:48
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch from e5f3e6b to 3383bad Compare May 30, 2024 23:48
@OwenKephart OwenKephart force-pushed the 05-23-Allow_AllPartitionsSubsets_in_TimeWindowPartitionMapping branch from 6ea95c7 to 792aa21 Compare May 31, 2024 17:26
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch from 3383bad to 065bd0c Compare May 31, 2024 17:26
@OwenKephart OwenKephart force-pushed the 05-23-Allow_AllPartitionsSubsets_in_TimeWindowPartitionMapping branch from 792aa21 to 5b01fff Compare May 31, 2024 17:56
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch from 065bd0c to ff4397d Compare May 31, 2024 17:56
| SchedulingCondition.will_be_requested()
)
all_deps_updated_since_cron = SchedulingCondition.all_deps_match(
SchedulingCondition.newly_updated().since_last_cron_tick(cron_schedule, cron_timezone)
Copy link
Member

Choose a reason for hiding this comment

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

this is super cool

@OwenKephart OwenKephart force-pushed the 05-23-Allow_AllPartitionsSubsets_in_TimeWindowPartitionMapping branch from 5b01fff to 8af990d Compare May 31, 2024 22:50
@OwenKephart OwenKephart force-pushed the 05-20-Replace_UpdatedSinceCronCondition_with_XSinceYCondition branch from ff4397d to 5d95b55 Compare May 31, 2024 22:50
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