[FEATURE] Add YAML config option to disable progress bars #3794

cdkini · 2021-12-03T19:14:43Z

Changes proposed in this pull request:

Ability to disable progress bars for expectations #3775

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 made corresponding changes to the documentation
I have added unit tests where applicable and made sure that new and existing tests are passing.
I have run any local integration tests and made sure that nothing is broken.

Thank you for submitting!

netlify · 2021-12-03T19:14:51Z

✔️ Deploy Preview for niobium-lead-7998 ready!

🔨 Explore the source changes: a4352f3

🔍 Inspect the deploy log: https://app.netlify.com/sites/niobium-lead-7998/deploys/61d4ca1adc7b8c0008befb57

😎 Browse the preview: https://deploy-preview-3794--niobium-lead-7998.netlify.app

github-actions · 2021-12-03T19:15:06Z

HOWDY! This is your friendly 🤖 CHANGELOG bot 🤖

Please don't forget to add a clear and succinct description of your change under the Develop header in docs_rtd/changelog.rst, if applicable. This will help us with the release process. See the Contribution checklist in the Great Expectations documentation for the type of labels to use!

✨ Thank you! ✨

…tions into feature/devrel-766/option-to-disable-progress-bars

…great-expectations/great_expectations into feature/devrel-766/option-to-disable-progress-bars

…tions into feature/devrel-766/option-to-disable-progress-bars

cdkini · 2021-12-14T23:16:55Z

great_expectations/data_context/types/base.py

+    def is_enabled(self, key: str) -> bool:
+        if super().__getitem__("globally") is False:
+            return False
+        elif super().__getitem__(key) is False:
+            return False
+        return True


Am I getting this relationship correct? Global has the ability to shut down everything even if profilers or metric_calculations is enabled?

cdkini · 2021-12-14T23:18:49Z

great_expectations/validator/validator.py

+            if self._data_context:
+                disable = not self._data_context.progress_bars.is_enabled(
+                    "metric_calculations"
+                )
+            else:
+                disable = len(graph.edges) < 3
+


This is slightly different because we're calculating a boolean and immediately passing it into the tdqm funciton. disable=disable is readable to me but let me know if this should be enable and disable=not enable

cdkini · 2021-12-14T23:19:15Z

tests/data_context/test_config.py

+from great_expectations.data_context import BaseDataContext
+from great_expectations.data_context.types.base import (
+    ConcurrencyConfig,
+    DataContextConfig,
+    InMemoryStoreBackendDefaults,
+    ProgressBarConfig,
+)
+


Couldn't find a good location for tests so I took the existing test_concurrency_config and made it more general

…tions into feature/devrel-766/option-to-disable-progress-bars

talagluck

Looks good overall! A few suggestions for adjustments in functionality. It would also be good to add some documentation for this one.

talagluck · 2021-12-16T15:52:26Z

great_expectations/data_context/types/base.py

+class ProgressBarsConfig(DictDot):
+    def __init__(
+        self,
+        globally: bool = True,
+        profilers: bool = True,
+        metric_calculations: bool = True,
+    ):
+        self.globally = globally
+        self.profilers = profilers
+        self.metric_calculations = metric_calculations
+
+
+class ProgressBarsConfigSchema(Schema):
+    globally = fields.Boolean(default=True)
+    profilers = fields.Boolean(default=True)
+    metric_calculations = fields.Boolean(default=True)


Will this automatically create this field when a new Great Expectations project is initialized? I want to make sure that we make this option available through the Data Context, but also that we don't add a mandatory field to the Data Context, so we don't run into an issue where every new Data Context created won't work with older version of Great Expectations, unless the user specifically wants this feature.

Now that I think about it, I think that the same issue might have happened a couple of times with concurrency, and I wonder if we should treat that in the same way.

when creating a new DataContext through v3 init, I don't see this field nor concurrency. Let's discuss this live to make sure I'm capturing everything

talagluck · 2021-12-16T15:55:57Z

great_expectations/profile/user_configurable_profiler.py

+            progress_bars = context.progress_bars
+            if progress_bars.get("globally") is False:
+                self._enable_progress_bars = False
+            elif progress_bars.get("profilers") is False:
+                self._enable_progress_bars = False
+


I would expect that progress_bars.get("profilers") would override progress_bars.get("globally") irrespective of whether it is True or False. So something like:

Suggested change

progress_bars = context.progress_bars

