[BUGFIX] Insure Proper Indexing of Metric Computation Results in ParameterBuilder #4426

alexsherstinsky · 2022-03-16T00:58:58Z

Scope

We return results from get_metrics() as list, even if the length of list is 1 (previous simplification introduced inconsistencies in the way results are interpreted). Now, different ParameterBuilder implementations interpret the results according to their own logic, often depending on whether or not the number of metric_value_kwargs is greater than 1.

Please annotate your PR title to describe what the PR does, then give a brief bulleted description of your PR below. PR titles should begin with [BUGFIX], [FEATURE], [DOCS], or [MAINTENANCE]. If a new feature introduces breaking changes for the Great Expectations API or configuration files, please also add [BREAKING]. You can read about the tags in our contributor checklist.

Changes proposed in this pull request:

After submitting your PR, CI checks will run and @ge-cla-bot will check for your CLA signature.

For a PR with nontrivial changes, we review with both design-centric and code-centric lenses.

In a design review, we aim to ensure that the PR is consistent with our relationship to the open source community, with our software architecture and abstractions, and with our users' needs and expectations. That review often starts well before a PR, for example in github issues or slack, so please link to relevant conversations in notes below to help reviewers understand and approve your PR more quickly (e.g. closes #123).

Previous Design Review notes:

JIRA: GREAT-464/GREAT-498/GREAT-674

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!

…esults

…sherstinsky/rule_based_profiler_fix_bug_one_regex_get_parameters_result_error-2022_03_15-52

netlify · 2022-03-16T00:59:02Z

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

🔨 Explore the source changes: d8b5ac0

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

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

Shinnnyshinshin

Looks wonderful sir @alexsherstinsky thank you so much for your help with this.

Shinnnyshinshin · 2022-03-16T01:02:45Z

great_expectations/rule_based_profiler/parameter_builder/parameter_builder.py

-            ].metric_values
-            if len(metric_value_kwargs) == 1
-            else attributed_resolved_metrics_map.values(),
+            list(attributed_resolved_metrics_map.values()),


This is a long method, it seems we can encapsulate much of what is happening here in MetricValues (have MetricValues responsible for itself rather than having ParameterBuilder manipulate it)

@donaldheppner These types are placeholder ones, which we intended to replace as part of a MAINTENANCE task (I just created a JIRA item for it). Thanks.

Shinnnyshinshin · 2022-03-16T01:05:27Z

...t_expectations/rule_based_profiler/parameter_builder/metric_multi_batch_parameter_builder.py

