Skip to content

Commit

Permalink
[external-assets] Cache asset key subsets in refactored AssetGraph (#…
Browse files Browse the repository at this point in the history
…20059)

## Summary & Motivation

#19900 led to an auto-materialize performance regression caught by our
regression tests. After investigating with a profiler, I found the issue
was lack of caching on asset key subset methods in the new `AssetGraph`.
This PR adds them.

## How I Tested These Changes

Ran perf regression tests against this branch.
  • Loading branch information
smackesey authored and PedramNavid committed Mar 28, 2024
1 parent 158ef66 commit f24970b
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,12 @@ def asset_dep_graph(self) -> DependencyGraph[AssetKey]:
return {"upstream": upstream, "downstream": downstream}

@property
@cached_method
def all_asset_keys(self) -> AbstractSet[AssetKey]:
return {node.asset_key for node in self.asset_nodes}

@property
@cached_method
def materializable_asset_keys(self) -> AbstractSet[AssetKey]:
return {
node.asset_key
Expand All @@ -195,6 +197,7 @@ def is_materializable(self, asset_key: AssetKey) -> bool:
return self.get_asset_node(asset_key).execution_type == AssetExecutionType.MATERIALIZATION

@property
@cached_method
def observable_asset_keys(self) -> AbstractSet[AssetKey]:
return {
node.asset_key
Expand All @@ -208,6 +211,7 @@ def is_observable(self, asset_key: AssetKey) -> bool:
return self.get_asset_node(asset_key).is_observable

@property
@cached_method
def external_asset_keys(self) -> AbstractSet[AssetKey]:
return {
node.asset_key
Expand All @@ -219,6 +223,7 @@ def is_external(self, asset_key: AssetKey) -> bool:
return self.get_asset_node(asset_key).execution_type != AssetExecutionType.MATERIALIZATION

@property
@cached_method
def executable_asset_keys(self) -> AbstractSet[AssetKey]:
return {node.asset_key for node in self.asset_nodes if node.is_executable}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,12 @@ def asset_dep_graph(self) -> DependencyGraph[AssetKey]:
return generate_asset_dep_graph(self._assets_defs, [])

@property
@cached_method
def all_asset_keys(self) -> AbstractSet[AssetKey]:
return {key for ad in self._assets_defs for key in ad.keys}

@property
@cached_method
def materializable_asset_keys(self) -> AbstractSet[AssetKey]:
return {key for ad in self._assets_defs if ad.is_materializable for key in ad.keys}

Expand All @@ -124,20 +126,23 @@ def is_materializable(self, asset_key: AssetKey) -> bool:
return self.has_asset(asset_key) and self.get_assets_def(asset_key).is_materializable

@property
@cached_method
def observable_asset_keys(self) -> AbstractSet[AssetKey]:
return {key for ad in self._assets_defs if ad.is_observable for key in ad.keys}

def is_observable(self, asset_key: AssetKey) -> bool:
return self.get_assets_def(asset_key).is_observable

@property
@cached_method
def external_asset_keys(self) -> AbstractSet[AssetKey]:
return {key for ad in self._assets_defs if ad.is_external for key in ad.keys}

def is_external(self, asset_key: AssetKey) -> bool:
return self.get_assets_def(asset_key).is_external

@property
@cached_method
def executable_asset_keys(self) -> AbstractSet[AssetKey]:
return {key for ad in self._assets_defs if ad.is_executable for key in ad.keys}

Expand Down

0 comments on commit f24970b

Please sign in to comment.