[MAINTENANCE] Refactor `usage_stats_opt_out` method in DataContext #5339

Shinnnyshinshin · 2022-06-17T21:42:38Z

Changes proposed in this pull request:

Follow up to [MAINTENANCE] Move _apply_global_config_overrides() to AbstractDataContext #5285
Small refactor to _check_global_usage_statistics_opt_out() so that we dont have to "flip" the boolean in our minds. Before if opt_out = True, then usage_statistics_enabled = False.
Now we use _get_global_usage_statistics_override() which returns True if usage_statistics_enabled = True, and False if usage_statistics_enabled = False.
GREAT-953

Update 2022-08-26

anonymous_usage_stats_enabled can be set in the following 3 places
- great_expectations.yml (YML)
- GE_USAGE_STATS environment variable (env)
- conf file (conf)

If it is set as False in any of the 3 places, the global value is set to False

This is a change from previous behavior which set the following precedence: environment variable > home folder > /etc > yml
This PR adds a 3 tests that check if setting YML, env, or conf to False will set the global usage_statistics flag to False.
It also removes tests that used to check the test percedence.

Definition of Done

Please delete options that are not relevant.

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.

…-abc

…ure/GREAT-953/migration-part1-move-to-abc

…ure/GREAT-953/migration-part1-move-to-abc * bugfix/GREAT-953/tests-deleting-env-variable: pushing fix for test

…ure/GREAT-953/migration-part1-move-to-abc * bugfix/GREAT-953/tests-deleting-env-variable: removed extra fixture