-            metric_values = metric_values[:, 0]
+        if (
+            reduce_scalar_metric
+            and len(metric_values) == 1


I would rather handle a well defined metric values type than introduce these complicated if statements. As a future refactor, can we encapsulate this logic in the MetricValues class?

@donaldheppner These types are placeholder ones, which led to needing this logic for certain ParameterBuilder implementations. It was intended to be replaced as part of a MAINTENANCE task (I just created a JIRA item for it). Thanks.

donaldheppner · 2022-03-16T13:04:33Z

...t_expectations/rule_based_profiler/parameter_builder/metric_multi_batch_parameter_builder.py

-            metric_values = metric_values[:, 0]
+        if (
+            reduce_scalar_metric
+            and len(metric_values) == 1


I would rather handle a well defined metric values type than introduce these complicated if statements. As a future refactor, can we encapsulate this logic in the MetricValues class?

donaldheppner · 2022-03-16T13:07:05Z

...t_expectations/rule_based_profiler/parameter_builder/metric_multi_batch_parameter_builder.py

+            and isinstance(metric_values[0], AttributedResolvedMetrics)
+            and metric_values[0].metric_values.shape[1] == 1
+        ):
+            metric_values = metric_values[0].metric_values[:, 0]

        return (


Can we change the return type of this method from Tuple(Any, dict) to Tuple(MetricValues, MetricComputationDetails)? Also, should MetricComputationDetailsbe an attribute ofMetricValues` so we can avoid the Tuple all together? Every time you return a Tuple, it's one more thing for the caller to worry about

@donaldheppner This can be done as part of replacing placeholder types with full-fledged classes. This was intended all along to be accomplished as part of a MAINTENANCE task (I just created a JIRA item for it). Thanks.

donaldheppner · 2022-03-16T13:10:53Z

great_expectations/rule_based_profiler/parameter_builder/parameter_builder.py

-            ].metric_values
-            if len(metric_value_kwargs) == 1
-            else attributed_resolved_metrics_map.values(),
+            list(attributed_resolved_metrics_map.values()),


This is a long method, it seems we can encapsulate much of what is happening here in MetricValues (have MetricValues responsible for itself rather than having ParameterBuilder manipulate it)

donaldheppner · 2022-03-16T13:12:09Z

...expectations/rule_based_profiler/parameter_builder/regex_pattern_string_parameter_builder.py

+            isinstance(metric_computation_result.metric_values, list)
+            and len(metric_computation_result.metric_values) == 1
+        ):
+            raise ge_exceptions.ProfilerExecutionError(


When does this occur? Is there a way to make this code path unreachable with earlier checks?

This is especially relevant since it is repeated across MANY parameter builders. Again, this could be great functionality to encapsulate in the MetricValues class

When does this occur? Is there a way to make this code path unreachable with earlier checks?

@donaldheppner This never occurs (just did this check for defensive programming).

Alex Sherstinsky added 2 commits March 15, 2022 17:55

insure proper indexing of parameter builder get metrics computation r…

2e5d876

…esults

Merge branch 'develop' into bugfix/GREAT-464/GREAT-498/GREAT-674/alex…

066fbfc

…sherstinsky/rule_based_profiler_fix_bug_one_regex_get_parameters_result_error-2022_03_15-52

alexsherstinsky requested a review from Shinnnyshinshin March 16, 2022 00:59

github-actions bot added the core-team label Mar 16, 2022

alexsherstinsky requested review from cdkini, NathanFarmer, anthonyburdi and donaldheppner March 16, 2022 00:59

Shinnnyshinshin approved these changes Mar 16, 2022

View reviewed changes

clean up

d8b5ac0

alexsherstinsky merged commit 6a7dd66 into develop Mar 16, 2022

alexsherstinsky deleted the bugfix/GREAT-464/GREAT-498/GREAT-674/alexsherstinsky/rule_based_profiler_fix_bug_one_regex_get_parameters_result_error-2022_03_15-52 branch March 16, 2022 02:57

donaldheppner reviewed Mar 16, 2022

View reviewed changes

Shinnnyshinshin mentioned this pull request Mar 16, 2022

[MAINTENANCE] Clean up of RegexPatternStringParameterBuilder tests to use unittests #4436

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX] Insure Proper Indexing of Metric Computation Results in ParameterBuilder #4426

[BUGFIX] Insure Proper Indexing of Metric Computation Results in ParameterBuilder #4426

alexsherstinsky commented Mar 16, 2022 •

edited

netlify bot commented Mar 16, 2022 •

edited

Shinnnyshinshin left a comment

Shinnnyshinshin Mar 16, 2022

donaldheppner Mar 16, 2022

alexsherstinsky Mar 16, 2022

Shinnnyshinshin Mar 16, 2022

donaldheppner Mar 16, 2022

alexsherstinsky Mar 16, 2022

donaldheppner Mar 16, 2022

donaldheppner Mar 16, 2022

alexsherstinsky Mar 16, 2022

donaldheppner Mar 16, 2022

donaldheppner Mar 16, 2022

donaldheppner Mar 16, 2022

alexsherstinsky Mar 16, 2022

[BUGFIX] Insure Proper Indexing of Metric Computation Results in ParameterBuilder #4426

[BUGFIX] Insure Proper Indexing of Metric Computation Results in ParameterBuilder #4426

Conversation

alexsherstinsky commented Mar 16, 2022 • edited

Scope

Changes proposed in this pull request:

Definition of Done

netlify bot commented Mar 16, 2022 • edited

Shinnnyshinshin 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

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

alexsherstinsky commented Mar 16, 2022 •

edited

netlify bot commented Mar 16, 2022 •

edited