Move SemanticModel data artifacts to dbt/artifacts
#9485
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
QMalcolm
changed the title
SemanticModel data artifacts to dbt/artifacts
QMalcolm
force-pushed
the
qmalcolm--9387-move-semantic-model-to-dbt-artifacts
branch
from
44405cf to
0365498
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #9485 +/- ##
==========================================
- Coverage 87.96% 87.95% -0.01%
==========================================
Files 166 166
Lines 22094 22119 +25
==========================================
+ Hits 19435 19455 +20
- Misses 2659 2664 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
QMalcolm
commented
Jan 30, 2024
|
|
||
|
|
||
| @dataclass | ||
| class SemanticModel(GraphResource): |
There was a problem hiding this comment.
This class has a fair number of @property methods, and even a method that isn't @property'd. Felt kind of weird moving them all, however they are expected parts of the dbt-semantic-interfaces protocol specification for SemanticModel. I would like to reduce some of these in the protocol itself, but for now keeping them together in this implementation seems right-ish
MichelleArk
requested review from
MichelleArk
and removed request for
martynydbt
MichelleArk
approved these changes
Jan 30, 2024
|
Let's also do the satisfying thing and remove the dbt.contracts.graph.nodes in v12/manifest.py in favor of dbt.artifacts.resources. Presumably at this point we can do this for SavedQuery, Metric, and SemanticModel Similar to this commit: cdfdb7a |
…tric`, and `SemanticModel`
|
Mypy keeps finding errors in GHA that it's not raising locally on pre-commit hooks 🙃 |
…tor search In the `search` method in `selector_methods.py`, we were getting object representations from the incoming writable manifest by unique id. What we get from the writable manifest though is increasingly the `resource` (data artifact) part of the node, not the full node. This was problematic because a number of the selector processes _compare_ the old node to the new node, but the `resource` representation doesn't have the comparator methods. In this commit we dict-ify the resource and then get the full node by undictifying that. We should probably have a better built in process to the full node objects to do this, but this will do for now.
…node conversion We want to easily be able to create nodes from their resource counter parts. It's actually imperative that we can do so. The previous commit had a manual way to do so where needed. However, we don't want to have to put `from_dict(.to_dict())` everywhere. So here we hadded a `from_resource` class method to `BaseNode`. Everything that inherits from `BaseNode` thus automatically gets this functionality. HOWEVER, the implementation currently has a problem. Specifically, the type for `resource_instance` is `BaseResource`. Which means if one is calling say `Metric.from_resource()`, one could hand it a `SemanticModelResource` and mypy won't complain. In this case, a semi-cryptic error might get raised at runtime. Whether or not an error gets raised depends entirely on whether or not the dictified resource instance manages to satisfy all the required attributes of the desired node class. THIS IS VERY BAD. We should be able to solve this issue in an upcoming (hopefully next) commit, wherein we genericize `BaseNode` such that when inheriting it you declare it with a resource type. Technically a runtime error will still be possible, however any mixups should be caught by mypy on pre-commit hooks as well as PRs.
Turning `BaseNode` into an ABC generic allows us to say that the inheriting class can define what resource type from artifacts it should be used with. This gives us added type safety to what resource type can be passed into `from_resource` when called via `SemanticModel.from_resource(...)`, `Metric.from_resource(...)`, and etc. NOTE: This only gives us type safety from mypy. If we begin ignoring mypy errors during development, we can still get into a situation for runtime errors (it's just harder to do so now).
MichelleArk
approved these changes
Feb 1, 2024
QMalcolm
deleted the
qmalcolm--9387-move-semantic-model-to-dbt-artifacts
branch
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #9387
Problem
We are moving data artifacts to
dbt/artifactsin a piecewise fashion. We needed to moveSemanticModelas part of that.Solution
Moved data portion of
SemanticModelto dbt/artifacts (and all other classes that doing so required)Checklist