* develop: [MAINTENANCE] Suppress persisting of temporary ExpectationSuite configurations in Rule-Based Profiler computations (#5305) [DOCS] Update standard arguments doc for Expectations to not reference datasets. (#5052) [MAINTENANCE] Handle compare Expectation in presence of high precision floating point numbers and NaN values (#5298) [MAINTENANCE] `DataAssistantResult` cleanup and extensibility enhancements (#5259) chore: check for mypy existence (#5291)

…ure/GREAT-953/migration-part1-move-to-abc * bugfix/GREAT-953/tests-deleting-env-variable: Update migration_guide_postgresql_v3_api.py [MAINTENANCE] Suppress persisting of temporary ExpectationSuite configurations in Rule-Based Profiler computations (#5305) [DOCS] Update standard arguments doc for Expectations to not reference datasets. (#5052) [MAINTENANCE] Handle compare Expectation in presence of high precision floating point numbers and NaN values (#5298) [MAINTENANCE] `DataAssistantResult` cleanup and extensibility enhancements (#5259) chore: check for mypy existence (#5291)

…r-usage-stats-opt * develop: [MAINTENANCE] In ExecutionEngine: Make variable names and usage more descriptive of their purpose. (#6342) [MAINTENANCE] Refactor variable aggregation/substitution logic into `ConfigurationProvider` hierarchy (#6321) [MAINTENANCE] Add missing one-line docstrings and try to make the others consistent (#6340) [BUGFIX] Fix issue with misaligned indentation in docs snippets (#6339)

Shinnnyshinshin · 2022-11-11T01:48:36Z

great_expectations/data_context/data_context/abstract_data_context.py

@@ -2437,6 +2440,21 @@ def _get_metric_configuration_tuples(

        return metric_configurations_list

+    @classmethod
+    def validate_config(
+        cls, project_config: Union[DataContextConfig, Mapping]


validate_config() 's behavior has been extended to always return a DataContextConfig or error out. This helps with mypy errors, which were complaining that parameters were being Union[DataContextConfig, Mapping] in one place and DataContextConfig in another.

It has also been moved up to AbstractDataContext because BaseDataContext and CloudDataContext both use it.

This is the previous implementation

@classmethod def validate_config(cls, project_config: Union[DataContextConfig, Mapping]) -> bool: if isinstance(project_config, DataContextConfig): return True try: dataContextConfigSchema.load(project_config) except ValidationError: raise return True

Is this name still appropriate? Is there perhaps a better name to signal what it is doing now that it isn't returning a bool?

alexsherstinsky

LGTM

cdkini · 2022-11-14T14:27:17Z

great_expectations/data_context/data_context/base_data_context.py

-            )
+
+        project_data_context_config: DataContextConfig = (
+            BaseDataContext.validate_config(project_config)


For static/class methods called in instances, do we want to use the class name or self? Good either way but let's be consistent. I think I'm the one who added this class call but I might be the outlier.

great question @cdkini. I had a look, and I think it's a mix of class name and self 😅 My opinion is that we should use class name to distinguish between these and instance methods

cdkini · 2022-11-14T14:27:28Z

great_expectations/data_context/data_context/cloud_data_context.py

@@ -88,8 +88,13 @@ def __init__(
            project_config = self.retrieve_data_context_config_from_ge_cloud(
                ge_cloud_config=self._ge_cloud_config,
            )
+
+        project_data_context_config: DataContextConfig = (
+            CloudDataContext.validate_config(project_config)


replied above

cdkini · 2022-11-14T14:28:25Z

great_expectations/util.py

@@ -1668,7 +1669,7 @@ def convert_ndarray_decimal_to_float_dtype(data: np.ndarray) -> np.ndarray:


 def get_context(
-    project_config: Optional[Union["DataContextConfig", dict]] = None,
+    project_config: Optional[Union["DataContextConfig", Mapping]] = None,


Could we fix up this DataContextConfig type annotation by using from __future__ import annotations and if TYPE_CHECKING?

Good call :) doing now, and for a few more parameters below.

cdkini · 2022-11-14T14:29:46Z

tests/integration/usage_statistics/test_usage_stats_common_messages_are_sent_v3api.py

@@ -94,7 +94,7 @@ def test_common_usage_stats_are_sent_no_mocking(
    context._usage_statistics_handler._close_worker()

    # Make sure usage stats are enabled
-    assert not context._check_global_usage_statistics_opt_out()
+    assert context._get_global_usage_statistics_override()


I know we had this issue before but I don't love directly accessing a private method here - is it possible to get around this at all? Do we need this assertion?

Feels like we might be testing an implementation detail

I can understand this concern. I think we can get around this by checking the boolean flag on anonymous_usage_statistics property, which we do directly below. If that is the case then we can just remove this line

cdkini

Left some misc comments but mainly LGTM!

anthonyburdi

Just a non blocking rename suggestion.

anthonyburdi · 2022-11-14T14:52:03Z

great_expectations/data_context/data_context/abstract_data_context.py

@@ -2530,21 +2545,24 @@ def _load_config_variables(self) -> Dict:
            return config_var_provider.get_values()
        return {}

-    def _check_global_usage_statistics_opt_out(self) -> bool:
+    @staticmethod
+    def _get_global_usage_statistics_override() -> bool:


How about something like _is_usage_stats_enabled() -> bool: since this method returns a boolean?

I really like this suggestion :) changing now

anthonyburdi · 2022-11-14T15:06:19Z

tests/core/usage_statistics/test_usage_statistics.py

@@ -83,7 +83,7 @@ def test_consistent_name_anonymization(


 @pytest.mark.base_data_context
-def test_opt_out_environment_variable_base_data_context(
+def test_global_override_environment_variable_base_data_context(


anthonyburdi · 2022-11-14T15:08:00Z

tests/core/usage_statistics/test_usage_statistics.py

+        - great_expectations.yml (YML)
+        - GE_USAGE_STATS environment variable (env)
+        - conf file (conf)
+    If it is set as `False` in *any* of the 3 places, the global value is set to `False`


…r-usage-stats-opt

* develop: [MAINTENANCE] Update column_reflection_fallback to also use schema name for Trino (#6350) [MAINTENANCE] Misc cleanup of GX Cloud helpers (#6352) [MAINTENANCE] Pin `mypy` to `0.990` (#6361) Subject: Support to include ID/PK in validation result for each row t… (#5876) [MAINTENANCE] Fix computed metrics type hint in ExecutionEngine.resolve_metrics() method (#6347) [MAINTENANCE] Refactor `usage_stats_opt_out` method in DataContext (#5339) Zep PostgresDatasource returns a list of batches. (#6341) [DOCS] Add `yarn snippet-check` command (#6351) [MAINTENANCE] Refactor out `termcolor` dependency (#6348) [MAINTENANCE] Move Cloud-specific enums to `cloud_constants.py` (#6349)

Shinnnyshinshin added 30 commits June 9, 2022 16:46

push of first change. Getting config to be loaded in 1 step

0f0bcb5

remove bad imports

8bc01b7

pushing initial test_project tests

d3d75c3

Update test_datasource_pandas.py

5b89f22

Update test_datasource_sqlite.py

c1350c4

Update test_datasource_sqlite.py

4dc5e33

Update test_project.py

77579db

pushing modified test

471c935

Merge branch 'develop' into feature/GREAT-953/migration-part1-move-to…

52e5fa5

…-abc

pushing changes before submitting other PR

a264be4

refactor and test

9d9ef96

Merge branch 'develop' into bugfix/GREAT-953/tests-deleting-env-variable

65a7a42

Merge branch 'bugfix/GREAT-953/tests-deleting-env-variable' into feat…

42d6815

…ure/GREAT-953/migration-part1-move-to-abc

pushing fix for test

6715b9f

Merge branch 'bugfix/GREAT-953/tests-deleting-env-variable' into feat…

8a33d30

…ure/GREAT-953/migration-part1-move-to-abc * bugfix/GREAT-953/tests-deleting-env-variable: pushing fix for test

linted

5e02338

removed extra fixture

372a861

Merge branch 'bugfix/GREAT-953/tests-deleting-env-variable' into feat…

32be5ab

…ure/GREAT-953/migration-part1-move-to-abc * bugfix/GREAT-953/tests-deleting-env-variable: removed extra fixture

clean up

e670c7e

Update migration_guide_postgresql_v3_api.py

80e10ad

Update ephemeral_data_context.py

9eabba5

upgrading test

251e8ff

Merge branch 'develop' into bugfix/GREAT-953/tests-deleting-env-variable

c8bdf8e

Update test_project.py

7ad92a2

added fixture again

a2b638c

final clean up. remove redundant test

336443e

remove redundant fixture

9045cdf

fixed test

b55e4c5

Will Shin added 4 commits November 10, 2022 16:19

Update cloud_data_context.py

35cf4d0

clean up and removing (hopefully) all of the typing errors

81b562f

moving validate_config method out so that it can be shared to abstract

d427765

Shinnnyshinshin commented Nov 11, 2022

View reviewed changes

Shinnnyshinshin requested a review from cdkini November 11, 2022 01:49

alexsherstinsky approved these changes Nov 11, 2022

View reviewed changes

Kilo59 self-requested a review November 14, 2022 14:05

cdkini reviewed Nov 14, 2022

View reviewed changes

anthonyburdi approved these changes Nov 14, 2022

View reviewed changes

Will Shin added 2 commits November 14, 2022 09:25

after review

f70323c

Merge branch 'develop' into feature/GREAT-953/migration-part2-refacto…

e03604c

…r-usage-stats-opt

Shinnnyshinshin enabled auto-merge (squash) November 14, 2022 17:26

Shinnnyshinshin disabled auto-merge November 14, 2022 17:30

Will Shin added 8 commits November 14, 2022 09:45

renamed to get_or_create_data_context_config

07a2987

Update abstract_data_context.py

57eecb7

typo

9ffdc30

linting

973dfa1

Update util.py

278263b

pushing

2912db3

Merge branch 'develop' into feature/GREAT-953/migration-part2-refacto…

a96ae27

…r-usage-stats-opt

Merge branch 'develop' into feature/GREAT-953/migration-part2-refacto…

f80e1ea

…r-usage-stats-opt

Shinnnyshinshin enabled auto-merge (squash) November 14, 2022 21:01

Shinnnyshinshin merged commit 93ee0e9 into develop Nov 14, 2022

Shinnnyshinshin deleted the feature/GREAT-953/migration-part2-refactor-usage-stats-opt branch November 14, 2022 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAINTENANCE] Refactor `usage_stats_opt_out` method in DataContext #5339

[MAINTENANCE] Refactor `usage_stats_opt_out` method in DataContext #5339

Shinnnyshinshin commented Jun 17, 2022 •

edited

Loading

Shinnnyshinshin Nov 11, 2022

cdkini Nov 14, 2022

alexsherstinsky left a comment

cdkini Nov 14, 2022

Shinnnyshinshin Nov 14, 2022 •

edited

Loading

cdkini Nov 14, 2022

Shinnnyshinshin Nov 14, 2022

cdkini Nov 14, 2022

Shinnnyshinshin Nov 14, 2022 •

edited

Loading

cdkini Nov 14, 2022

Shinnnyshinshin Nov 14, 2022

cdkini left a comment

anthonyburdi left a comment

anthonyburdi Nov 14, 2022

Shinnnyshinshin Nov 14, 2022

anthonyburdi Nov 14, 2022

anthonyburdi Nov 14, 2022

[MAINTENANCE] Refactor usage_stats_opt_out method in DataContext #5339

[MAINTENANCE] Refactor usage_stats_opt_out method in DataContext #5339

Conversation

Shinnnyshinshin commented Jun 17, 2022 • edited Loading

Update 2022-08-26

Definition of Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsherstinsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shinnnyshinshin Nov 14, 2022 • edited Loading

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

Shinnnyshinshin Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdkini left a comment

Choose a reason for hiding this comment

anthonyburdi 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

[MAINTENANCE] Refactor `usage_stats_opt_out` method in DataContext #5339

[MAINTENANCE] Refactor `usage_stats_opt_out` method in DataContext #5339

Shinnnyshinshin commented Jun 17, 2022 •

edited

Loading

Shinnnyshinshin Nov 14, 2022 •

edited

Loading

Shinnnyshinshin Nov 14, 2022 •

edited

Loading