Add AssetChecksDefinitionProps to replace untyped dictionary shenanigans #16747

schrockn · 2023-09-23T22:58:00Z

Summary & Motivation

Right in now in AssetChecksDefinition and AssetsDefinition we have a bunch of keyword arguments to construct definitions objects, and then for places where we need to make copies keep a parallel get_attributes_dict method that has to be kept in sync. Instead here we just create a tuple with the "core properties" of the definition and leverage the typing system to keep everything in sync. This mean less code, and the satisfying deletion of the comment "if adding new fields, make sure to handle them in the get_attributes_dict method" and its associated get_attributes_dict method.

This also has another key advantage. We can, as a rule, have the props object be exactly what was passed in to the origin construction, rather than anything post-processed. That means that copies will more guaranteed fidelity. We are at risk right now––especially in AssetsDefinition––of having the __init__ method process data in subtle ways that might alter behavior when passed through __init__ again when copies are made. There is an underlying assumption in that pattern that __init__ is idempotent, and it is difficult to guarantee that assumption. This pattern avoids that issue entirely.

Another nice side benefit if the elimination of AssetChecksDefinitionInputOutputProps, which was quite the mouthful. This just hoists its properties into the top-level object.

I would like to eventually use this pattern in AssetsDefinition as part of cleanup of that class. However it is easy to add earlier rather than later, so adding this to AssetChecksDefinition now before its complexity inevitably grows.

How I Tested These Changes

BK

schrockn · 2023-09-23T22:58:11Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @schrockn and the rest of your teammates on Graphite

sryza

A downside here is that a user who wants to construct an AssetChecksDefinition needs to go through the extra hoop of constructing one of these props objects.

Might it make sense to make AssetChecksDefinition itself a NamedTuple?

schrockn · 2023-09-25T15:42:58Z

Might it make sense to make AssetChecksDefinition itself a NamedTuple?

I don't think so. You want the option to add variables in __init__ or on demand.

schrockn · 2023-09-25T15:45:43Z

A downside here is that a user who wants to construct an AssetChecksDefinition needs to go through the extra hoop of constructing one of these props objects.

I agree this is a nuisance, but a fairly minor one IMO since people constructing AssetChecksDefinitions directly are likely to be framework authors.

I can take a stab at some shenanigans, but it will likely involve having an __init__ or __new__ with broken out arguments and keeping them in sync with unit tests that iterate over the types themselves.

schrockn · 2023-09-27T00:30:56Z

python_modules/dagster/dagster/_utils/test/__init__.py

@@ -330,12 +330,15 @@ def check_concurrency_claim(
        return claim_status.with_sleep_interval(float(self._sleep_interval))


-def get_all_direct_subclasses_of_marker(marker_interface_cls: Type) -> List[Type]:
-    import dagster as dagster
+def get_all_direct_subclasses_of_marker(marker_interface_cls: Type, module=None) -> List[Type]:


in progress adherence test version that does not require props in __init__ cp cp cp type as ModuleType

schrockn

@alangenfeld this is the pattern I want to institute to prevent the description shenanigans you called brutal in #16755 (review)

alangenfeld

I like the conceptual set-up of explicitly capturing the immutable props separate from memoization/utility instance members.

self._props.XXX is a little bit unfortunate but it does make it clear on every touch whats "fundamental" properties of the definition vs not

alangenfeld · 2023-10-12T16:28:03Z

...gster/dagster_tests/definitions_tests/test_i_definition_prop_class_matches_init_adherence.py

+    hard_coded_list = [
+        (AssetChecksDefinition, AssetChecksDefinitionProps),
+    ]
+
+    # keep hard_coded_list up-to-date with all subclasses of IDefinitionPropsClassMatchesInit
+    assert set([def_type.__name__ for def_type in def_types]) == set(
+        [entry[0].__name__ for entry in hard_coded_list]
+    ), (
+        "You likely added a subclass of IDefinitionPropsClassMatchesInit without adding it to the"
+        " hard_coded_list"
+    )


this is just here because you couldn't figure out how to pluck TPropsType & TDefType from the class object?

alangenfeld · 2023-10-12T16:29:26Z

python_modules/dagster/dagster/_utils/test/__init__.py

+    marker_interface_cls: Type, module: Optional[ModuleType] = None
+) -> List[Type]:
+    """Get all direct subclasses of a given marker interface class from a given module. If the
+    module is not specified, it defaults to the dagster module.
+    """
+    if module is None:
+        import dagster as dagster
+
+        module = dagster


