[MAINTENANCE] Migrate remaining methods from `BaseDataContext` #6403

cdkini · 2022-11-20T18:04:26Z

Changes proposed in this pull request:

Move final methods out of BaseDataContext such that only the following remain:
- Logic to instantiate self._data_context
- Methods that take in a call and pass it to the underlying self._data_context
The BaseDataContext will become a true facade with a subsequent ticket - we'll pass any attr calls to the underlying self._data_context instance utilizing some Python magic (__getattr__ override)
- This is out of scope for this particular ticket.

Definition of Done

My code follows the Great Expectations style guide
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have run any local integration tests and made sure that nothing is broken.

netlify · 2022-11-20T18:04:31Z

✅ Deploy Preview for niobium-lead-7998 ready!

Name	Link
🔨 Latest commit	`c1faf0f`
🔍 Latest deploy log	https://app.netlify.com/sites/niobium-lead-7998/deploys/637b98492998810009579326
😎 Deploy Preview	https://deploy-preview-6403--niobium-lead-7998.netlify.app
📱 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 settings.

ghost · 2022-11-20T18:05:43Z

👇 Click on the image for a new way to code review

Make big changes easier — review code in small groups of related files
Know where to start — see the whole change at a glance
Take a code tour — explore the change with an interactive tour
Make comments and review — all fully sync’ed with github

Try it now!

Legend

cdkini · 2022-11-20T21:15:01Z

great_expectations/data_context/data_context/abstract_data_context.py

-    @abstractmethod
+    @usage_statistics_enabled_method(
+        event_name=UsageStatsEvents.DATA_CONTEXT_SAVE_EXPECTATION_SUITE,
+        args_payload_fn=save_expectation_suite_usage_statistics,
+    )


The implementation is pretty much the same across the hierarchy so let's remove some impls and the @abstracmethod decorator and put the default impl here. Note we're still overriding in CloudDataContext due to the GXCloudIdentifier creation logic.

Further work can be done here but for now, I think this feels pretty good.

cdkini · 2022-11-20T21:16:50Z

great_expectations/data_context/data_context/abstract_data_context.py

