-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
support Asset and Op ExecutionContexts in standard execution path #17972
support Asset and Op ExecutionContexts in standard execution path #17972
Conversation
e01caed
to
f724155
Compare
513ece0
to
8a1bc21
Compare
f724155
to
541fde7
Compare
8a1bc21
to
e64fb39
Compare
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit e64fb39. |
fd482bd
to
18c5c92
Compare
a8f971d
to
a45639e
Compare
a6d086b
to
62b7ad6
Compare
a45639e
to
115aabb
Compare
5043c25
to
770c04c
Compare
07736d6
to
815d4e3
Compare
770c04c
to
4acf05c
Compare
815d4e3
to
f172b7e
Compare
4acf05c
to
4e90152
Compare
f172b7e
to
5960e17
Compare
455bd9c
to
63cf5b0
Compare
"_ExecutionProperties", | ||
[ | ||
("step_description", PublicAttr[str]), | ||
("op_execution_context", PublicAttr["OpExecutionContext"]), |
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.
having to pass the OpExecutionContext here is kind of annoying, but the alternative is doing isinstance(context, AssetExecutionContext)
checks in the core execution code so that we can access context.op_execution_context
when the context is an AssetExecutionContext
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.
another option could be to move the functions needed from OpExecutionContext
in the core execution path (has_events
and consume_events
) into the ExecutionProperties
named tuple. I can look into that option as well. It might require passing the step execution context to this class, so might not be much better
cae085a
to
3a8e919
Compare
63cf5b0
to
e5e974c
Compare
3a8e919
to
fa8509f
Compare
fc6cef4
to
358d4f4
Compare
58934d1
to
fe91874
Compare
Summary & Motivation
This PR is not strictly required to merge #17339. However, as we deprecate more methods, we will begin to deprecate context methods that are called in the main execution pathway.
This PR proposes adding a subobject to both AssetExecutionContext and OpExecutionContext that contains all methods/properties that need to be accessed in the execution code path.
How I Tested These Changes