[FEATURE] `ValidationConfigStore` #9523

cdkini · 2024-02-23T18:02:04Z

Description of PR changes above includes a link to an existing GitHub issue
PR title is prefixed with one of: [BUGFIX], [FEATURE], [DOCS], [MAINTENANCE], [CONTRIB]
Code is linted - run invoke lint (uses black + ruff)
Appropriate tests and docs have been updated

For more information about contributing, see Contribute.

After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!

netlify · 2024-02-23T18:02:27Z

✅ Deploy Preview for niobium-lead-7998 ready!

Name	Link
🔨 Latest commit	`a82f189`
🔍 Latest deploy log	https://app.netlify.com/sites/niobium-lead-7998/deploys/65e0c200ba66c7000858f348
😎 Deploy Preview	https://deploy-preview-9523.docs.greatexpectations.io
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

…_expectations into m/v1-178/validation_config_store

great_expectations/core/validation_config.py

cdkini · 2024-02-27T16:22:50Z

great_expectations/data_context/data_context/serializable_data_context.py

@@ -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,


Updates to enable proper filesystem scaffolding

cdkini · 2024-02-27T16:23:05Z

great_expectations/data_context/store/gx_cloud_store_backend.py

@@ -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",


Updates to enable proper Cloud integration (purely mocking for now)

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.

cdkini · 2024-02-27T16:23:28Z

great_expectations/data_context/store/validation_config_store.py