@@ -3724,3 +3756,52 @@ def escape_all_config_variables(
                return value.replace("$", dollar_sign_escape_string)
            return value
        return value.replace("$", dollar_sign_escape_string)
+
+    def save_config_variable(


This method has some pretty filesystem specific logic but a larger overhaul to file-based logic will occur shortly.

In short, both FileDataContext and CloudDataContext have "fileystem-enabled" capabilities - these two classes should inherit from a separate _FilesystemEnabledDataContext or something to that effect.

For now, I think it's okay to put in AbstractDataContext.

…_expectations into m/great-1389/migrate_final_base_context_methods

cdkini · 2022-11-21T15:31:38Z

great_expectations/data_context/data_context/base_data_context.py

+    def save_expectation_suite(
+        self,
+        expectation_suite: ExpectationSuite,
+        expectation_suite_name: Optional[str] = None,
+        overwrite_existing: bool = True,
+        include_rendered_content: Optional[bool] = None,
+        **kwargs: Optional[dict],
+    ) -> None:
+        self._data_context.save_expectation_suite(
+            expectation_suite,
+            expectation_suite_name=expectation_suite_name,
+            overwrite_existing=overwrite_existing,
+            include_rendered_content=include_rendered_content,
+            **kwargs,
+        )


An unfortunate necessity to get things working - this will be removed in GREAT-1379 (converting the BaseDataContext into a facade class)

Sounds good :) thanks for the note

Shinnnyshinshin · 2022-11-21T18:02:43Z

great_expectations/data_context/data_context/cloud_data_context.py

@@ -508,6 +513,10 @@ def get_expectation_suite(
            expectation_suite.render()
        return expectation_suite

+    @usage_statistics_enabled_method(


Shinnnyshinshin · 2022-11-21T18:03:21Z

great_expectations/data_context/data_context/data_context.py

@@ -264,6 +264,22 @@ def __init__(
        if self._check_for_usage_stats_sync(project_config):
            self._save_project_config()

+    def _save_project_config(self) -> None:


Shinnnyshinshin

Looks great :) Thank you @cdkini

* develop: (22 commits) [BUGFIX] issue-4295-fix-issue (#6164) [DOCS] add boto3 explanations on document (#6407) [FEATURE] add multiple column metric (#6372) [MAINTENANCE] Small refactor (#6422) [MAINTENANCE] Sorting batch IDs and typehints clean up (#6421) [MAINTENANCE] Clean Up Type Hints and Minor Refactoring For Better Code Elegance/Readability (#6418) [MAINTENANCE] Implement `RendererConfiguration` (#6412) [BUGFIX] updated capitalone setup.py file (#6410) [FEATURE]: DataProfilerUnstructuredDataAssistant Integration (#6400) [FEATURE] add new metric - query template values (#5994) [MAINTENANCE] Cleanup For Better Code Elegance/Readability (#6406) [MAINTENANCE] ZEP - `GxConfig` cleanup (#6404) [MAINTENANCE] Migrate remaining methods from `BaseDataContext` (#6403) [BUGFIX] Patch key-generation issue with `DataContext.save_profiler()` (#6405) [MAINTENANCE] Migrate additional CRUD methods from `BaseDataContext` to `AbstractDataContext` (#6395) [MAINTENANCE] ZEP add yaml methods to all experimental models (#6401) [FEATURE] ZEP Config serialize as YAML (#6398) [MAINTENANCE] Remove call to verify_library_dependent_modules for pybigquery (#6394) [MAINTENANCE] Make "IDDict.to_id()" serialization more efficient. (#6389) [RELEASE] 0.15.34 (#6397) ...

refactor: move last methods

bdfa181

github-actions bot added the core-team label Nov 20, 2022

cdkini self-assigned this Nov 20, 2022

cdkini commented Nov 20, 2022

View reviewed changes

cdkini force-pushed the m/great-1389/migrate_final_base_context_methods branch from 0d69f1d to bdfa181 Compare November 21, 2022 14:59

cdkini added 3 commits November 21, 2022 09:59

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

7b85783

…_expectations into m/great-1389/migrate_final_base_context_methods

refactor: clean up save_expectation_suite

36ead04

chore: remove unused imports

6a0e933

cdkini marked this pull request as ready for review November 21, 2022 15:15

cdkini added 3 commits November 21, 2022 10:21

chore: revert usage stats decorator

6553ae4

feat: add back impl to base

bfd19ca

feat: add decorator back to cloud

c1faf0f

cdkini commented Nov 21, 2022

View reviewed changes

cdkini enabled auto-merge (squash) November 21, 2022 16:07

Shinnnyshinshin reviewed Nov 21, 2022

View reviewed changes

Shinnnyshinshin approved these changes Nov 21, 2022

View reviewed changes

cdkini merged commit 60a1876 into develop Nov 21, 2022

cdkini deleted the m/great-1389/migrate_final_base_context_methods branch November 21, 2022 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAINTENANCE] Migrate remaining methods from `BaseDataContext` #6403

[MAINTENANCE] Migrate remaining methods from `BaseDataContext` #6403

cdkini commented Nov 20, 2022 •

edited

Loading

netlify bot commented Nov 20, 2022 •

edited

Loading

ghost commented Nov 20, 2022 •

edited by ghost

Loading

cdkini Nov 20, 2022

cdkini Nov 20, 2022

cdkini Nov 21, 2022

Shinnnyshinshin Nov 21, 2022

Shinnnyshinshin Nov 21, 2022

Shinnnyshinshin Nov 21, 2022

Shinnnyshinshin left a comment

[MAINTENANCE] Migrate remaining methods from BaseDataContext #6403

[MAINTENANCE] Migrate remaining methods from BaseDataContext #6403

Conversation

cdkini commented Nov 20, 2022 • edited Loading

Definition of Done

netlify bot commented Nov 20, 2022 • edited Loading

✅ Deploy Preview for niobium-lead-7998 ready!

ghost commented Nov 20, 2022 • edited by ghost Loading

Legend

cdkini Nov 20, 2022

Choose a reason for hiding this comment

cdkini Nov 20, 2022

Choose a reason for hiding this comment

cdkini Nov 21, 2022

Choose a reason for hiding this comment

Shinnnyshinshin Nov 21, 2022

Choose a reason for hiding this comment

Shinnnyshinshin Nov 21, 2022

Choose a reason for hiding this comment

Shinnnyshinshin Nov 21, 2022

Choose a reason for hiding this comment

Shinnnyshinshin left a comment

Choose a reason for hiding this comment

[MAINTENANCE] Migrate remaining methods from `BaseDataContext` #6403

[MAINTENANCE] Migrate remaining methods from `BaseDataContext` #6403

cdkini commented Nov 20, 2022 •

edited

Loading

netlify bot commented Nov 20, 2022 •

edited

Loading

ghost commented Nov 20, 2022 •

edited by ghost

Loading