-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[FEATURE] ValidationConfigStore
#9523
Changes from all commits
44172f8
2b92cd4
008bd50
31aafb3
be70160
3e9a833
295eba0
fe3ebfe
ddddc8d
9fa6eba
0997f8b
d62cd3b
8f088d6
3771806
764ad93
537def9
3eba6a4
61a1c23
76700d8
dffbbe1
b7d1072
a216c92
2c766f9
07acaea
f137989
8a948fb
ec7c5f8
947d901
72bdf51
6882737
deb1076
695cc17
182edab
a65ce0b
c533188
f987bef
22e5d55
1c256ca
a82f189
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,14 @@ | ||
from __future__ import annotations | ||
|
||
from typing import TYPE_CHECKING | ||
from typing import Union | ||
|
||
from great_expectations._docs_decorators import public_api | ||
from great_expectations.compatibility.pydantic import BaseModel | ||
|
||
if TYPE_CHECKING: | ||
from great_expectations.core.batch_config import BatchConfig | ||
from great_expectations.core.expectation_suite import ExpectationSuite | ||
|
||
# from great_expectations.datasource.fluent.interfaces import DataAsset | ||
from great_expectations.compatibility.pydantic import BaseModel, validator | ||
from great_expectations.core.batch_config import BatchConfig # noqa: TCH001 | ||
from great_expectations.core.expectation_suite import ( | ||
ExpectationSuite, | ||
expectationSuiteSchema, | ||
) | ||
|
||
|
||
class ValidationConfig(BaseModel): | ||
|
@@ -23,9 +22,26 @@ class ValidationConfig(BaseModel): | |
|
||
""" | ||
|
||
class Config: | ||
arbitrary_types_allowed = True | ||
json_encoders = { | ||
ExpectationSuite: lambda v: expectationSuiteSchema.dump(v), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think we should add a comment on the why here in the code. |
||
} | ||
Comment on lines
+26
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Concurrently using Marshmallow and Pydantic results in something like this. We need custom encoders/decoders but this should work as intended |
||
|
||
name: str | ||
data: BatchConfig # TODO: Should support a union of Asset | BatchConfig | ||
suite: ExpectationSuite | ||
id: Union[str, None] = None | ||
|
||
@validator("suite", pre=True) | ||
def _validate_suite(cls, v): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above - I think it's worth a docstring explaining this is just because the suite isn't pydantic. |
||
if isinstance(v, dict): | ||
return ExpectationSuite(**expectationSuiteSchema.load(v)) | ||
elif isinstance(v, ExpectationSuite): | ||
return v | ||
raise ValueError( | ||
"Suite must be a dictionary (if being deserialized) or an ExpectationSuite object." | ||
) | ||
|
||
@public_api | ||
def run(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,7 @@ class SerializableDataContext(AbstractDataContext): | |
DataContextConfigDefaults.EXPECTATIONS_BASE_DIRECTORY.value, | ||
DataContextConfigDefaults.PLUGINS_BASE_DIRECTORY.value, | ||
DataContextConfigDefaults.PROFILERS_BASE_DIRECTORY.value, | ||
DataContextConfigDefaults.VALIDATION_CONFIGS_BASE_DIRECTORY.value, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updates to enable proper filesystem scaffolding |
||
GX_UNCOMMITTED_DIR, | ||
] | ||
GX_DIR: ClassVar[str] = "gx" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -133,6 +133,7 @@ class GXCloudStoreBackend(StoreBackend, metaclass=ABCMeta): | |
GXCloudRESTResource.PROFILER: "profiler", | ||
GXCloudRESTResource.RENDERED_DATA_DOC: "rendered_data_doc", | ||
GXCloudRESTResource.VALIDATION_RESULT: "result", | ||
GXCloudRESTResource.VALIDATION_CONFIG: "validation_config", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updates to enable proper Cloud integration (purely mocking for now) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like the enum can be modified/restructured if we have to list all these enums and define a new value for them. This makes it appear that our enum/abstraction is wrong. |
||
} | ||
|
||
ALLOWED_SET_KWARGS_BY_RESOURCE_TYPE: Dict[GXCloudRESTResource, Set[str]] = { | ||
|
@@ -157,6 +158,7 @@ class GXCloudStoreBackend(StoreBackend, metaclass=ABCMeta): | |
GXCloudRESTResource.PROFILER: "profilers", | ||
GXCloudRESTResource.RENDERED_DATA_DOC: "rendered_data_docs", | ||
GXCloudRESTResource.VALIDATION_RESULT: "validation_results", | ||
GXCloudRESTResource.VALIDATION_CONFIG: "validation_configs", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly here. |
||
} | ||
) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
from __future__ import annotations | ||
|
||
from great_expectations.compatibility.typing_extensions import override | ||
from great_expectations.core.data_context_key import StringKey | ||
from great_expectations.core.validation_config import ValidationConfig | ||
from great_expectations.data_context.cloud_constants import GXCloudRESTResource | ||
from great_expectations.data_context.store.store import Store | ||
from great_expectations.data_context.types.resource_identifiers import ( | ||
GXCloudIdentifier, | ||
) | ||
|
||
|
||
class ValidationConfigStore(Store): | ||
_key_class = StringKey | ||
|
||
def get_key( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implementing the bare minimum required in the factory. Tests should reflect this as well |
||
self, name: str, id: str | None = None | ||
) -> GXCloudIdentifier | StringKey: | ||
"""Given a name and optional ID, build the correct key for use in the ValidationConfigStore.""" | ||
if self.cloud_mode: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not for this PR but we need to get away from these cloud mode checks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. It's poorly encapsulated but I couldn't think of a better way to get both file and cloud working |
||
return GXCloudIdentifier( | ||
resource_type=GXCloudRESTResource.VALIDATION_CONFIG, | ||
id=id, | ||
resource_name=name, | ||
) | ||
return StringKey(key=name) | ||
|
||
@override | ||
def serialize(self, value): | ||
if self.cloud_mode: | ||
data = value.dict() | ||
data["suite"] = data["suite"].to_json_dict() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another necessity due to Marshmallow |
||
return data | ||
|
||
return value.json() | ||
|
||
@override | ||
def deserialize(self, value): | ||
return ValidationConfig.parse_raw(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This type can be inferred and is the same as the constructor so you don't need this annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weirdly enough I got a mypy error without this (when using it in the custom Pydantic encoder below)