+            runtime_environment=runtime_environment,
+        )
+
+    def get_key(


Implementing the bare minimum required in the factory. Tests should reflect this as well

cdkini · 2024-02-27T17:53:19Z

great_expectations/core/validation_config.py

+        arbitrary_types_allowed = True
+        json_encoders = {
+            ExpectationSuite: lambda v: expectationSuiteSchema.dump(v),
+        }


Concurrently using Marshmallow and Pydantic results in something like this. We need custom encoders/decoders but this should work as intended

cdkini · 2024-02-27T17:54:31Z

great_expectations/data_context/data_context/abstract_data_context.py

+    @property
+    def validation_config_store_name(self) -> str:
+        return DataContextConfigDefaults.DEFAULT_VALIDATION_CONFIG_STORE_NAME.value
+
+    def validaiton_config_store(self) -> ValidationConfigStore:
+        return self.stores[self.validation_config_store_name]


Wasn't sure about this but deciding to not make validation_config_store_name a context variable. I've wavered on it but I'm now confident that store naming just causes more harm than benefit.

An argument against this is that it is a deviation from the existing pattern and perhaps consistency is more important?

I'm usually not pro-deviation, but I think in this case it's probably reasonable. It'd require minimal changes if we changed our mind later.

Do we know this key will always be present?
What is the harm you see from making this a context variable? By context variable, do you mean a top level property?

By adding it as a context variable, its another field that a user can modify.

These top level store names are dangerous in that they can enable the following:

context.checkpoint_store_name = "my_nonexistent_store" ... context.checkpoint_store # Raises an error context.checkpoints.add(...) # Raises an error ...

We allow customization of the names but don't automatically update the stores dict, resulting in the possibility of orphaning a store by losing its name.

Seems relatively unlikely since we don't advertise this much in docs/public API.

We should always have a ValidationConfigStore but the name may vary.

cdkini · 2024-02-27T17:55:16Z

great_expectations/data_context/store/validation_config_store.py

+    def serialize(self, value):
+        if self.cloud_mode:
+            data = value.dict()
+            data["suite"] = data["suite"].to_json_dict()


Another necessity due to Marshmallow

billdirks

Thanks for doing this. I don't have the best context with respect to stores. I've asked some questions inline.

great_expectations/data_context/data_context/abstract_data_context.py

billdirks · 2024-02-27T19:43:01Z

great_expectations/data_context/data_context/abstract_data_context.py

+    @property
+    def validation_config_store_name(self) -> str:
+        return DataContextConfigDefaults.DEFAULT_VALIDATION_CONFIG_STORE_NAME.value
+
+    def validaiton_config_store(self) -> ValidationConfigStore:
+        return self.stores[self.validation_config_store_name]


Do we know this key will always be present?
What is the harm you see from making this a context variable? By context variable, do you mean a top level property?

billdirks · 2024-02-27T19:46:11Z

great_expectations/data_context/store/validation_config_store.py

+        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:


Not for this PR but we need to get away from these cloud mode checks.

I agree. It's poorly encapsulated but I couldn't think of a better way to get both file and cloud working

…text.py Co-authored-by: Bill Dirks <bill@greatexpectations.io>

…_expectations into m/v1-178/validation_config_store

great_expectations/data_context/data_context/abstract_data_context.py

billdirks

Mostly looks good. I've asked 1 question and added a couple of nits that don't need to be solved now. There is also the outstanding property comment you've made.

billdirks · 2024-02-28T19:52:46Z

great_expectations/core/expectation_suite.py

@@ -1207,4 +1207,4 @@ def _convert_uuids_to_str(self, data, **kwargs):
        return data


-expectationSuiteSchema = ExpectationSuiteSchema()
+expectationSuiteSchema: ExpectationSuiteSchema = ExpectationSuiteSchema()


nit: This type can be inferred and is the same as the constructor so you don't need this annotation.

weirdly enough I got a mypy error without this (when using it in the custom Pydantic encoder below)

billdirks · 2024-02-28T19:54:48Z

great_expectations/core/validation_config.py

+    def _validate_suite(cls, v):
+        if isinstance(v, dict):
+            return ExpectationSuite(**expectationSuiteSchema.load(v))
+        return v


Should we check if v is an expectationSuiteSchema and raise if it's not in the fall through case? Or does pydantic do this for us since it won't be the right type?

Pydantic validation will happen after this return - I've updated this to be a bit more explicit but it should work as-is

billdirks · 2024-02-28T20:01:55Z

great_expectations/data_context/store/gx_cloud_store_backend.py

@@ -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",


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.

billdirks · 2024-02-28T20:02:12Z

great_expectations/data_context/store/gx_cloud_store_backend.py

@@ -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",


Similarly here.

…_expectations into m/v1-178/validation_config_store

tyler-hoffman

Left some nits, but this LGTM

tyler-hoffman · 2024-02-27T19:00:04Z

great_expectations/data_context/data_context/abstract_data_context.py

+    @property
+    def validation_config_store_name(self) -> str:
+        return DataContextConfigDefaults.DEFAULT_VALIDATION_CONFIG_STORE_NAME.value
+
+    def validaiton_config_store(self) -> ValidationConfigStore:
+        return self.stores[self.validation_config_store_name]


I'm usually not pro-deviation, but I think in this case it's probably reasonable. It'd require minimal changes if we changed our mind later.

tyler-hoffman · 2024-02-29T19:15:50Z

great_expectations/core/validation_config.py

+    class Config:
+        arbitrary_types_allowed = True
+        json_encoders = {
+            ExpectationSuite: lambda v: expectationSuiteSchema.dump(v),


nit: I think we should add a comment on the why here in the code.

tyler-hoffman · 2024-02-29T19:16:15Z

great_expectations/core/validation_config.py

+    id: Union[str, None] = None
+
+    @validator("suite", pre=True)
+    def _validate_suite(cls, v):


same as above - I think it's worth a docstring explaining this is just because the suite isn't pydantic.

tyler-hoffman · 2024-02-29T19:23:49Z

tests/data_context/store/test_validation_config_store.py

+    )
+
+
+@pytest.mark.parametrize("store_fixture", ["ephemeral_store", "file_backed_store"])


why use strings for the stores and then call getfixturevalue? Can't you just reference the fixture directly?

No, the fixture won't be properly instantiated unless you do it this way.
Fixtures only get loaded by pytest if you include them in the test constructor.
Although this is a common complaint because it prevents you from using fixtures as part of parametrize.

There are packages out there like pytest-lazy-fixture that essentially do this work for you... but you still have to reference it by the string.
https://github.com/tvorog/pytest-lazy-fixture

billdirks

LGTM!

billdirks · 2024-02-29T20:17:27Z

great_expectations/data_context/types/base.py

+                )
+            if config["class_name"] not in configured_stores:
+                # Create ephemeral store config
+                store_configs[name] = {"class_name": config["class_name"]}


Please write a warning if we use the default store.

Kilo59 · 2024-02-29T21:59:13Z

tests/data_context/store/test_validation_config_store.py

+
+@pytest.mark.parametrize("store_fixture", ["ephemeral_store", "file_backed_store"])
+@pytest.mark.unit
+def test_add(request, store_fixture: str, validation_config: ValidationConfig):


Suggested change

def test_add(request, store_fixture: str, validation_config: ValidationConfig):

def test_add(request: pytest.FixtureRequest, store_fixture: str, validation_config: ValidationConfig):

Annoting request will developers understand what's going on.

https://docs.pytest.org/en/stable/reference/reference.html#request

start impl

44172f8

cdkini added 7 commits February 23, 2024 13:02

update init

2b92cd4

remove type ignore

008bd50

patch tests

31aafb3

patch test

be70160

Merge branch 'develop' of https://github.com/great-expectations/great…

3e9a833

…_expectations into m/v1-178/validation_config_store

start tests

295eba0

more tests

fe3ebfe

cdkini commented Feb 27, 2024

View reviewed changes

great_expectations/core/validation_config.py Outdated Show resolved Hide resolved

cdkini commented Feb 27, 2024

View reviewed changes

cdkini added 3 commits February 27, 2024 11:50

simplify store

ddddc8d

get tests passing

9fa6eba

plug in store to factory

0997f8b

cdkini commented Feb 27, 2024