if progress_bars.get("globally") is False:

self._enable_progress_bars = False

elif progress_bars.get("profilers") is False:

self._enable_progress_bars = False

progress_bars = context.progress_bars

self._enable_progress_bars = progress_bars.get("globally")

if progress_bars.get("profilers") is not None:

self._enable_progress_bars = progress_bars.get("profilers")

ah yes that's what we had discussed! I knew I was missing something 🙇🏽

talagluck · 2021-12-16T15:57:23Z

great_expectations/validator/validator.py

+                if progress_bars.get("globally") is False:
+                    disable = True
+                elif progress_bars.get("metric_calculations") is False:
+                    disable = True
+


Same as above in Profilers.

talagluck · 2021-12-16T15:57:38Z

great_expectations/validator/validator.py

@@ -689,12 +693,21 @@ def resolve_validation_graph(
                validation_graph=graph, metrics=metrics
            )

+            # Check to see if the user has disabled progress bars
+            disable = len(graph.edges) < 3


How did you arrive at this?

This was the previous condition as set by Alex! I didn't want to remove old behavior so in the case we've enabled the progress bars, that original conditional still applies

cdkini · 2021-12-16T19:55:05Z

great_expectations/validator/validator.py

+            if self._data_context:
+                progress_bars = self._data_context.progress_bars
+                if "globally" in progress_bars:
+                    disable = not progress_bars["globally"]
+                if "metric_calculations" in progress_bars:
+                    disable = not progress_bars["metric_calculations"]


Suggested change

if self._data_context:

progress_bars = self._data_context.progress_bars

if "globally" in progress_bars:

disable = not progress_bars["globally"]

if "metric_calculations" in progress_bars:

disable = not progress_bars["metric_calculations"]

if self._data_context:

progress_bars = self._data_context.progress_bars

if "globally" in progress_bars:

disable = not progress_bars["globally"]

if "metric_calculations" in progress_bars:

disable = not progress_bars["metric_calculations"]

Suggested change

if self._data_context:

progress_bars = self._data_context.progress_bars

if "globally" in progress_bars:

disable = not progress_bars["globally"]

if "metric_calculations" in progress_bars:

disable = not progress_bars["metric_calculations"]

if self._data_context:

progress_bars = self._data_context.progress_bars

if "globally" in progress_bars:

disable = not progress_bars["globally"]

if "metric_calculations" in progress_bars:

disable = not progress_bars["metric_calculations"]

if len(graph.edges) < 3:

disable = True

…tions into feature/devrel-766/option-to-disable-progress-bars

…ress-bars

…tions into feature/devrel-766/option-to-disable-progress-bars

…github.com:great-expectations/great_expectations into feature/devrel-766/option-to-disable-progress-bars

…ress-bars

…tions into feature/devrel-766/option-to-disable-progress-bars

talagluck

LGTM! Have you done manual testing to confirm that the progress_bars key doesn't write to a DataContext config if null, and that this won't populate a new DataContext by default? Assuming so, this is all set! Otherwise one other non-gating question. Nice work!

talagluck · 2022-01-04T21:23:39Z

tests/profile/test_user_configurable_profiler_v3_batch_request.py

@@ -1187,3 +1189,48 @@ def test_expect_compound_columns_to_be_unique(
    }

    assert expected_expectations == expectations_from_suite
+
+
+@mock.patch("great_expectations.profile.user_configurable_profiler.tqdm")


Interesting - is this checking to see if the library was called?

yup! the mock object has called and called_list attrs so we can use those to investigate the behavior without actually using the lib

feat: Add YAML config option to disable progress bars

cacf5cf

cdkini self-assigned this Dec 3, 2021

Shinnnyshinshin and others added 21 commits December 7, 2021 16:22

push before adding test descriptions

0f8f952

updated tests

0635ed6

Update test_data_context.py

6e1aed0

Update test_data_context.py

a303d8a

Merge branch 'develop' into feature/ANT-29/datacontext-is-singleton

70a1bbb

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

2356561

…tions into feature/devrel-766/option-to-disable-progress-bars

Merge branch 'feature/ANT-29/datacontext-is-singleton' of github.com:…

df1fbc4

…great-expectations/great_expectations into feature/devrel-766/option-to-disable-progress-bars

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

cdb5544

…tions into feature/devrel-766/option-to-disable-progress-bars

feat: Add granular fields

c064add

chore: Remove util method

