Prep DataModel for removal by NickCrews · Pull Request #1102 · dedupeio/dedupe

NickCrews · 2022-09-23T21:32:45Z

Split off from #1098

This makes it easier to create Variable instances from variable definitions, because Variables are responsible for both predicates and distance functions. So, if we split DataModel into these two separate parts, we're going to need to do something like

# In eg Dedupe or RecordLink 
vars = typify_variables(var_defs)
predicates = [v.predicates() for v in vars]
featurizer = Featurizer(vars)

so it would be important that we can create our variable instances once, and then pass those around to the other parts.

It isn't used publicly anywhere, and it adds more API surface area to worry about. This is still backwards compatible, as self._len is still saved/restored byt pickle the same way, now we just access it directly instead of going via __len__().

We iterate through the input more than once, so an Iterable is insufficient.

Before, we never checked for this, so when we called list.index() we just got back the first instance. I swapped us over to a lookup table because that makes more sense, but I wanted to also add this check because if someone had duplicate names, this would quitely change the behavior underneath someone: before it gave the first index, now it gives the last.

These variables 1. you can't currently create them because they inherit from Variable, not FieldVariable, so they don't appear in the datamodel.VARIABLE_CLASSES lookup table 2. You SHOULDN'T be able to instantiate these variables directly from a variable definition

Before the flow was: 1. create all the primary variables EXCEPT for the interactions 2. Expand those. 3. go back through and NOW create the InteractionVariables I found this confusing. We parse the var definitions twice in separate places, and InteractionVariables are a special case in the first pass. The other point of this is to make it so that there is one list of Variable instances which is the single source of truth. Once we do this then we can turn all the other private instance variables of DataModel into @functools.cached_property's, which will make it much easier to ensure pickle compatibility in the future, because only one variable needs to get saved and restored

NickCrews · 2022-09-23T21:37:45Z

dedupe/datamodel.py

