Add `Mergeable` to Simplify Merging Collections #790

plypaul · 2023-09-28T17:55:49Z

Description

This PR adds the utility class MergeBuilder that allows you to combine collections that implements Mergeable. This is useful as there are a few spec-related classes that are collections, and there are upcoming cases where they need to be merged. This replaces the collection of merge-related methods in the spec set classes.

This PR also includes a minor update to the signature of InstanceSpec to make it more flexible.

github-actions · 2023-09-28T17:56:03Z

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

tlento

In a vacuum where all implementation choices are reasonable and correct this all seems fine. However, we don't operate in such a vacuum, and there are a lot of "but WHY is it like this?" implementation choices in here. Every single one of them is preceded by a #noqa: D override on docstrings, and every commit message is completely devoid of the crucial "why" for each change.

Please write docstrings explaining the context behind these decisions, and then I'll be able to evaluate this better. I'm sure it's fine, but I also know that I'm going to screw up using this and when I go through the code (and commit history) I'll be quite irritated to find that nothing has been explained anywhere.

tlento · 2023-09-29T23:12:01Z

metricflow/specs/merge_builder.py

+from abc import ABC, abstractmethod
+from typing import Generic, Iterable, TypeVar
+
+SelfTypeT = TypeVar("SelfTypeT", bound="Mergeable")


from typing_extensions import Self is a thing. Let's use that instead.

Didn't realize that was a thing - updated.

tlento · 2023-09-29T23:18:44Z

metricflow/specs/merge_builder.py

+        self._accumulator = initial_item
+
+    def add(self, other: MergeableT) -> None:  # noqa: D
+        if self._accumulator is None:


Do we need this? Can the generic actually be Optional[MergeableThing]? I don't think it can, with a bound of "Mergeable" here.

I ran into some issues there, but this is no longer needed as this class was removed.

tlento · 2023-09-29T23:21:42Z

metricflow/specs/merge_builder.py

+
+        self._accumulator = self._accumulator.merge(other)
+
+    def add_all(self, *others: MergeableT) -> None:  # noqa: D


Ok. I realize this is my not-Python developer side showing, but I prefer others: Sequence[...] to *others.

Given add_iterable is a thing, do we really need this method? If they can't be combined, some docstrings with explanations providing that context would be nice.

No longer relevant as this class was removed.

tlento · 2023-09-29T23:23:52Z

metricflow/specs/merge_builder.py

+        return builder.build_result
+
+    @property
+    def build_result(self) -> MergeableT:  # noqa: D


This sounds like an action rather than a property. Maybe merged_result or merged_output would be better?

No longer relevant as this class was removed.

tlento · 2023-09-29T23:31:39Z

metricflow/test/specs/test_merge_builder.py

+
+
+def test_merge_iterable() -> None:  # noqa: D
+    assert MergeBuilder.merge_iterable(NumberList(), [NumberList([1, 2, 3])]) == NumberList([1, 2, 3])


You might want a test that merges a non-empty thing as well, because return other_iterable.next() passes this test.

Added a test with a non-empty item, but not sure what you mean by return other_iterable.next()

tlento · 2023-09-29T23:40:36Z

metricflow/specs/specs.py

+            entity_specs=self.entity_specs + other.entity_specs,
+        )
+
+    def dedupe(self) -> LinkableSpecSet:  # noqa: D


Why are we separating these? Is there ever a case where we want duplicates in this output?

If not, this feels like we're adding an old bug factory back to the codebase.

Preferring to make dedupe() a separate method as it makes the Mergeable interface more flexible. I disagree on adding an old bug factory back to the codebase. If there is always de-duplication behavior, it could hide bugs in testing. e.g. the output of a merged result is checked in a test, and it's not expected for the sources of the to produce duplicate items. However, the de-duplication of the merged result hides that.

The first two words (if not) in that bug factory sentence are extremely important. If we never want duplicates to come out of a merge, then any case where we have them is a bug, and requiring developers to manually call a dedupe method can only lead to bugs, while encapsulating that behavior can only prevent them. If there is an upstream bug that the encapsulation hides that's a separate problem, and it's one that having developers blindly call dedupe() on their merge outputs (which is a thing we should expect to happen if our contributor base grows) will also mask.

As it stands right now every merge call either explicitly requests deduped results or implicitly resolves to them, which suggests that we shouldn't force developers to call it in order to get the behavior they need.

Ultimately, this isn't a big deal, but it's interesting to me that the only extant cases where we don't call dedupe() are still cases where we need deduplicated results.

tlento · 2023-09-29T23:43:03Z

metricflow/specs/specs.py

@@ -594,6 +594,37 @@ def linkable_specs(self) -> LinkableSpecSet:  # noqa: D
            entity_specs=self.entity_specs,
        )

+    @override
+    def merge(self, other: MetricFlowQuerySpec) -> MetricFlowQuerySpec:  # noqa: D
+        # Can't merge if both properties are set.


Wait, why can we merge when these don't match at all?

Can you please add a docstring explaining this?

Context on the original reason for this change was that queries for derived metrics could be thought of as a composition of queries on separate metrics during query resolution. However, this is no longer needed due to a different approach that was taken in a later revision.

plypaul

Updated the commit messages and docstrings, and found a fix for type issues to allow removal of the MergeBuilder and just have the Mergeable interface.

tlento

LGTM apart from that last commit and the hanging If in the docstring.

You built the thing so you get to choose whether to opt for more flexibility here.

tlento · 2023-11-15T19:11:29Z

metricflow/specs/specs.py

+            entity_specs=self.entity_specs + other.entity_specs,
+        )
+
+    def dedupe(self) -> LinkableSpecSet:  # noqa: D


