Add Configuration Object to Semantic Manifest #95

plypaul · 2023-06-23T18:48:00Z

Resolves #94

Description

Please see the linked issue.

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
This PR includes tests, or tests are not required/relevant for this PR
I have run changie new to create a changelog entry

marcodamore · 2023-06-23T19:19:03Z

tests/fixtures/semantic_manifest_yamls/simple_semantic_manifest/project_configuration.yaml

+project_configuration:
+  dsi_package_version: 1.2.3
+  time_spine_table_configurations:
+    - location: example_schema.example_table


{{project_schema}}.mf_time_spine

QMalcolm

Looking good

QMalcolm · 2023-06-23T19:46:55Z

dbt_semantic_interfaces/protocols/semantic_manifest.py

@@ -23,5 +24,10 @@ def metrics(self) -> Sequence[Metric]:  # noqa: D
    def interfaces_version(self) -> str:  # noqa: D


Is this now duplicate information? Should we remove this property in favor of dsi_package_version in project_configurations?

QMalcolm

More thoughts came to me after my initial review. We should never be expecting the user to to set the dsi_package_version. Thus we shouldn't be including it in the json schema spec. We should use a pydantic validator on the PydanticSemanticVersion implementation which parses it if a value is provided, otherwise it should populate the the current package version. The former change indicates to users that it isn't something they should directly set. The latter change allows for delineation between deserialization and new object instantiation.

QMalcolm · 2023-06-23T19:56:37Z

dbt_semantic_interfaces/parsing/schemas.py

+    "$id": "project_configuration",
+    "type": "object",
+    "properties": {
+        "dsi_package_version": {"type": "string"},


I don't think we want the user the be able to specify the dsi_package_version. This is something that should be auto-generated from the implementation classes. See the validator that sets the interfaces_version on the SemanticManifest pydantic implementation

Can you clarify how that would work? e.g. we release DSI v0.2, but the customer wrote and committed configs based on DSI v0.1. If the server is upgraded and deserializes the files, wouldn't the version be incorrect?

Deserialization and parsing logic is not the same. The JSON schema is only used for validating the yaml prior to parsing it to the object. Specifically it is invoked in the from of <object_validator>.validate(...) (example). Deserialization is the act of calling <object_type>.parse_raw(...yaml...) deserialization by itself doesn't call json schema validation. Additionally in our json schema we explicitly disallow extra keys thus if someone tries to set a dsi_package_version we want things to fail. Because deserialization doesn't do json schema validation, this is not a problem. Thus by using using a pydantic validate setter we can handle both situtiations. When going through parsing, the dsi_package_version should be None, so we auto generate it. When deserializing, the value for dsi_package_version would be included in the serialization and thus will be used.

QMalcolm · 2023-06-23T20:00:24Z

dbt_semantic_interfaces/implementations/semantic_version.py

+        """Parses a MetricInputMeasure from a string (name only) or object (struct spec) input.
+
+        For user input cases, the original YAML spec for a PydanticMetric included measure(s) specified as string names
+        or lists of string names. As such, configs pre-dating the addition of this model type will only provide the
+        base name for this object.
+        """


This doc string need some updating

QMalcolm · 2023-06-23T20:02:07Z

dbt_semantic_interfaces/implementations/semantic_version.py

+        if isinstance(input, str):
+            input_split = input.split(".")
+            if len(input_split) not in (2, 3):
+                raise ValueError(f"Expected version string to be of the form x.y or x.y.z but got {input}")
+            return PydanticSemanticVersion(
+                major_version=input_split[0],
+                minor_version=input_split[1],
+                patch_version=input_split[2] if len(input_split) == 3 else None,
+            )
+        else:
+            raise ValueError(
+                f"{cls.__name__} inputs from YAML files are expected to be of either type string or "
+                f"object (key/value pairs), but got type {type(input)} with value: {input}"
+            )


We probably want to update this to be a validator similar to

dbt-semantic-interfaces/dbt_semantic_interfaces/implementations/semantic_manifest.py

Line 26 in 739fae8

def __create_default_interfaces_version(cls, value: str) -> str: # type: ignore[misc]

A little unclear how the validator would work in the case with the custom parser. Mind elaborating?

plypaul · 2023-06-23T23:01:03Z

@QMalcolm - updated as per comments.

plypaul · 2023-06-23T23:01:41Z

NVM, left out something.

QMalcolm

Looks great! Thank you so much for all the work! I know we had to go back and forth on a few things, but it worked out great 🙂

cla-bot bot added the cla:yes label Jun 23, 2023

plypaul requested review from tlento and QMalcolm and removed request for tlento June 23, 2023 19:01

plypaul marked this pull request as ready for review June 23, 2023 19:09

marcodamore reviewed Jun 23, 2023

View reviewed changes

marcodamore requested a review from tlento June 23, 2023 19:25

QMalcolm reviewed Jun 23, 2023

View reviewed changes

QMalcolm requested changes Jun 23, 2023

View reviewed changes

plypaul force-pushed the plypaul--36--project-configuration branch from db47347 to 333469e Compare June 23, 2023 21:29

Add project configuration files.

462d0ac

plypaul force-pushed the plypaul--36--project-configuration branch from 333469e to 22218c8 Compare June 23, 2023 23:00

plypaul requested a review from QMalcolm June 23, 2023 23:00

plypaul added 3 commits June 23, 2023 16:06

Update JSON schema and parsing of project configuration files.

a9ea3d9

Update tests.

417f5ec

Add change log for #94.

5b3f00d

plypaul force-pushed the plypaul--36--project-configuration branch from 22218c8 to 5b3f00d Compare June 23, 2023 23:09

Change project_configurations -> project_configuration.

d995cb0

QMalcolm approved these changes Jun 24, 2023

View reviewed changes

plypaul merged commit f8dd217 into main Jun 26, 2023
5 checks passed

plypaul deleted the plypaul--36--project-configuration branch June 26, 2023 16:50

QMalcolm mentioned this pull request Jul 6, 2023

Add semantic manifest construct for specifying a time spine location to dbt-semantic-interfaces #86

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Configuration Object to Semantic Manifest #95

Add Configuration Object to Semantic Manifest #95

plypaul commented Jun 23, 2023 •

edited

Loading

marcodamore Jun 23, 2023

QMalcolm left a comment

QMalcolm Jun 23, 2023

QMalcolm left a comment

QMalcolm Jun 23, 2023

plypaul Jun 23, 2023

QMalcolm Jun 23, 2023

QMalcolm Jun 23, 2023

plypaul Jun 23, 2023

QMalcolm Jun 23, 2023

plypaul Jun 23, 2023

plypaul commented Jun 23, 2023

plypaul commented Jun 23, 2023

QMalcolm left a comment •

edited

Loading

		@@ -23,5 +24,10 @@ def metrics(self) -> Sequence[Metric]: # noqa: D
		def interfaces_version(self) -> str: # noqa: D

Add Configuration Object to Semantic Manifest #95

Add Configuration Object to Semantic Manifest #95

Conversation

plypaul commented Jun 23, 2023 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

QMalcolm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QMalcolm 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

plypaul commented Jun 23, 2023

plypaul commented Jun 23, 2023

QMalcolm left a comment • edited Loading

Choose a reason for hiding this comment

plypaul commented Jun 23, 2023 •

edited

Loading

QMalcolm left a comment •

edited

Loading