Simplified AssetExecutionContext #16487
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
jamiedemaria
changed the title
|
|
||
| @public | ||
| @experimental | ||
| def get_asset_provenance(self, asset_key: AssetKey) -> Optional[DataProvenance]: |
There was a problem hiding this comment.
if we want to simplify the context even further, we could make get_asset_provenance and get_assets_code_version not public since provenance, provenance_by_asset_key, code_version and code_version_by_asset_key should cover all use cases. If we get requests, we could then make these methods public
There was a problem hiding this comment.
I think the other option is to have a single object that has all the code version/provenance-related info and only return that.
|
|
||
| @public | ||
| # TODO - method naming. this needs work | ||
| def get_assets_code_version( |
There was a problem hiding this comment.
Alternate names:
get_code_versions_for_assets
Alternately, we could delete this method entirely, and update the impls of code_version and code_version_by_asset_key to
@property
def code_version(self) -> Optional[str]:
return self.code_version_by_asset_key[self.asset_key]
@property
def code_version_by_asset_key(self) -> Mapping[AssetKey, Optional[str]]:
return self.op_execution_context.instance.get_latest_materialization_code_versions(
self.asset_keys
)
|
|
||
| @public | ||
| def partition_key_range_for_asset_key(self, asset_key: AssetKey) -> PartitionKeyRange: | ||
| """TODO - implement in stacked pr.""" |
There was a problem hiding this comment.
implementing this method will require some changes the to step execution context. Leaving it for a stacked PR so that this PR stays easier to review
| "file_manager", | ||
| "has_assets_def", | ||
| "get_mapping_key", | ||
| # "get_step_execution_context", # used by internals |
There was a problem hiding this comment.
We should add these internal methods to a different set and emit a different warning
jamiedemaria
force-pushed
the
jamie/op-context-metaclass
branch
from
0ed2a54 to
9e2cf21
Compare
jamiedemaria
force-pushed
the
jamie/asset-context
branch
from
a6e9235 to
4694660
Compare
|
closing as we're going down the subclass route. Can re-open if needed |
Summary & Motivation
Breaks out the
AssetExecutionContextinto it's own class. This class should implement theIContextfrom #16480, but I can't import fromdagster-extso need to figure that part out first...How I Tested These Changes