Finalize validations for custom granularities #370
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
courtneyholcomb
requested review from
QMalcolm,
graciegoheen and
a team
as code owners
courtneyholcomb
requested review from
aiguofer,
DevonFulcher and
theyostalservice
dbt_semantic_interfaces/transformations/cumulative_type_params.py
Outdated
Show resolved
Hide resolved
…t offset_window We decided to cut scope on custom calendar in the meantime and only enable it for offset_windows. These validations will make that an easy user experience, while still allowing us to easily remove them once we build support.
courtneyholcomb
force-pushed
the
court/case-sens
branch
from
693783c to
825622c
Compare
courtneyholcomb
changed the title
grain_to_date and offset_to_grain
WilliamDee
reviewed
Nov 27, 2024
| """Adds custom parsing to the default method.""" | ||
| data = deepcopy(input) | ||
|
|
||
| # Ensure grain_to_date is lowercased |
There was a problem hiding this comment.
Could we lowercase window here too?
There was a problem hiding this comment.
We could but I think the way it's implemented now is less hacky! This one feels less optimal because we have to go through these dict keys that are not typed. But I couldn't use the same strategy as we did for window because grain is just a string, not an object.
| expected_substrings = [ | ||
| "Invalid time granularity", | ||
| "Both window and grain_to_date set for cumulative metric. Please set one or the other.", | ||
| "Got differing values for `window`", | ||
| "Invalid time granularity 'martian_week' in window: '3 martian_weeks'", | ||
| "Invalid time granularity 'martian_week' in window: '3 martian_week'", | ||
| "Invalid time granularity 'martian_week' in window: '5 martian_week'", |
There was a problem hiding this comment.
Do we have tests for successful custom grain parsing now that these fails?
There was a problem hiding this comment.
Yes on line 339! That one is successful.
WilliamDee
approved these changes
Dec 2, 2024
theyostalservice
approved these changes
Dec 2, 2024
| @@ -304,7 +304,7 @@ def test_derived_metric() -> None: # noqa: D | |||
| metrics=[ | |||
| PydanticMetricInput( | |||
| name="random_metric", | |||
| offset_window=PydanticMetricTimeWindow.parse("3 weeks", custom_granularity_names=()), | |||
| offset_window=PydanticMetricTimeWindow.parse("3 weekies"), | |||
| @validate_safely(whats_being_done="running model validation ensuring cumulative metrics are valid") | ||
| def validate_manifest(semantic_manifest: SemanticManifestT) -> Sequence[ValidationIssue]: # noqa: D | ||
| def validate_manifest(cls, semantic_manifest: SemanticManifestT) -> Sequence[ValidationIssue]: # noqa: D | ||
| issues: List[ValidationIssue] = [] | ||
|
|
||
| custom_granularity_names = [ |
There was a problem hiding this comment.
nit / premature optimization - why not make this a set here, rather than making it a set inside validate_metric_time_window, which is called every iteration of the for loop in this function?
In the process of writing the prior commit, I discovered that type checking was being erased for function parameters if this decorator was used. This commit fixes that.
…n revealed some type errors. This commit fixes those revealed type errors.
theyostalservice
approved these changes
Dec 2, 2024
| @@ -28,7 +25,7 @@ class ElementConsistencyRule(SemanticManifestValidationRule[SemanticManifestT], | |||
|
|
|||
| @staticmethod | |||
| @validate_safely(whats_being_done="running model validation ensuring model wide element consistency") | |||
| def validate_manifest(semantic_manifest: PydanticSemanticManifest) -> Sequence[ValidationIssue]: # noqa: D | |||
| def validate_manifest(semantic_manifest: SemanticManifestT) -> Sequence[ValidationIssue]: # noqa: D | |||
| @@ -120,8 +120,10 @@ def validate_manifest(cls, semantic_manifest: SemanticManifestT) -> Sequence[Val | |||
| ) | |||
|
|
|||
| if window: | |||
| issues += cls.validate_metric_time_window( | |||
| metric_context=metric_context, window=window, custom_granularities=custom_granularity_names | |||
| issues.extend( | |||
There was a problem hiding this comment.
I don't know why the typechecker would accept one of these and not the other, but tbh, I prefer extend here anyway so I'm not going to complain :P
There was a problem hiding this comment.
you can't do list += sequence but you CAN pass a sequence into extend()!
| @functools.wraps(func) | ||
| def wrapper(*args: Any, **kwargs: Any) -> List[ValidationIssue]: # type: ignore | ||
| def wrapper(*args: P.args, **kwargs: P.kwargs) -> Sequence[ValidationIssue]: # type: ignore |
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.
Description
This PR does 2 things:
grain_to_dateandoffset_to_grainfields case sensitive. This needs to be fixed before we can release to core. This PR fixes that.offset_window. This is due to a product decision we made to prioritize that feature and defer the rest.Checklist
changie newto create a changelog entry