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][40/n] Create an as_auto_materialize_policy method on SchedulingCondition #21788

Conversation

OwenKephart
Copy link
Contributor

Summary & Motivation

As title -- this is a more convenient way of converting a condition to a policy, as it doesn't require an additional import.

Also makes sure that the policy survives serdes and all that

How I Tested These Changes

Copy link
Contributor Author

OwenKephart commented May 10, 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



def test_defs() -> None:
@asset(auto_materialize_policy=SchedulingCondition.eager().as_auto_materialize_policy())
Copy link
Member

Choose a reason for hiding this comment

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

I think given this if I were a user the first think I would write is a shorthand function like as_amp_policy.

auto_materialize_policy=AutoMaterializePolicy.from_scheduling_condition(SchedulingCondition.eager() also an option

@asset(auto_materialize_policy=as_amp_policy(SchedulingCondition.eager())

I think the as_auto_materialize_policy might be a bit annoying to detail with in cases where there are complicated boolean experessions. I think it will be easy to make the mistake where you forgot to wrap the entire thing in parents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@schrockn "as amp policy" = "as auto materialize policy policy" (ATM machine-type thing). I think as_amp() might be too "informal".

AutoMaterializePolicy.from_scheduling_condition is possible for the user to do in this diff, but the drawback is they now need two imports (AutoMaterializePolicy and SchedulingCondition).

Considering we'd ideally want this construction to be fairly short-lived, I think a bit of verbosity isn't the end of the world, but open to pushback. Maybe the best solution is just to remove the as_auto_materialize_policy() thing and force users to go through from_scheduling_condition()?

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.

Question/alt suggestion inline

@OwenKephart OwenKephart force-pushed the 05-08-Update_eager_condition_to_not_retry_as_frequently_on_failure branch from eda89ce to 105586a Compare May 10, 2024 22:14
@OwenKephart OwenKephart force-pushed the 05-10-Create_an_as_auto_materialize_policy_method_on_SchedulingCondition branch from 63d0625 to 79ea6be Compare May 10, 2024 22:14
@OwenKephart OwenKephart force-pushed the 05-08-Update_eager_condition_to_not_retry_as_frequently_on_failure branch from 105586a to 00932a0 Compare May 10, 2024 22:45
@OwenKephart OwenKephart force-pushed the 05-10-Create_an_as_auto_materialize_policy_method_on_SchedulingCondition branch from 79ea6be to fdfe042 Compare May 10, 2024 22:45
@OwenKephart OwenKephart changed the title [DS][x/n] Create an as_auto_materialize_policy method on SchedulingCondition [DS][40/n] Create an as_auto_materialize_policy method on SchedulingCondition May 10, 2024
@OwenKephart OwenKephart requested a review from schrockn May 10, 2024 22:46
@OwenKephart OwenKephart force-pushed the 05-08-Update_eager_condition_to_not_retry_as_frequently_on_failure branch from 00932a0 to 236a661 Compare May 13, 2024 17:30
@OwenKephart OwenKephart force-pushed the 05-10-Create_an_as_auto_materialize_policy_method_on_SchedulingCondition branch from fdfe042 to ea24387 Compare May 13, 2024 17:30
@OwenKephart OwenKephart force-pushed the 05-08-Update_eager_condition_to_not_retry_as_frequently_on_failure branch from 236a661 to edb5d0f Compare May 13, 2024 17:54
@OwenKephart OwenKephart force-pushed the 05-10-Create_an_as_auto_materialize_policy_method_on_SchedulingCondition branch from ea24387 to 09b88d2 Compare May 13, 2024 17:54
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.

I think I prefer AutoMaterializePolicy.from_scheduling_condition but we can change this once we get going writing docs etc so let's just press on

@OwenKephart OwenKephart force-pushed the 05-08-Update_eager_condition_to_not_retry_as_frequently_on_failure branch from edb5d0f to 3a9b64b Compare May 14, 2024 13:48
@OwenKephart OwenKephart force-pushed the 05-10-Create_an_as_auto_materialize_policy_method_on_SchedulingCondition branch from 09b88d2 to 02031a7 Compare May 14, 2024 13:48
@OwenKephart OwenKephart force-pushed the 05-08-Update_eager_condition_to_not_retry_as_frequently_on_failure branch from 3a9b64b to 8156f78 Compare May 14, 2024 13:56
@OwenKephart OwenKephart force-pushed the 05-10-Create_an_as_auto_materialize_policy_method_on_SchedulingCondition branch from 02031a7 to 94ef757 Compare May 14, 2024 13:56
@OwenKephart OwenKephart force-pushed the 05-08-Update_eager_condition_to_not_retry_as_frequently_on_failure branch from 8156f78 to d270075 Compare May 14, 2024 13:59
@OwenKephart OwenKephart force-pushed the 05-10-Create_an_as_auto_materialize_policy_method_on_SchedulingCondition branch from 94ef757 to 49cb9db Compare May 14, 2024 13:59
@OwenKephart OwenKephart force-pushed the 05-08-Update_eager_condition_to_not_retry_as_frequently_on_failure branch from d270075 to 8b48a51 Compare May 14, 2024 14:14
@OwenKephart OwenKephart force-pushed the 05-10-Create_an_as_auto_materialize_policy_method_on_SchedulingCondition branch from 49cb9db to ebf7c8e Compare May 14, 2024 14:14
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, 11:17 AM EDT: Graphite rebased this pull request as part of a merge.
  • May 14, 11:18 AM EDT: @OwenKephart merged this pull request with Graphite.

@OwenKephart OwenKephart force-pushed the 05-08-Update_eager_condition_to_not_retry_as_frequently_on_failure branch from 8b48a51 to 50eaa6c Compare May 14, 2024 15:13
Base automatically changed from 05-08-Update_eager_condition_to_not_retry_as_frequently_on_failure to master May 14, 2024 15:15
@OwenKephart OwenKephart force-pushed the 05-10-Create_an_as_auto_materialize_policy_method_on_SchedulingCondition branch from ebf7c8e to fc4966f Compare May 14, 2024 15:16
@OwenKephart OwenKephart merged commit d5a4bc6 into master May 14, 2024
1 check was pending
@OwenKephart OwenKephart deleted the 05-10-Create_an_as_auto_materialize_policy_method_on_SchedulingCondition branch May 14, 2024 15:18
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