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

provide AssetExecutionContext class to context #16493

Closed

Conversation

jamiedemaria
Copy link
Contributor

@jamiedemaria jamiedemaria commented Sep 13, 2023

Summary & Motivation

Introduces the logic to determine which kind of context to provide to which kind of step.

Follows these rules:

@asset + no type annotation -> AssetExecutionContext
@asset + AssetExecutionContext annotation -> AssetExecutionContext
@asset + OpExecutionContext annotation -> OpExecutionContext 
@op + no type annotation -> OpExecutionContext 
@op + AssetExecutionContext annotation -> Error - we need an AssetsDef for AssetExecutionContext init, and op doesn't have one 
@op + OpExecutionContext -> OpExecutionContext 

The ops that make up graph backed assets is where is gets funky. This is what would happen in the current implementation:

@graph_asset @op + no type annotation -> OpExecutionContext
@graph_asset @op + AssetExecutionContext -> AssetExecutionContext 
@graph_asset @op + OpExecutionContext -> OpExecutionContext

The reasoning behind @graph_asset @op + no type annotation -> OpExecutionContext is best explained with an example:
Imagine you have an op that is used in both a job and a graph-backed asset

@op 
def my_fun_op(context):
   context.describe_op # this function is on the OpExecutionContext, but not on the AssetExecutionContext

@job 
def my_fun_job():
    my_fun_op()

@graph_asset 
def my_fun_asset():
    my_fun_op()

When executing my_fun_job, my_fun_op will receive an OpExecutionContext and run as expected. When materializing my_fun_asset, my_fun_op will receive an AssetExecutionContext. It will fire a deprecation warning, but still call describe_op on the internally held op_execution_context. When we deprecate the __get_attr__ behavior here, my_fun_op will instead error.

How I Tested These Changes

@@ -864,6 +864,10 @@ def op_config(self) -> Any:
op_config = self.resolved_run_config.ops.get(str(self.node_handle))
return op_config.config if op_config else None

@property
def is_graph_asset_op(self) -> bool:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in combination with is_sda_step this will tell us if an op is in a graph backed asset.

@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from e8fa9da to 28e28f8 Compare September 18, 2023 15:06
@jamiedemaria jamiedemaria changed the base branch from jamie/asset-context to jamie/subclass-asset-context September 18, 2023 18:15
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from 28e28f8 to 12894b6 Compare September 18, 2023 18:15
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from 12894b6 to ec70892 Compare September 18, 2023 20:14
@jamiedemaria jamiedemaria changed the title [RFC] provide AssetExecutionContext class to context provide AssetExecutionContext class to context Sep 18, 2023
@jamiedemaria jamiedemaria requested a review from yuhan September 18, 2023 20:54
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from ec70892 to a09fa99 Compare September 19, 2023 14:27
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from a09fa99 to bcf4e6e Compare September 19, 2023 21:38
@jamiedemaria
Copy link
Contributor Author

jamiedemaria commented Sep 19, 2023

Edge case to consider:

@op(config_schema={"foo": int})
    def one(context):
        assert context.op_config["foo"] == 1

asset_one = AssetsDefinition.from_op(one)

the op gets an AssetExecutionConext - need to decide if that's what we want

@jamiedemaria jamiedemaria changed the base branch from jamie/subclass-asset-context to jamie/partition-method September 20, 2023 16:43
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from 5e7ed31 to dfd922e Compare September 20, 2023 16:43
@jamiedemaria jamiedemaria force-pushed the jamie/subclass-asset-context branch from ad9cab2 to 7231b2f Compare September 21, 2023 21:02
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from 07101b5 to d14f690 Compare September 21, 2023 21:02
@jamiedemaria jamiedemaria force-pushed the jamie/subclass-asset-context branch from 7231b2f to e688144 Compare September 21, 2023 21:05
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from d14f690 to ecb5178 Compare September 21, 2023 21:05
@jamiedemaria jamiedemaria force-pushed the jamie/subclass-asset-context branch from e688144 to 27152b9 Compare September 22, 2023 13:08
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from ecb5178 to 9937b2f Compare September 22, 2023 13:08
@jamiedemaria jamiedemaria force-pushed the jamie/subclass-asset-context branch from 27152b9 to d01d5d2 Compare September 22, 2023 14:27
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch 2 times, most recently from a9785f3 to 583cb1e Compare September 22, 2023 15:16
context_param = compute_fn.get_context_arg()
context_annotation = context_param.annotation

# TODO - i dont know how to move this check to Definition time since we don't know if the op is
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO - i'd like to move this check to one we can catch at definition time, but i'm not sure if we can since with just this

@op 
def my_op(context: AssetExecutionContext)

we don't know if the op will be used in a graph backed asset (meaning this annotation is fine) or in an op job (meaning this annotation is not fine)

@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from 583cb1e to 0885642 Compare September 22, 2023 16:02
@jamiedemaria jamiedemaria force-pushed the jamie/subclass-asset-context branch from 2e71fd1 to 49b0a60 Compare September 22, 2023 17:46
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch 2 times, most recently from 84d7ea6 to a76c119 Compare September 22, 2023 20:50
@jamiedemaria jamiedemaria force-pushed the jamie/subclass-asset-context branch from 1448509 to 5ba005b Compare September 26, 2023 14:06
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from a76c119 to d2c1d93 Compare September 26, 2023 14:06
Copy link
Member

@alangenfeld alangenfeld left a comment

Choose a reason for hiding this comment

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

the change discussed in the summary got landed in another PR right?

@jamiedemaria
Copy link
Contributor Author

closed in favor of #16759

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.

2 participants