Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CT-2677] [BUG] Update SemanticModel definitions to match DSI 0.1.0dev3 protocols #7833

Closed
Tracked by #7498
QMalcolm opened this issue Jun 9, 2023 · 2 comments · Fixed by #7848
Closed
Tracked by #7498

[CT-2677] [BUG] Update SemanticModel definitions to match DSI 0.1.0dev3 protocols #7833

QMalcolm opened this issue Jun 9, 2023 · 2 comments · Fixed by #7848
Assignees
Labels
bug Something isn't working MetricFlow

Comments

@QMalcolm
Copy link
Contributor

QMalcolm commented Jun 9, 2023

In #7769 we added SemanticModel nodes in core. We started this work while pointed at DSI 0.1.0dev2 but midway updated to depend on DSI 0.1.0dev3. The main problem is that there are some protocol changes from DSI dev2 to DSI dev3, specifically SemanticModel.dimensions[x].is_primary has moved to SemanticModel.defaults.agg_time_dimension. Due to our serialization process, the semantic manifest artifact contains neither is_primary or agg_time_dimension 🙃 This issue was able to slip through because we don't check currently in core that our node implementations satisfy the protocol. Thus in solving this issue we should not only correct the nodes, but also add tests to verify that our nodes satisfy the DSI protocols.

@QMalcolm QMalcolm self-assigned this Jun 9, 2023
@github-actions github-actions bot changed the title [BUG] Update SemanticModel definitions to match DSI 0.1.0dev3 protocols [CT-2677] [BUG] Update SemanticModel definitions to match DSI 0.1.0dev3 protocols Jun 9, 2023
@QMalcolm QMalcolm added MetricFlow bug Something isn't working labels Jun 9, 2023
@QMalcolm QMalcolm added this to the v1.6 milestone Jun 9, 2023
@emmyoop
Copy link
Member

emmyoop commented Jun 12, 2023

duplicates #7827

@QMalcolm
Copy link
Contributor Author

duplicates #7827

I'm not sure this is so much a duplicate as it is just simply related. #7827 brings up that we need a way to ensure the nodes satisfy the protocol. This issue is specifically about fixing a bug which is that one of our nodes doesn't currently satisfy the protocol.

@jtcohen6 jtcohen6 removed this from the v1.6 milestone Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working MetricFlow
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants