Create `metricflow-semantics` Package #1151

plypaul · 2024-04-24T22:00:25Z

Resolves #1150

Description

This PR carves out semantic-related modules from the metricflow package into the metricflow-semantics package.

When viewing by commit, commit messages prefixed with Move... indicate that it only contains changes that move modules / objects with associated import changes. Other commit messages indicate a potential code change.

The publish workflow will be updated in a later PR.

plypaul · 2024-04-24T23:09:47Z

metricflow-semantics/pyproject.toml

@@ -57,11 +57,12 @@ features = [

 [tool.hatch.build.targets.sdist]


Is there a reason why we use exclude vs include?

I don't know, I suspect this is a holdover from the original poetry configurations in the internal Transform repo pre-MetricFlow.

That said, I tried to switch to include when I split out the dbt dependencies and that kept producing broken builds for some reason, so I went back to exclude just to move on with my life.

Also, I don't think this applies right here right now, but there's stuff we're supposed to include in any distributions for legal reasons, and it's really easy to forget to update the packaging if we move one of those files or something.

plypaul · 2024-04-24T23:13:29Z

Heads up - need to check the packaging setup for metricflow.

tlento

Thanks for taking the effort to separate out the mechanical changes by type - apart from one auto-format things in the middle this was quite reviewable. Watching the adding and removing of the DataflowPlanBuilder and so forth was quite a ride. The end state streamlined production dependencies are a thing of beauty. I have two concerns:

I'm not convinced the production MetricFlow package will build and run correctly. I see you commented that you're working on this, so I won't approve until that is confirmed to be viable.
There's an inline about this but we have some copy/paste code held over in the prod packages. This can be dealt with in a follow-up PR.

The remaining inlines are all minor things but probably worth attending to even if it's just to say "yeah cool let's do that later." I took out all the fun ones because this PR is too big for that, it takes forever to do anything with GitHub here.

tlento · 2024-04-24T23:52:18Z

metricflow-semantics/README.md

+This repo encapsulates the modules needed for semantic resolution for MetricFlow queries.
+
+## Repo use cases
+- Resolving the dbt model dependencies of a (saved) query in MetricFlow for DAG dependency management.


Ok. We might want to make this more general. If you've got an idea now that's great otherwise we can revisit later.

No ideas at the moment.

tlento · 2024-04-24T23:52:27Z

metricflow-semantics/README.md

+
+# Welcome to metricflow-semantics
+
+This repo encapsulates the modules needed for semantic resolution for MetricFlow queries.


It's a package, not a repo, right?

tlento · 2024-04-24T23:56:56Z

metricflow-semantics/pyproject.toml

@@ -57,11 +57,12 @@ features = [

 [tool.hatch.build.targets.sdist]


I don't know, I suspect this is a holdover from the original poetry configurations in the internal Transform repo pre-MetricFlow.

That said, I tried to switch to include when I split out the dbt dependencies and that kept producing broken builds for some reason, so I went back to exclude just to move on with my life.

Also, I don't think this applies right here right now, but there's stuff we're supposed to include in any distributions for legal reasons, and it's really easy to forget to update the packaging if we move one of those files or something.

tlento · 2024-04-24T23:57:58Z

metricflow-semantics/pyproject.toml

+  "python-dateutil>=2.8.2, <2.9.0",
+  "rapidfuzz>=3.0, <4.0",
+  "ruamel.yaml>=0.17.21, <0.18.0",
+  "tabulate>=0.8.9",


Do we need tabulate? I think we only use it in tests.

Doesn't matter much, I have a todo item in my brain to go through and streamline our dependencies, so I'll deal with it then.

Moved to a dev dependency.

tlento · 2024-04-25T00:13:47Z

metricflow-semantics/metricflow_semantics/test_helpers/config_helpers.py

@@ -23,3 +24,23 @@ class MetricFlowTestConfiguration(SnapshotConfiguration):
    # is created and persisted between runs. The source schema name includes a hash of the tables that should be in
    # the schema, so
    use_persistent_source_schema: bool
+
+
+class DirectoryPathAnchor:


Writing a wrapper class around a builtin one-liner is cutting against one of Python's core design principles. Just make the constant use pathlib directly. pathlib.Path(__file__).parent.directory

Actually, I was looking to get rid of __file__ to make it easier to use. See the added commit that does this. Could be made into a function though.

I wanted to use these anchors for each of the semantic manifests. In getting the new package test configuration setup, not being able to auto-complete and relying on strings was error-prone. This is the n-th iteration that we've had to adjust this.

tlento · 2024-04-25T00:16:24Z

metricflow-semantics/metricflow_semantics/test_helpers/snapshot_helpers.py

+PLACEHOLDER_CHAR_FOR_INCOMPARABLE_STRINGS = "*"
+
+
+def make_schema_replacement_function(system_schema: str, source_schema: str) -> Callable[[str], str]:


I assume this is all just a straight copy/paste with maybe some autoformatting applied, but nothing else is changed. If that isn't correct I should probably read these files.

Yep, that's correct.

tlento · 2024-04-25T00:22:45Z

tests_metricflow/validation/test_join_validator.py

+from tests_metricflow.fixtures.manifest_fixtures import MetricFlowEngineTestFixture, SemanticManifestSetup
+
+
+def test_natural_entity_instance_set_validation(


These were all a straight copy apart from the imports and calls to the new validator, correct?

tlento · 2024-04-25T00:25:40Z

metricflow/validation/dataflow_join_validator.py

+    def _semantic_model_of_entity_in_instance_set(
+        instance_set: InstanceSet,
+        entity_reference: EntityReference,
+    ) -> SemanticModelReference:
+        """Return the semantic model where the entity was defined in the instance set."""
+        matching_instances: List[EntityInstance] = []
+        for entity_instance in instance_set.entity_instances:
+            assert len(entity_instance.defined_from) == 1
+            if len(entity_instance.spec.entity_links) == 0 and entity_instance.spec.reference == entity_reference:
+                matching_instances.append(entity_instance)
+
+        assert len(matching_instances) == 1, (
+            f"Not exactly 1 matching entity instances found: {matching_instances} for {entity_reference} in "
+            f"{mf_pformat(instance_set)}"
+        )
+        return matching_instances[0].origin_semantic_model_reference.semantic_model_reference
+
+    def is_valid_instance_set_join(
+        self,
+        left_instance_set: InstanceSet,
+        right_instance_set: InstanceSet,
+        on_entity_reference: EntityReference,
+    ) -> bool:
+        """Return true if the instance sets can be joined using the given entity."""
+        return self._join_evaluator.is_valid_semantic_model_join(
+            left_semantic_model_reference=JoinDataflowOutputValidator._semantic_model_of_entity_in_instance_set(
+                instance_set=left_instance_set, entity_reference=on_entity_reference
+            ),
+            right_semantic_model_reference=JoinDataflowOutputValidator._semantic_model_of_entity_in_instance_set(
+                instance_set=right_instance_set,
+                entity_reference=on_entity_reference,
+            ),
+            on_entity_reference=on_entity_reference,
+        )


Shouldn't we be deleting these from the SemanticModelJoinEvaluator now that they've been moved here?

Thought I deleted that - let me remove it.

tlento · 2024-04-25T00:28:21Z

metricflow-semantics/metricflow_semantics/mf_logging/pretty_print.py

@@ -36,7 +36,8 @@ def __init__(

    @staticmethod
    def _is_pydantic_base_model(obj: Any):  # type:ignore
-        return isinstance(obj, BaseModel)
+        # Checking the attribute as the BaseModel check fails for newer version of Pydantic.
+        return isinstance(obj, BaseModel) or hasattr(obj, "__config__")


That's risky but I guess it's ok in this context.

I couldn't figure out a good solution after a few minutes of searching, but yeah, I'm looking for a better solution.

tlento · 2024-04-25T00:32:15Z

.changes/unreleased/Features-20240424-145949.yaml

@@ -0,0 +1,6 @@
+kind: Features


micro-nit: I'd call this Under the Hood since that's what it is from the perspective of MetricFlow users.

tlento · 2024-04-25T00:43:46Z

One other thing from Slack - when the time comes please use a merge commit. Squash will be awful in the repo for git diff, and rebase-style will be bad because there are definitely broken commits in here and we may need to be able to use the merge commit to bisect past these or whatever at some point.

plypaul · 2024-04-25T10:19:18Z

@tlento Updated the build configuration to an include-package style. Looks good to me when I examine them.

The include-package style flattens the directory structure in the artifact.
When I build a distribution from main, the result contained a few things that we weren't supposed to (e.g. .ruff_cache), so the include-package style does help to keep it tidy more easily.
The wheel build configuration wasn't there, so that artifact was including tests. Added a section for that.
Do you know which files other than LICENSE required?

tlento

Looks great, thank you! Love the new packaging layout, it's so clean and simple and I wish I'd found that in the docs when I was digging around last month. As discussed:

When you're confident in the packaging, merge away!
Squash and merge or a merge commit is fine, just don't rebase-merge. Thank you!

plypaul added 30 commits April 24, 2024 14:45

Add missing __init__.py files in tests.

34bb6ff

Add semantics module.

86420c6

Rename errors.

f208ee1

Rename dataset.

c75352d

Rename specs.

678b993

Add missing __init__.py in inference.

e50a1e5

Move base_time_grain.py.

8929eec

Move patterns.

614f0e7

Move spec_classes.py

2b2ea34

Move specs.

8657abf

Move naming.

38f689e

Move filters.

e9e04f4

Move model.

91529ec

Move dag.

248474b

Move dataset.

c952eb0

Move dataflow plan sub-modules.

0cc46a3

Move the rest of dataflow.

4f28202

Move some files out of plan_conversion.

64cad82

Move plan_conversion.

fb53b89

Move errors.

d6cbc72

Move protocols.

02eda1b

Move query.

e1160dd

Move sql.

d80ebc3

Move time.

246b3b0

Move top level.

c7f6a04

Move collection_helpers.

298b45e

Move mf_logging.

d2cd7be

Move out time.

5de0abd

Move out sql.

be48a6d

Move out protocols/sql_client.

68875ee

plypaul added 7 commits April 24, 2024 15:36

Move semantic tests to tests_metricflow_semantics.

ec9d092

Move DunderColumnAssociationResolver to metricflow-semantics.

9b468a9

Add column_association_resolver fixture.

d201599

Add missing query_parser fixture.

ae485c0

Update pyproject.toml.

742b85b

Move tests to metricflow-semantics.

d80bf8a

Fix pretty_printing for newer Pydantic versions.

1699579

plypaul force-pushed the plypaul--11--repackage branch from 0cea41f to e626208 Compare April 24, 2024 22:43

plypaul added 5 commits April 24, 2024 15:50

Update various build-related files.

dbf35f8

Move ID-related objects to test_helpers`.

f9d3544

Move snapshots.

b853369

Add change log for #1150.

d54a4ad

Lint fixes.

4afbd09

plypaul force-pushed the plypaul--11--repackage branch from e626208 to 4afbd09 Compare April 24, 2024 22:51

plypaul commented Apr 24, 2024

View reviewed changes

tlento reviewed Apr 25, 2024

View reviewed changes

plypaul added 2 commits April 25, 2024 02:02

Address comments.

f6f64cb

Update DirectoryPathAnchor to not require __file__.

9e2c131

plypaul force-pushed the plypaul--11--repackage branch from 9aee2e6 to 1df0aca Compare April 25, 2024 17:03

tlento approved these changes Apr 25, 2024

View reviewed changes

Update / cleanup build configuration.

60773a9

plypaul force-pushed the plypaul--11--repackage branch 2 times, most recently from 80c3abd to d62f62c Compare April 25, 2024 20:32

Add package CI tests.

47d7057

plypaul force-pushed the plypaul--11--repackage branch from d62f62c to 47d7057 Compare April 26, 2024 09:59

plypaul merged commit ff139fb into main Apr 26, 2024
15 checks passed

plypaul deleted the plypaul--11--repackage branch April 26, 2024 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create `metricflow-semantics` Package #1151

Create `metricflow-semantics` Package #1151

plypaul commented Apr 24, 2024 •

edited

Loading

plypaul Apr 24, 2024

tlento Apr 24, 2024

plypaul commented Apr 24, 2024

tlento left a comment

tlento Apr 24, 2024

plypaul Apr 25, 2024

tlento Apr 24, 2024

plypaul Apr 25, 2024

tlento Apr 24, 2024

tlento Apr 24, 2024

plypaul Apr 25, 2024

tlento Apr 25, 2024

plypaul Apr 25, 2024

tlento Apr 25, 2024

plypaul Apr 25, 2024

tlento Apr 25, 2024

plypaul Apr 25, 2024

tlento Apr 25, 2024

plypaul Apr 25, 2024

tlento Apr 25, 2024

plypaul Apr 25, 2024

tlento Apr 25, 2024

plypaul Apr 25, 2024

tlento commented Apr 25, 2024

plypaul commented Apr 25, 2024

tlento left a comment

		@@ -57,11 +57,12 @@ features = [

		[tool.hatch.build.targets.sdist]


		# Welcome to metricflow-semantics

		This repo encapsulates the modules needed for semantic resolution for MetricFlow queries.

		PLACEHOLDER_CHAR_FOR_INCOMPARABLE_STRINGS = "*"


		def make_schema_replacement_function(system_schema: str, source_schema: str) -> Callable[[str], str]:

		from tests_metricflow.fixtures.manifest_fixtures import MetricFlowEngineTestFixture, SemanticManifestSetup


		def test_natural_entity_instance_set_validation(

Create metricflow-semantics Package #1151

Create metricflow-semantics Package #1151

Conversation

plypaul commented Apr 24, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plypaul commented Apr 24, 2024

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

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

tlento commented Apr 25, 2024

plypaul commented Apr 25, 2024

tlento left a comment

Choose a reason for hiding this comment

Create `metricflow-semantics` Package #1151

Create `metricflow-semantics` Package #1151

plypaul commented Apr 24, 2024 •

edited

Loading