Skip to content

Commit

Permalink
Remove description property, make names more descriptive (#25186)
Browse files Browse the repository at this point in the history
## Summary & Motivation

The `description` property is a holdover from the old UI flow, and is not rendered in the new UI. Therefore, having it be specified on a bunch of built in conditions was a waste. This updates the name properties to be more descriptive (containing the relevant properties), which allows them to be represented in the UI.

## How I Tested These Changes

## Changelog

NOCHANGELOG
  • Loading branch information
OwenKephart authored Oct 15, 2024
1 parent 8a30044 commit 564cd78
Show file tree
Hide file tree
Showing 10 changed files with 49 additions and 104 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def get_snapshot(

def get_node_unique_id(self, *, parent_unique_id: Optional[str], index: Optional[int]) -> str:
"""Returns a unique identifier for this condition within the broader condition tree."""
parts = [str(parent_unique_id), str(index), self.__class__.__name__, self.description]
parts = [str(parent_unique_id), str(index), self.name]
return non_secure_md5_hash_str("".join(parts).encode())

def get_unique_id(
Expand Down Expand Up @@ -453,9 +453,7 @@ def cron_tick_passed(
CronTickPassedCondition,
)

return CronTickPassedCondition(
cron_schedule=cron_schedule, cron_timezone=cron_timezone
).with_label(f"cron_tick_passed({cron_schedule}, {cron_timezone})")
return CronTickPassedCondition(cron_schedule=cron_schedule, cron_timezone=cron_timezone)

@experimental
@staticmethod
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@
@whitelist_for_serdes
@record
class CodeVersionChangedCondition(BuiltinAutomationCondition[AssetKey]):
@property
def description(self) -> str:
return "Asset code version changed since previous tick"

@property
def name(self) -> str:
return "code_version_changed"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@
class InitialEvaluationCondition(BuiltinAutomationCondition):
"""Condition to determine if this is the initial evaluation of a given AutomationCondition."""

@property
def description(self) -> str:
return "Initial evaluation"

@property
def name(self) -> str:
return "initial_evaluation"
Expand Down Expand Up @@ -63,10 +59,6 @@ def evaluate(self, context: AutomationContext[T_EntityKey]) -> AutomationResult[
@whitelist_for_serdes
@record
class MissingAutomationCondition(SubsetAutomationCondition):
@property
def description(self) -> str:
return "Missing"

@property
def name(self) -> str:
return "missing"
Expand All @@ -80,10 +72,6 @@ def compute_subset(self, context: AutomationContext) -> EntitySubset:
@whitelist_for_serdes
@record
class InProgressAutomationCondition(SubsetAutomationCondition):
@property
def description(self) -> str:
return "Part of an in-progress run"

@property
def name(self) -> str:
return "in_progress"
Expand All @@ -95,10 +83,6 @@ def compute_subset(self, context: AutomationContext) -> EntitySubset:
@whitelist_for_serdes(storage_name="FailedAutomationCondition")
@record
class ExecutionFailedAutomationCondition(SubsetAutomationCondition):
@property
def description(self) -> str:
return "Latest execution failed"

@property
def name(self) -> str:
return "execution_failed"
Expand Down Expand Up @@ -140,10 +124,6 @@ def compute_subset(self, context: AutomationContext) -> EntitySubset:
@whitelist_for_serdes
@record
class NewlyRequestedCondition(SubsetAutomationCondition):
@property
def description(self) -> str:
return "Was requested on the previous tick"

@property
def name(self) -> str:
return "newly_requested"
Expand All @@ -155,10 +135,6 @@ def compute_subset(self, context: AutomationContext) -> EntitySubset:
@whitelist_for_serdes
@record
class NewlyUpdatedCondition(SubsetAutomationCondition):
@property
def description(self) -> str:
return "Updated since previous tick"

@property
def name(self) -> str:
return "newly_updated"
Expand All @@ -178,13 +154,9 @@ class CronTickPassedCondition(SubsetAutomationCondition):
cron_schedule: str
cron_timezone: str

@property
def description(self) -> str:
return f"New tick of {self.cron_schedule} ({self.cron_timezone})"

@property
def name(self) -> str:
return "cron_tick_passed"
return f"cron_tick_passed(cron_schedule={self.cron_schedule}, cron_timezone={self.cron_timezone})"

def _get_previous_cron_tick(self, effective_dt: datetime.datetime) -> datetime.datetime:
previous_ticks = reverse_cron_string_iterator(
Expand Down Expand Up @@ -240,7 +212,10 @@ def description(self) -> str:

@property
def name(self) -> str:
return "in_latest_time_window"
name = "in_latest_time_window"
if self.serializable_lookback_timedelta:
name += f"(lookback_timedelta={self.lookback_timedelta})"
return name

def compute_subset(self, context: AutomationContext) -> EntitySubset:
return context.asset_graph_view.compute_latest_time_window_subset(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ class DownstreamConditionWrapperCondition(BuiltinAutomationCondition[AssetKey]):
operand: AutomationCondition

@property
def description(self) -> str:
return ",".join(key.to_user_string() for key in self.downstream_keys)
def name(self) -> str:
return ", ".join(key.to_user_string() for key in self.downstream_keys)

@property
def children(self) -> Sequence[AutomationCondition]:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,14 @@ class ChecksAutomationCondition(BuiltinAutomationCondition[AssetKey]):

@property
@abstractmethod
def base_description(self) -> str: ...
def base_name(self) -> str: ...

@property
def description(self) -> str:
description = f"{self.base_description} "
if self.blocking_only:
description += "blocking checks"
def name(self) -> str:
name = self.base_name
if self.blocking_only:
description += "checks"
return description
name += "(blocking_only=True)"
return name

@property
def requires_cursor(self) -> bool:
Expand All @@ -53,11 +51,7 @@ def _get_check_keys(
@record
class AnyChecksCondition(ChecksAutomationCondition):
@property
def base_description(self) -> str:
return "Any"

@property
def name(self) -> str:
def base_name(self) -> str:
return "ANY_CHECKS_MATCH"

def evaluate(self, context: AutomationContext[AssetKey]) -> AutomationResult[AssetKey]:
Expand Down Expand Up @@ -86,11 +80,7 @@ def evaluate(self, context: AutomationContext[AssetKey]) -> AutomationResult[Ass
@record
class AllChecksCondition(ChecksAutomationCondition):
@property
def base_description(self) -> str:
return "All"

@property
def name(self) -> str:
def base_name(self) -> str:
return "ALL_CHECKS_MATCH"

def evaluate(self, context: AutomationContext[AssetKey]) -> AutomationResult[AssetKey]:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,16 +66,20 @@ class DepsAutomationCondition(BuiltinAutomationCondition[T_EntityKey]):

@property
@abstractmethod
def base_description(self) -> str: ...
def base_name(self) -> str: ...

@property
def description(self) -> str:
description = f"{self.base_description} deps"
def name(self) -> str:
name = self.base_name
props = []
if self.allow_selection is not None:
description += f" within selection {self.allow_selection}"
props.append("allow_selection={self.allow_selection}")
if self.ignore_selection is not None:
description += f" except for {self.ignore_selection}"
return description
props.append("ignore_selection={self.ignore_selection}")

if props:
name += f"({','.join(props)})"
return name

@property
def requires_cursor(self) -> bool:
Expand Down Expand Up @@ -119,11 +123,7 @@ def _get_dep_keys(
@whitelist_for_serdes
class AnyDepsCondition(DepsAutomationCondition[T_EntityKey]):
@property
def base_description(self) -> str:
return "Any"

@property
def name(self) -> str:
def base_name(self) -> str:
return "ANY_DEPS_MATCH"

def evaluate(self, context: AutomationContext[T_EntityKey]) -> AutomationResult[T_EntityKey]:
Expand All @@ -149,11 +149,7 @@ def evaluate(self, context: AutomationContext[T_EntityKey]) -> AutomationResult[
@whitelist_for_serdes
class AllDepsCondition(DepsAutomationCondition[T_EntityKey]):
@property
def base_description(self) -> str:
return "All"

@property
def name(self) -> str:
def base_name(self) -> str:
return "ALL_DEPS_MATCH"

def evaluate(self, context: AutomationContext[T_EntityKey]) -> AutomationResult[T_EntityKey]:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,6 @@
class NewlyTrueCondition(BuiltinAutomationCondition[T_EntityKey]):
operand: AutomationCondition[T_EntityKey]

@property
def description(self) -> str:
return "Condition newly became true."

@property
def name(self) -> str:
return "NEWLY_TRUE"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,6 @@ class SinceCondition(BuiltinAutomationCondition[T_EntityKey]):
trigger_condition: AutomationCondition[T_EntityKey]
reset_condition: AutomationCondition[T_EntityKey]

@property
def description(self) -> str:
return (
"Trigger condition has become true since the last time the reset condition became true."
)

@property
def name(self) -> str:
return "SINCE"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,11 @@ def d(): ...
assert len(a_result.child_results) == 2

res1 = a_result.child_results[0]
assert res1.condition.description == "b,d"
assert res1.condition.name == "b, d"
assert res1.child_results[0].condition == cond2

res2 = a_result.child_results[1]
assert res2.condition.description == "c"
assert res2.condition.name == "c"
assert res2.child_results[0].condition == cond3


Expand Down Expand Up @@ -126,16 +126,16 @@ def d(): ...

# the first condition is a bit gnarly, but it should resolve to b: ((d: eager) & ~in_progress)
res1 = a_result.child_results[0]
assert res1.condition.description == "b"
assert res1.condition.name == "b"
res1_and = res1.child_results[0]
assert isinstance(res1_and.condition, AndAutomationCondition)
res1_1 = res1_and.child_results[0].child_results[0]
assert res1_1.condition.description == "d"
assert res1_1.condition.name == "d"
assert res1_1.child_results[0].condition == cond3
res1_2 = res1_and.child_results[1]
assert res1_2.condition == ~AutomationCondition.in_progress()

# the second condition resolves to just (d: eager)
res2 = a_result.child_results[1]
assert res2.condition.description == "d"
assert res2.condition.name == "d"
assert res2.child_results[0].condition == cond3
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,24 @@
[
# cron condition returns a unique value hash if parents change, if schedule changes, if the
# partitions def changes, or if an asset is materialized
("ad228f0044da1efba407e794c845e858", SC.on_cron("0 * * * *"), one_parent, False),
("f34de3cd3e1ab283a95a892192437076", SC.on_cron("0 * * * *"), one_parent, True),
("d9533b4eb0aad1798d5da85520b9852c", SC.on_cron("0 0 * * *"), one_parent, False),
("8a233d38e569faba1470b0717c28fbee", SC.on_cron("0 * * * *"), one_parent_daily, False),
("e8fa53c550e99edc1346a1f80979cddd", SC.on_cron("0 * * * *"), two_parents, False),
("5c58fb8fc117d69b32e734c45af219ea", SC.on_cron("0 * * * *"), two_parents_daily, False),
("93831bb3ab9c6ef4b10a7f823ce5cc1f", SC.on_cron("0 * * * *"), one_parent, False),
("97ea4d62fcef2f6a2ecc99a95d5c1769", SC.on_cron("0 * * * *"), one_parent, True),
("504192a87594854d3964bb03e2092394", SC.on_cron("0 0 * * *"), one_parent, False),
("ccbd282bf8e6d711c2bb0e01ebb16728", SC.on_cron("0 * * * *"), one_parent_daily, False),
("dd74c7cfe19d869931ea4aad9ee10127", SC.on_cron("0 * * * *"), two_parents, False),
("861f8e40d4624d49c4ebdd034c8e1e84", SC.on_cron("0 * * * *"), two_parents_daily, False),
# same as above
("678e0a2be6bba89bd2d37fb432d8fb51", SC.eager(), one_parent, False),
("72bd068363441e02d67b3407fe3e9cae", SC.eager(), one_parent, True),
("4f0e2e38131ae91b1b9408e3cd549dd0", SC.eager(), one_parent_daily, False),
("00ecedd77a8d887940856950c556c7d1", SC.eager(), two_parents, False),
("6ad1fd331c63c75e17572ec60b9c27b5", SC.eager(), two_parents_daily, False),
("b5cb0d7a1c627bd2c9e7c6da3313ab71", SC.eager(), one_parent, False),
("7802a65024d04bbe44a3e0e541c0a577", SC.eager(), one_parent, True),
("5d9c70da7ecca9e40f32c1ad99956b5d", SC.eager(), one_parent_daily, False),
("904bac575906542d28b9e069129dad37", SC.eager(), two_parents, False),
("3ef1d373a2b38752ad8e23fe9c053d9f", SC.eager(), two_parents_daily, False),
# missing condition is invariant to changes other than partitions def changes
("5c24ffc21af9983a4917b91290de8f5d", SC.missing(), one_parent, False),
("5c24ffc21af9983a4917b91290de8f5d", SC.missing(), one_parent, True),
("5c24ffc21af9983a4917b91290de8f5d", SC.missing(), two_parents, False),
("c722c1abf97c5f5fe13b2f6fc00af739", SC.missing(), two_parents_daily, False),
("c722c1abf97c5f5fe13b2f6fc00af739", SC.missing(), one_parent_daily, False),
("6d7809c4949e3d812d7eddfb1b60d529", SC.missing(), one_parent, False),
("6d7809c4949e3d812d7eddfb1b60d529", SC.missing(), one_parent, True),
("6d7809c4949e3d812d7eddfb1b60d529", SC.missing(), two_parents, False),
("7f852ab7408c67e0830530d025505a37", SC.missing(), two_parents_daily, False),
("7f852ab7408c67e0830530d025505a37", SC.missing(), one_parent_daily, False),
],
)
def test_value_hash(
Expand Down

0 comments on commit 564cd78

Please sign in to comment.