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][28/n] Create WillBeRequestedCondition #21640

Merged
merged 1 commit into from
May 14, 2024

Conversation

OwenKephart
Copy link
Contributor

Summary & Motivation

Creates a WillBeRequestedCondition. Naming is somewhat sketch, but the basic idea is that in the past we've merged together this concept with existing rules.

I.e. in AMP-world, the "skip_on_parent_missing" rule is actually "skip on parent (missing and will not be requested this tick)".

This is obviously a bit confusing, and now that all of our dep conditions are composable, breaking this out into its own separate condition is desirable.

How I Tested These Changes

Copy link
Contributor Author

OwenKephart commented May 3, 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 force-pushed the 05-02-Update_values_within_InLatestTimeWindow branch from c1120be to 102f170 Compare May 3, 2024 21:11
@OwenKephart OwenKephart force-pushed the 05-03-Create_WillBeRequested_condition branch from 745c94c to ff7943c Compare May 3, 2024 21:12
@OwenKephart OwenKephart force-pushed the 05-02-Update_values_within_InLatestTimeWindow branch from 102f170 to a226325 Compare May 3, 2024 22:51
@OwenKephart OwenKephart force-pushed the 05-03-Create_WillBeRequested_condition branch from ff7943c to 5b377a0 Compare May 3, 2024 22:51
@ion-elgreco
Copy link
Contributor

ion-elgreco commented May 4, 2024

Could you add a condition that instead of only working on any or all assets, it applies to an explicit subset?

@OwenKephart OwenKephart changed the base branch from 05-02-Update_values_within_InLatestTimeWindow to 05-03-Make_candidate_slice_and_true_slice_Optional May 6, 2024 20:04
@OwenKephart OwenKephart force-pushed the 05-03-Create_WillBeRequested_condition branch from 5b377a0 to 449ff84 Compare May 6, 2024 20:04
@OwenKephart OwenKephart changed the title [DS][27/n] Create WillBeRequestedCondition [DS][29/n] Create WillBeRequestedCondition May 6, 2024
@OwenKephart OwenKephart force-pushed the 05-03-Make_candidate_slice_and_true_slice_Optional branch from e138ccc to f8b4f5d Compare May 6, 2024 23:45
@OwenKephart OwenKephart force-pushed the 05-03-Create_WillBeRequested_condition branch from 449ff84 to 006a6b4 Compare May 6, 2024 23:45
@OwenKephart OwenKephart changed the title [DS][29/n] Create WillBeRequestedCondition [DS][28/n] Create WillBeRequestedCondition May 8, 2024
@OwenKephart OwenKephart requested a review from schrockn May 8, 2024 22:14
@OwenKephart OwenKephart force-pushed the 05-03-Make_candidate_slice_and_true_slice_Optional branch from 7c8a70c to a4f2744 Compare May 8, 2024 23:00
@OwenKephart OwenKephart force-pushed the 05-03-Create_WillBeRequested_condition branch from ae6fe51 to d01bb66 Compare May 8, 2024 23:00
@@ -48,6 +48,31 @@ def compute_slice(self, context: SchedulingContext) -> AssetSlice:
return context.asset_graph_view.compute_in_progress_asset_slice(context.asset_key)


@whitelist_for_serdes
class WillBeRequestedCondition(SliceSchedulingCondition):
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think class name should include the word "tick" just like the static constructor and description

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.

Just req'ing changes based on class name to make sure you see it since the name will get serialized and it is hard to change.

@OwenKephart
Copy link
Contributor Author

@ion-elgreco

Could you add a condition that instead of only working on any or all assets, it applies to an explicit subset?

Adding that functionality to SchedulingCondition.any_deps_match and similar is certainly in the plans -- currently conceptualizing adding allow_keys and ignore_keys type arguments.

@OwenKephart OwenKephart force-pushed the 05-03-Make_candidate_slice_and_true_slice_Optional branch from a4f2744 to ba19f8d Compare May 10, 2024 22:44
@OwenKephart OwenKephart force-pushed the 05-03-Create_WillBeRequested_condition branch from 389c08d to 6a8d1b0 Compare May 10, 2024 22:44
@OwenKephart OwenKephart requested a review from schrockn May 10, 2024 22:47
@OwenKephart OwenKephart force-pushed the 05-03-Make_candidate_slice_and_true_slice_Optional branch from ba19f8d to 1bd133f Compare May 13, 2024 17:29
@OwenKephart OwenKephart force-pushed the 05-03-Create_WillBeRequested_condition branch from 6a8d1b0 to 7732a0a Compare May 13, 2024 17:29
@OwenKephart OwenKephart force-pushed the 05-03-Make_candidate_slice_and_true_slice_Optional branch from 1bd133f to 56c11e4 Compare May 14, 2024 13:47
@OwenKephart OwenKephart force-pushed the 05-03-Create_WillBeRequested_condition branch from 7732a0a to 09e1d91 Compare May 14, 2024 13:47
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:45 AM EDT: Graphite rebased this pull request as part of a merge.
  • May 14, 10:46 AM EDT: @OwenKephart merged this pull request with Graphite.

@OwenKephart OwenKephart force-pushed the 05-03-Make_candidate_slice_and_true_slice_Optional branch from 56c11e4 to 10b0a4b Compare May 14, 2024 14:43
Base automatically changed from 05-03-Make_candidate_slice_and_true_slice_Optional to master May 14, 2024 14:44
@OwenKephart OwenKephart force-pushed the 05-03-Create_WillBeRequested_condition branch from 09e1d91 to 1a917ab Compare May 14, 2024 14:44
@OwenKephart OwenKephart merged commit 271bd37 into master May 14, 2024
1 check was pending
@OwenKephart OwenKephart deleted the 05-03-Create_WillBeRequested_condition branch May 14, 2024 14:46
danielgafni pushed a commit to danielgafni/dagster that referenced this pull request Jun 18, 2024
## Summary & Motivation

Creates a WillBeRequestedCondition. Naming is somewhat sketch, but the basic idea is that in the past we've merged together this concept with existing rules.

I.e. in AMP-world, the "skip_on_parent_missing" rule is actually "skip on parent (missing and will not be requested this tick)".

This is obviously a bit confusing, and now that all of our dep conditions are composable, breaking this out into its own separate condition is desirable.

## 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

3 participants