-            all_variables.append(variable_object)
-
-    if only_custom:
+    no_blocking_variables = all(


Continuing from #1098 (comment)

I think it would be better if this was more duck-typed though, such as
no_blocking_variables = not any(len(v.predicates) for v in variables).

That would be a slight change though, since Variable.predicates is filled with an ExistsPredicate for any var def with the has missing flag. Perhaps this is OK? But I don't think so.

I think really there needs to be a more official API where variables declare the predicates they export.

the code that add the ExistsPredicate should only really be on FieldPredicates as it depends on there being a field.

https://github.com/dedupeio/dedupe/blob/main/dedupe/variables/base.py#L37

if we move that code, then we could do pretty much what you are saying.

no_blocking_variables = not any(len(v.predicates) for v in variables)

we might also want to have a check like

only_exists = all(isinstance(pred, ExistsPredicate) for v in variables for pred in v.predicates) if only_exists: warning.warn('You are only using the Exists Predicate which is usually pretty bad')

coveralls · 2022-09-23T21:38:04Z

Coverage increased (+0.03%) to 73.315% when pulling 5482c21 on NickCrews:datamodel-prep-for-removal into 522e7b2 on dedupeio:main.

NickCrews · 2022-09-23T21:46:01Z

dedupe/datamodel.py

-        all_variables += interactions(variable_definitions, self.primary_variables)
+        variables = typify_variables(variable_definitions)
+        non_interactions: list[FieldVariable] = [
+            v for v in variables if not isinstance(v, InteractionType)  # type: ignore[misc]


This is another case where I think a Variable should have a more official API for if they export if they are an Interaction or not. Maybe should think of a better name for this too. Atomic vs derived? leaf vs nonleaf? base vs derived?

NickCrews · 2022-09-23T21:46:41Z

dedupe/datamodel.py

+    no_blocking_variables = all(
+        isinstance(v, (CustomType, InteractionType)) for v in variables
+    )
+    if no_blocking_variables:


Perhaps this check should happen in DataModel.init, and not in this utility function?

it seems of a piece with the "Incorrect variable specification: variable" error?

NickCrews · 2022-09-23T21:47:42Z

dedupe/datamodel.py

-    var_d = {var.name: var for var in primary_variables}
-
+def _expanded_interactions(variables: list[Variable]) -> list[InteractionType]:
+    field_vars = {var.name: var for var in variables if isinstance(var, FieldVariable)}


Again, this should feels like it needs a more official API, or should be responsibility of the InteractionType

i can think of a few designs here, but this code is actually doing two things for us, both of which are kind of important.

isolating the interaction features for us, so we can put them in the right place in the sequence of features.

actually resolving the feature indices.

seems like we kind of need to do both.

fgregg · 2022-09-24T00:00:41Z

dedupe/datamodel.py

-            all_variables.append(variable_object)
-
-    if only_custom:
+    no_blocking_variables = all(


the code that add the ExistsPredicate should only really be on FieldPredicates as it depends on there being a field.

https://github.com/dedupeio/dedupe/blob/main/dedupe/variables/base.py#L37

if we move that code, then we could do pretty much what you are saying.

no_blocking_variables = not any(len(v.predicates) for v in variables)

we might also want to have a check like

only_exists = all(isinstance(pred, ExistsPredicate) for v in variables for pred in v.predicates) if only_exists: warning.warn('You are only using the Exists Predicate which is usually pretty bad')

fgregg · 2022-09-24T00:08:25Z

dedupe/datamodel.py

+    no_blocking_variables = all(
+        isinstance(v, (CustomType, InteractionType)) for v in variables
+    )
+    if no_blocking_variables:


it seems of a piece with the "Incorrect variable specification: variable" error?

fgregg · 2022-09-24T00:35:43Z

dedupe/datamodel.py

+            result.extend(variable.higher_vars)
+        else:
+            result.append(variable)
+    return result


what if we added an iter method to Variable so that we could just do something like.

features = [] for variable in variables: for feature in variable: features.append(feature)

I think this is a good idea in general, but I would want to name it to differentiate between predicates and features, since Variables supply both. Perhaps Variable.features?

fgregg · 2022-09-24T00:39:38Z

dedupe/datamodel.py

-    var_d = {var.name: var for var in primary_variables}
-
+def _expanded_interactions(variables: list[Variable]) -> list[InteractionType]:
+    field_vars = {var.name: var for var in variables if isinstance(var, FieldVariable)}


i can think of a few designs here, but this code is actually doing two things for us, both of which are kind of important.

isolating the interaction features for us, so we can put them in the right place in the sequence of features.

actually resolving the feature indices.

seems like we kind of need to do both.

fgregg · 2022-09-24T00:40:45Z

dedupe/variables/base.py



 class DerivedType(Variable):
-    type = "Derived"


why did you remove this?

I might be wrong but my understanding is:

This would only be used if someone passes in a variable definition like {"type": "Derived"}, and then we would create an instance of DerivedType, but that doesn't make sense because DerivedType is an abstract base class and shouldn't ever be created directly.

fgregg · 2022-09-24T00:40:52Z

dedupe/variables/base.py



 class MissingDataType(Variable):
-    type = "MissingData"


why did you remove this?

Same reason as for DerivedType

fgregg · 2022-09-24T00:45:43Z

thanks for working through this @NickCrews. this is some of the first code i wrote for project and there's a lot i didn't know!

NickCrews added 11 commits September 23, 2022 13:10

ref: remove __len__ from DataModel

22c54bb

It isn't used publicly anywhere, and it adds more API surface area to worry about. This is still backwards compatible, as self._len is still saved/restored byt pickle the same way, now we just access it directly instead of going via __len__().

ref: make typifying variables more clear

9a34c00

ref: tweak error raising in typify_variables()

a6504d5

ref: Clarify only_custom logic in typify_variables

4871eae

typ: Fix typing of typify_variables()

8dc4b80

We iterate through the input more than once, so an Iterable is insufficient.

ref: Further rename field to variable in datamodel

442a65a

test: Add more tests for interaction variables

4eb1aa1

ref: Move check for empty var def into typify_variables

5482c21

NickCrews commented Sep 23, 2022

View reviewed changes

NickCrews mentioned this pull request Sep 23, 2022

Reduce responsibilities of DataModel #1098

Closed

NickCrews commented Sep 23, 2022

View reviewed changes

fgregg requested changes Sep 24, 2022

View reviewed changes

NickCrews mentioned this pull request Sep 24, 2022

Can we overhaul internals of Variables #1104

Open

fgregg mentioned this pull request Jun 21, 2024

more direct set up data model, tests passing #1193

Merged

6 tasks

Conversation

NickCrews commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fgregg commented Sep 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NickCrews commented Sep 23, 2022 •

edited

Loading