c98f6d8

chore: Revert Validator

632d7b6

feat: Add conditional logic to validator

af110ad

feat: Add logic to get progress bar config in UCP

4e909c9

feat: Add util func

e5bd91a

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

2bd69ce

…tions into feature/devrel-766/option-to-disable-progress-bars

feat: use DataContext progress_bars attr

015a483

chore: remove unnecessary imports

9b7e19b

feat: move functionality to ProgressBarsConfig

a4a964d

fix: ensure proper name of key in Validator

127a7fb

refactor: make methods public to work with method

b1bfc90

feat: use is_enabled instead of disabled

1ed9d26

cdkini changed the title ~~[WIP][FEATURE] Add YAML config option to disable progress bars~~ [FEATURE] Add YAML config option to disable progress bars Dec 14, 2021

cdkini marked this pull request as ready for review December 14, 2021 23:11

cdkini requested review from austiezr and talagluck December 14, 2021 23:11

cdkini commented Dec 14, 2021

View reviewed changes

cdkini added 4 commits December 16, 2021 09:16

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

e584418

…tions into feature/devrel-766/option-to-disable-progress-bars

fix: fix typo of new class

80903e7

fix: remove method on new config class

8ef6b94

test: add new field to tests

470a5f4

talagluck reviewed Dec 16, 2021

View reviewed changes

feat: update conditional per Tal's comments

ef7a1a5

cdkini commented Dec 16, 2021

View reviewed changes

cdkini added 15 commits December 16, 2021 15:31

chore: make misc changes after discussion with Tal

ad55c7d

feat: add post_dump hook

6cae763

chore: type hint

7c4961c

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

fa7fccd

…tions into feature/devrel-766/option-to-disable-progress-bars

docs: add comments to post_dump hook

ab27979

Merge branch 'develop' into feature/devrel-766/option-to-disable-prog…

58a6b6c

…ress-bars

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

a9006c4

…tions into feature/devrel-766/option-to-disable-progress-bars

Merge branch 'feature/devrel-766/option-to-disable-progress-bars' of …

9698c4f

…github.com:great-expectations/great_expectations into feature/devrel-766/option-to-disable-progress-bars

Merge branch 'develop' into feature/devrel-766/option-to-disable-prog…

fbedc25

…ress-bars

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

d041120

…tions into feature/devrel-766/option-to-disable-progress-bars

test: write tests for user configurable profiler

ea20dcb

test: write tests for validator

e2b82cd

docs: add comment to test

902ca99

test: misc cleanup in prep for review

38b6abb

chore: remove progress bars from test output

d668ac0

talagluck approved these changes Jan 4, 2022

View reviewed changes

test: fix breaking test

a4352f3

cdkini enabled auto-merge (squash) January 4, 2022 22:11

cdkini force-pushed the feature/devrel-766/option-to-disable-progress-bars branch from 7e6be55 to a4352f3 Compare January 4, 2022 22:28

cdkini merged commit 5168f73 into develop Jan 4, 2022

cdkini deleted the feature/devrel-766/option-to-disable-progress-bars branch January 4, 2022 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Add YAML config option to disable progress bars #3794

[FEATURE] Add YAML config option to disable progress bars #3794

cdkini commented Dec 3, 2021 •

edited

netlify bot commented Dec 3, 2021 •

edited

github-actions bot commented Dec 3, 2021

cdkini Dec 14, 2021

cdkini Dec 14, 2021

cdkini Dec 14, 2021

talagluck left a comment

talagluck Dec 16, 2021

cdkini Dec 16, 2021

talagluck Dec 16, 2021

cdkini Dec 16, 2021

talagluck Dec 16, 2021

talagluck Dec 16, 2021

cdkini Dec 16, 2021

cdkini Dec 16, 2021

talagluck left a comment

talagluck Jan 4, 2022

cdkini Jan 4, 2022

[FEATURE] Add YAML config option to disable progress bars #3794

[FEATURE] Add YAML config option to disable progress bars #3794

Conversation

cdkini commented Dec 3, 2021 • edited

Definition of Done

netlify bot commented Dec 3, 2021 • edited

github-actions bot commented Dec 3, 2021

HOWDY! This is your friendly 🤖 CHANGELOG bot 🤖

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talagluck 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

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

talagluck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdkini commented Dec 3, 2021 •

edited

netlify bot commented Dec 3, 2021 •

edited