The first two words (if not) in that bug factory sentence are extremely important. If we never want duplicates to come out of a merge, then any case where we have them is a bug, and requiring developers to manually call a dedupe method can only lead to bugs, while encapsulating that behavior can only prevent them. If there is an upstream bug that the encapsulation hides that's a separate problem, and it's one that having developers blindly call dedupe() on their merge outputs (which is a thing we should expect to happen if our contributor base grows) will also mask.

As it stands right now every merge call either explicitly requests deduped results or implicitly resolves to them, which suggests that we shouldn't force developers to call it in order to get the behavior they need.

Ultimately, this isn't a big deal, but it's interesting to me that the only extant cases where we don't call dedupe() are still cases where we need deduplicated results.

tlento · 2023-11-15T19:13:07Z

mypy.ini

+
+# Added to resolve an issue with mypy in CI:
+# ... python3.8/site-packages/referencing/jsonschema.py:426: error: Invalid "type: ignore" comment  [syntax]
+[mypy-referencing.jsonschema]
+follow_imports = skip


This was fixed by updating mypy, please revert this commit.

tlento · 2023-11-15T19:27:28Z

metricflow/collections/merger.py

+    def merge_iterable(cls: Type[MergeableT], items: Iterable[MergeableT]) -> MergeableT:
+        """Merge all items into a single instance.
+
+        If


If what? 😛

Merging objects are frequently needed in MetricFlow as there are several recursive operations where the output is the superset of the result of the recursive calls. Please see the docstring for Mergeable.

InstanceSpecSet already has a merge() call, so this updates the class to imlement the Mergeable interface for consistency.

LinkableSpecSet already has a merge() call, so this updates the class to imlement the Mergeable interface for consistency.

cla-bot bot added the cla:yes label Sep 28, 2023

plypaul changed the title ~~Add Mergeable / MergeBuilder.~~ Add Mergeable / MergeBuilder to Simplify Merging Collections Sep 28, 2023

plypaul added the Skip Changelog label Sep 28, 2023

plypaul marked this pull request as ready for review September 28, 2023 18:32

plypaul requested a review from tlento September 28, 2023 18:32

tlento reviewed Sep 29, 2023

View reviewed changes

Base automatically changed from plypaul--58.1--singular-spec-field-for-order-by to main October 5, 2023 20:15

plypaul force-pushed the plypaul--58.2--add-mergable branch from 26d1c26 to 7352a38 Compare November 14, 2023 20:19

plypaul changed the title ~~Add Mergeable / MergeBuilder to Simplify Merging Collections~~ Add Mergeable to Simplify Merging Collections Nov 14, 2023

plypaul force-pushed the plypaul--58.2--add-mergable branch from 7352a38 to 2799e2f Compare November 14, 2023 20:23

plypaul commented Nov 14, 2023

View reviewed changes

plypaul force-pushed the plypaul--58.2--add-mergable branch from 2799e2f to 809c5c6 Compare November 15, 2023 02:11

plypaul requested a review from tlento November 15, 2023 05:57

tlento approved these changes Nov 15, 2023

View reviewed changes

plypaul added 3 commits November 15, 2023 19:53

Add Mergeable to streamline collections merging.

7c69e0d

Merging objects are frequently needed in MetricFlow as there are several recursive operations where the output is the superset of the result of the recursive calls. Please see the docstring for Mergeable.

Implement Mergeable for InstanceSpecSet.

f8c2b22

InstanceSpecSet already has a merge() call, so this updates the class to imlement the Mergeable interface for consistency.

Implement Mergeable for LinkableSpecSet.

086cee4

LinkableSpecSet already has a merge() call, so this updates the class to imlement the Mergeable interface for consistency.

plypaul force-pushed the plypaul--58.2--add-mergable branch from 809c5c6 to 086cee4 Compare November 16, 2023 03:53

plypaul merged commit 50cd52d into main Nov 16, 2023
9 checks passed

plypaul deleted the plypaul--58.2--add-mergable branch November 16, 2023 04:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Mergeable` to Simplify Merging Collections #790

Add `Mergeable` to Simplify Merging Collections #790

plypaul commented Sep 28, 2023 •

edited

Loading

github-actions bot commented Sep 28, 2023

tlento left a comment

tlento Sep 29, 2023

plypaul Nov 14, 2023

tlento Sep 29, 2023

plypaul Nov 14, 2023

tlento Sep 29, 2023

plypaul Nov 14, 2023

tlento Sep 29, 2023

plypaul Nov 14, 2023

tlento Sep 29, 2023

plypaul Nov 14, 2023

tlento Sep 29, 2023

plypaul Nov 14, 2023

tlento Nov 15, 2023

tlento Sep 29, 2023

plypaul Nov 14, 2023

plypaul left a comment

tlento left a comment

tlento Nov 15, 2023

tlento Nov 15, 2023

plypaul Nov 16, 2023

tlento Nov 15, 2023

plypaul Nov 16, 2023


		self._accumulator = self._accumulator.merge(other)

		def add_all(self, *others: MergeableT) -> None: # noqa: D



		def test_merge_iterable() -> None: # noqa: D
		assert MergeBuilder.merge_iterable(NumberList(), [NumberList([1, 2, 3])]) == NumberList([1, 2, 3])

Add Mergeable to Simplify Merging Collections #790

Add Mergeable to Simplify Merging Collections #790

Conversation

plypaul commented Sep 28, 2023 • edited Loading

Description

github-actions bot commented Sep 28, 2023

tlento left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plypaul left a comment

Choose a reason for hiding this comment

tlento left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `Mergeable` to Simplify Merging Collections #790

Add `Mergeable` to Simplify Merging Collections #790

plypaul commented Sep 28, 2023 •

edited

Loading