what motivated this change?

alangenfeld · 2023-10-12T16:38:11Z

python_modules/dagster/dagster/_core/definitions/asset_checks.py

-@experimental
-class AssetChecksDefinitionInputOutputProps(NamedTuple):
+
+class IDefinitionPropsClassMatchesInit(Generic[TPropsType, TDefType], ABC):


nit: something in the direction of IDefinitionWithProps / PropsBackedDefinition, i think it makes more sense to target the fundamentlal pattern then a specific thing we enforce when participating in it

alangenfeld · 2023-10-12T16:38:39Z

python_modules/dagster/dagster/_core/definitions/asset_checks.py

+        return self.__class__(
+            **self._props._replace(
+                resource_defs=merge_resource_defs(
+                    old_resource_defs=self._props.resource_defs,
+                    resource_defs_to_merge_in=resource_defs,
+                    requires_resources=self,
+                )
+            )._asdict()


alangenfeld · 2023-10-12T16:39:10Z

python_modules/dagster/dagster/_core/definitions/asset_checks.py

+    @staticmethod
+    def from_props(props: TPropsType) -> TDefType: ...


this appears unused - should we instead capture [1] with a method on this class?

schrockn requested review from johannkm and sryza September 23, 2023 23:07

schrockn marked this pull request as ready for review September 24, 2023 00:40

sryza reviewed Sep 25, 2023

View reviewed changes

schrockn changed the base branch from master to reflection-helper September 26, 2023 14:08

schrockn force-pushed the asset-checks-definition-props branch from fab5c9a to 55e1992 Compare September 26, 2023 14:08

schrockn mentioned this pull request Sep 26, 2023

Extract helper function to find all subclasses of a marker interface #16793

Merged

schrockn commented Sep 27, 2023

View reviewed changes

Base automatically changed from reflection-helper to master September 27, 2023 02:04

Add AssetChecksDefinitionProps to replace untyped dictionary shenanigans

eade43c

in progress adherence test version that does not require props in __init__ cp cp cp type as ModuleType

schrockn force-pushed the asset-checks-definition-props branch from 6af1750 to eade43c Compare September 27, 2023 02:17

schrockn requested review from rexledesma, sryza and alangenfeld September 27, 2023 02:19

schrockn commented Sep 27, 2023

View reviewed changes

alangenfeld requested changes Oct 12, 2023

View reviewed changes

schrockn closed this Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AssetChecksDefinitionProps to replace untyped dictionary shenanigans #16747

Add AssetChecksDefinitionProps to replace untyped dictionary shenanigans #16747

schrockn commented Sep 23, 2023 •

edited

Loading

schrockn commented Sep 23, 2023 •

edited

Loading

sryza left a comment

schrockn commented Sep 25, 2023

schrockn commented Sep 25, 2023

schrockn Sep 27, 2023

schrockn left a comment

alangenfeld left a comment

alangenfeld Oct 12, 2023

alangenfeld Oct 12, 2023

alangenfeld Oct 12, 2023

alangenfeld Oct 12, 2023

alangenfeld Oct 12, 2023

		@staticmethod
		def from_props(props: TPropsType) -> TDefType: ...

Add AssetChecksDefinitionProps to replace untyped dictionary shenanigans #16747

Add AssetChecksDefinitionProps to replace untyped dictionary shenanigans #16747

Conversation

schrockn commented Sep 23, 2023 • edited Loading

Summary & Motivation

How I Tested These Changes

schrockn commented Sep 23, 2023 • edited Loading

sryza left a comment

Choose a reason for hiding this comment

schrockn commented Sep 25, 2023

schrockn commented Sep 25, 2023

schrockn Sep 27, 2023

Choose a reason for hiding this comment

schrockn left a comment

Choose a reason for hiding this comment

alangenfeld left a comment

Choose a reason for hiding this comment

alangenfeld Oct 12, 2023

Choose a reason for hiding this comment

alangenfeld Oct 12, 2023

Choose a reason for hiding this comment

alangenfeld Oct 12, 2023

Choose a reason for hiding this comment

alangenfeld Oct 12, 2023

Choose a reason for hiding this comment

alangenfeld Oct 12, 2023

Choose a reason for hiding this comment

schrockn commented Sep 23, 2023 •

edited

Loading

schrockn commented Sep 23, 2023 •

edited

Loading