View reviewed changes

remove debug call

d62cd3b

cdkini commented Feb 27, 2024

View reviewed changes

cdkini added 3 commits February 27, 2024 13:00

remove extraneous init

8f088d6

misc updates

3771806

from __future__

764ad93

billdirks reviewed Feb 27, 2024

View reviewed changes

cdkini and others added 6 commits February 27, 2024 14:55

Update great_expectations/data_context/data_context/abstract_data_con…

537def9

…text.py Co-authored-by: Bill Dirks <bill@greatexpectations.io>

remove forward refs

3eba6a4

mypy

61a1c23

patch spark test

76700d8

Merge branch 'develop' of https://github.com/great-expectations/great…

dffbbe1

…_expectations into m/v1-178/validation_config_store

add id

b7d1072

cdkini added 2 commits February 28, 2024 11:29

fix unit tests

a216c92

patch cloud test

2c766f9

cdkini commented Feb 28, 2024

View reviewed changes

great_expectations/data_context/data_context/abstract_data_context.py Show resolved Hide resolved

update validation_config_store prop

07acaea

billdirks reviewed Feb 28, 2024

View reviewed changes

cdkini added 14 commits February 28, 2024 15:21

add property

f137989

misc updates

8a948fb

Merge branch 'develop' of https://github.com/great-expectations/great…

ec7c5f8

…_expectations into m/v1-178/validation_config_store

add default stores

947d901

update store backend defaults and tests

72bdf51

mypy

6882737

ephemeral defaults

deb1076

update templates

695cc17

patch s3 tests

182edab

patch cloud tests

a65ce0b

patch cloud tests

c533188

fix docs tests

f987bef

Merge branch 'develop' of https://github.com/great-expectations/great…

22e5d55

…_expectations into m/v1-178/validation_config_store

update validator

1c256ca

cdkini requested review from tyler-hoffman and billdirks February 29, 2024 17:16

update docs tests

a82f189

tyler-hoffman approved these changes Feb 29, 2024

View reviewed changes

billdirks approved these changes Feb 29, 2024

View reviewed changes

billdirks reviewed Feb 29, 2024

View reviewed changes

cdkini added this pull request to the merge queue Feb 29, 2024

Merged via the queue into develop with commit 7b87f43 Feb 29, 2024
67 checks passed

cdkini deleted the m/v1-178/validation_config_store branch February 29, 2024 20:48

Kilo59 reviewed Feb 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] `ValidationConfigStore` #9523

[FEATURE] `ValidationConfigStore` #9523

cdkini commented Feb 23, 2024

netlify bot commented Feb 23, 2024 •

edited

cdkini Feb 27, 2024

cdkini Feb 27, 2024

billdirks Feb 28, 2024

cdkini Feb 27, 2024

cdkini Feb 27, 2024

cdkini Feb 27, 2024

tyler-hoffman Feb 27, 2024

billdirks Feb 27, 2024

cdkini Feb 27, 2024

cdkini Feb 27, 2024

cdkini Feb 27, 2024

billdirks left a comment

billdirks Feb 27, 2024

billdirks Feb 27, 2024

cdkini Feb 27, 2024

billdirks left a comment •

edited

billdirks Feb 28, 2024

cdkini Feb 28, 2024

billdirks Feb 28, 2024

cdkini Feb 29, 2024

billdirks Feb 28, 2024

billdirks Feb 28, 2024

tyler-hoffman left a comment

tyler-hoffman Feb 27, 2024

tyler-hoffman Feb 29, 2024

tyler-hoffman Feb 29, 2024

tyler-hoffman Feb 29, 2024

Kilo59 Feb 29, 2024 •

edited

billdirks left a comment

billdirks Feb 29, 2024

Kilo59 Feb 29, 2024 •

edited

		)


		@pytest.mark.parametrize("store_fixture", ["ephemeral_store", "file_backed_store"])

	def test_add(request, store_fixture: str, validation_config: ValidationConfig):
	def test_add(request: pytest.FixtureRequest, store_fixture: str, validation_config: ValidationConfig):

[FEATURE] ValidationConfigStore #9523

[FEATURE] ValidationConfigStore #9523

Conversation

cdkini commented Feb 23, 2024

netlify bot commented Feb 23, 2024 • edited

✅ Deploy Preview for niobium-lead-7998 ready!

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billdirks 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

billdirks left a comment • edited

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

tyler-hoffman 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

Kilo59 Feb 29, 2024 • edited

Choose a reason for hiding this comment

billdirks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kilo59 Feb 29, 2024 • edited

Choose a reason for hiding this comment

[FEATURE] `ValidationConfigStore` #9523

[FEATURE] `ValidationConfigStore` #9523

netlify bot commented Feb 23, 2024 •

edited

billdirks left a comment •

edited

Kilo59 Feb 29, 2024 •

edited

Kilo59 Feb 29, 2024 •

edited