-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[DS][40/n] Create an as_auto_materialize_policy method on SchedulingCondition #21788
Conversation
|
||
|
||
def test_defs() -> None: | ||
@asset(auto_materialize_policy=SchedulingCondition.eager().as_auto_materialize_policy()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
?
There was a problem hiding this 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
eda89ce
to
105586a
Compare
63d0625
to
79ea6be
Compare
105586a
to
00932a0
Compare
79ea6be
to
fdfe042
Compare
00932a0
to
236a661
Compare
fdfe042
to
ea24387
Compare
236a661
to
edb5d0f
Compare
ea24387
to
09b88d2
Compare
There was a problem hiding this 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
edb5d0f
to
3a9b64b
Compare
09b88d2
to
02031a7
Compare
3a9b64b
to
8156f78
Compare
02031a7
to
94ef757
Compare
8156f78
to
d270075
Compare
94ef757
to
49cb9db
Compare
d270075
to
8b48a51
Compare
49cb9db
to
ebf7c8e
Compare
Merge activity
|
8b48a51
to
50eaa6c
Compare
ebf7c8e
to
fc4966f
Compare
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