From 1357a85f53ff3e46228e7ab6bd972b639622d921 Mon Sep 17 00:00:00 2001 From: Owen Kephart Date: Thu, 10 Oct 2024 09:42:20 -0700 Subject: [PATCH] Remove description property, make names more descriptive --- .../automation_condition.py | 6 ++-- .../code_version_changed_condition.py | 4 --- .../operands/slice_conditions.py | 35 +++---------------- .../any_downstream_conditions_operator.py | 4 +-- .../operators/check_operators.py | 24 ++++--------- .../operators/dep_operators.py | 28 +++++++-------- .../operators/newly_true_operator.py | 4 --- .../operators/since_operator.py | 6 ---- .../fundamentals/test_result_value_hash.py | 32 ++++++++--------- 9 files changed, 44 insertions(+), 99 deletions(-) diff --git a/python_modules/dagster/dagster/_core/definitions/declarative_automation/automation_condition.py b/python_modules/dagster/dagster/_core/definitions/declarative_automation/automation_condition.py index 772bf4aa6fd0e..2dd8fb30241b0 100644 --- a/python_modules/dagster/dagster/_core/definitions/declarative_automation/automation_condition.py +++ b/python_modules/dagster/dagster/_core/definitions/declarative_automation/automation_condition.py @@ -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( @@ -458,9 +458,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 diff --git a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operands/code_version_changed_condition.py b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operands/code_version_changed_condition.py index f68c4af467c3a..793736027a8a6 100644 --- a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operands/code_version_changed_condition.py +++ b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operands/code_version_changed_condition.py @@ -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" diff --git a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operands/slice_conditions.py b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operands/slice_conditions.py index 50b210f61a83a..4e64e91f8d907 100644 --- a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operands/slice_conditions.py +++ b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operands/slice_conditions.py @@ -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" @@ -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" @@ -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" @@ -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" @@ -144,10 +128,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" @@ -159,10 +139,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" @@ -188,13 +164,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( @@ -250,7 +222,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( diff --git a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/any_downstream_conditions_operator.py b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/any_downstream_conditions_operator.py index 161c062814bf5..30cd79f1c1fbf 100644 --- a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/any_downstream_conditions_operator.py +++ b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/any_downstream_conditions_operator.py @@ -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]: diff --git a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/check_operators.py b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/check_operators.py index c62a043152f3f..0f069c5f22bd0 100644 --- a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/check_operators.py +++ b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/check_operators.py @@ -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: @@ -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]: @@ -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]: diff --git a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/dep_operators.py b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/dep_operators.py index f4b1fe6a3b756..57655625b5be8 100644 --- a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/dep_operators.py +++ b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/dep_operators.py @@ -52,16 +52,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: @@ -105,11 +109,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]: @@ -135,11 +135,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]: diff --git a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/newly_true_operator.py b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/newly_true_operator.py index 81ba978b214a8..32f7be9cccf38 100644 --- a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/newly_true_operator.py +++ b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/newly_true_operator.py @@ -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" diff --git a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/since_operator.py b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/since_operator.py index d732aecae063c..de869baaad5ca 100644 --- a/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/since_operator.py +++ b/python_modules/dagster/dagster/_core/definitions/declarative_automation/operators/since_operator.py @@ -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" diff --git a/python_modules/dagster/dagster_tests/definitions_tests/declarative_automation_tests/automation_condition_tests/fundamentals/test_result_value_hash.py b/python_modules/dagster/dagster_tests/definitions_tests/declarative_automation_tests/automation_condition_tests/fundamentals/test_result_value_hash.py index 6a32bda5c5dfa..c3b1b2e00b265 100644 --- a/python_modules/dagster/dagster_tests/definitions_tests/declarative_automation_tests/automation_condition_tests/fundamentals/test_result_value_hash.py +++ b/python_modules/dagster/dagster_tests/definitions_tests/declarative_automation_tests/automation_condition_tests/fundamentals/test_result_value_hash.py @@ -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(