Updated setup.cfg mypy flags and resolved related errors. #703

Sanketh7 · 2022-11-01T14:16:07Z

Added the following flags to setup.cfg. Run pre-commit install then pre-commit run -a to test these changes locally.

[mypy]
warn_return_any = True
warn_unused_configs = True
ignore_missing_imports = True

All of the warnings that needed to be resolved were because of warn_return_any. Some of my solutions are more elegant than others so let me know if you find a better way.

dataprofiler/profilers/base_column_profilers.py

dataprofiler/profilers/graph_profiler.py

dataprofiler/profilers/numerical_column_stats.py

taylorfturner · 2022-11-01T18:07:05Z

@Sanketh7 update branch on this to make sure tests still passing

taylorfturner

just questions... I'll also test for if np.nan does get seen as a float instead of Any on my end.

Need to review more too all the cast() statements

dataprofiler/data_readers/json_data.py

dataprofiler/data_readers/structured_mixins.py

dataprofiler/labelers/base_data_labeler.py

dataprofiler/data_readers/avro_data.py

dataprofiler/profilers/float_column_profile.py

dataprofiler/profilers/histogram_utils.py

taylorfturner · 2022-11-02T12:30:17Z

@Sanketh7 update branch ... there may be a couple new fixes needed too. My PSI #688 PR made a bunch of additions to numerical_column_stats.py

dataprofiler/profilers/base_column_profilers.py

dataprofiler/profilers/float_column_profile.py

… errors

taylorfturner

comments

dataprofiler/data_readers/structured_mixins.py

dataprofiler/profilers/float_column_profile.py

JGSweets · 2022-11-04T20:51:51Z

dataprofiler/profilers/numerical_column_stats.py

        )
        bin_counts_impose = bin_counts_impose_pos + bin_counts_impose_neg

        median_inds = np.abs(bin_counts_impose - 0.5) < 1e-10
        if np.sum(median_inds) > 1:
-            return np.mean(bin_edges_impose[median_inds])
+            return cast(float, np.mean(bin_edges_impose[median_inds]))


this would be a float64

I'm getting a similar issue as before:

"median_absolute_deviation": utils.find_diff_of_numbers( self.median_abs_deviation, other_profile.median_abs_deviation ),

dataprofiler/profilers/numerical_column_stats.py:381: error: Value of type variable "T" of "find_diff_of_numbers" cannot be "object"

Looking into it more, I believe it's because we're trying to assign a Union to a TypeVar with bound=Subtractable and the Union itself does not follow the Subtractable protocol. So far the only reasonable solution I can think of is doing:

"median_absolute_deviation": utils.find_diff_of_numbers( cast(float, self.median_abs_deviation), cast(float, other_profile.median_abs_deviation) ),

which isn't very ideal. I'll look into mypy generics more to see if there's a workaround.

Fixed. See #703 (comment)

JGSweets · 2022-11-07T16:41:52Z

dataprofiler/labelers/base_model.py

            cls, clsname, bases, attrs
        )
-        new_class._register_subclass()
+        new_class._register_subclass()  # type: ignore


I'm not 100% positive, but I think the bound above should be BaseModel which would fix this.

We should really try to avoid using these bandaid fixes with #type: ignore unless absolutely necessary

@Sanketh7 can we get rid of this #type: ignore?

Changing the typevar to BaseModel didn't end up fixing it because mypy doesn't detect the relationship between BaseModel and AutoSubRegistrationMeta (this caused super(AutoSubRegistrationMeta, cls) to error).

This has some interesting suggestions:
https://stackoverflow.com/questions/66121127/calling-new-on-an-any

However, if we can't do this, ultimately we should cast referring to the output as a derivation of BaseModel of some type. Returning it as AutoSubRegistrationMeta is not truly what matter since it is just a mixin.

https://stackoverflow.com/questions/63054541/how-to-type-the-new-method-in-a-python-metaclass-so-that-mypy-is-happy

This answer looks similar to what we're doing but they still end up doing a cast at the end. They also have a return type of the metaclass and not the derived class.

To be clear, I think that should go in a separate PR if we have to add anything to further validate. For now, I think as implemented is okay with regards to this PR getting merged.

Not that it should be BaseModel, T should be identified as T ultimately.

I'd recommend in order to get the rest of this merged, @Sanketh7, revert this change for the AutoSubRegistration and get it working to pass mypy checks. Then in a follow-up PR, we can resolve this specific issue more fully.

Reverted these changes so we can tackle it in another PR.

JGSweets · 2022-11-07T16:52:35Z

dataprofiler/profilers/numerical_column_stats.py

@@ -1842,7 +1857,7 @@ def is_int(x: str) -> bool:
            return a == b

    @staticmethod
-    def np_type_to_type(val: Any) -> Union[int, float]:
+    def np_type_to_type(val: Any) -> Union[int, float, Any]:


if this can be any, we technically don't need union, right?

However, will this error then bc we are using Any?

I guess the Union was redundant. Changing to Any actually doesn't cause issues with our mypy flag warn_return_any = True because that only complains when we return Any and the function return type is not Any.

Sanketh7 · 2022-11-07T23:07:41Z

dataprofiler/profilers/utils.py

@@ -412,7 +413,19 @@ def __sub__(self: T, other: T) -> Any:
 T = TypeVar("T", bound=Subtractable)


+@overload
+def find_diff_of_numbers(


Ended up going with an overload here. The issue is that there are a couple cases where we pass in a Union[float, np.float64] to find_diff_of_numbers and Unions don't satisfy the typevar bound.

nice -- I would recommend formatting slightly different based on the docs I was seeing here, but nice improvement

actually, disregard this. I'm seeing it both ways

I tried to format it like that but the pre-commit formatter would add the extra new lines.

Hmm, an object shouldn't be casted as Union[float, np.float64] but has the option to return as one or the other.

I wonder if this meant we did those returns wrong and should have done overloads on those funcs where it could return a float in one or np.float64 in the other.

The main reason we have the float case is because np.nan is float and some of our methods could return np.nan if certain conditions aren't met. From what I can see, a lot of these conditions can't be solved by typing overloads (for example, some are based on class fields and not method parameters).

At least for finding the diff between 2 Union[float, np.float64], we shouldn't run into issues because float gets promoted to float64 automatically.

Seems to be this problem:
https://stackoverflow.com/questions/73011336/different-behavior-of-mypy-with-generic-class-in-python-3-8-and-3-10

dataprofiler/profilers/numerical_column_stats.py

taylorfturner · 2022-11-08T14:58:45Z

dataprofiler/profilers/utils.py

@@ -412,7 +413,19 @@ def __sub__(self: T, other: T) -> Any:
 T = TypeVar("T", bound=Subtractable)


+@overload
+def find_diff_of_numbers(


nice -- I would recommend formatting slightly different based on the docs I was seeing here, but nice improvement

taylorfturner

LGTM -- let's create a follow-up issue for more fully resolving some of these comments in a subsequent, smaller PR

taylorfturner

follow-up PRs as discussed

updated setup.cfg mypy flags and resolved errors

a74ef96

Sanketh7 requested review from JGSweets, ksneab7, taylorfturner, micdavis and tyfarnan as code owners November 1, 2022 14:16

Sanketh7 commented Nov 1, 2022

View reviewed changes

dataprofiler/profilers/base_column_profilers.py Show resolved Hide resolved

Sanketh7 commented Nov 1, 2022

View reviewed changes

dataprofiler/profilers/graph_profiler.py Show resolved Hide resolved

Sanketh7 commented Nov 1, 2022

View reviewed changes

dataprofiler/profilers/numerical_column_stats.py Outdated Show resolved Hide resolved

taylorfturner assigned Sanketh7 Nov 1, 2022

taylorfturner added Bug Something isn't working static_typing mypy static typing issues High Priority Dramatic improvement, inaccurate calculation(s) or bug / feature making the library unusable labels Nov 1, 2022

Merge branch 'main' into mypy-flags

8b1d198

taylorfturner reviewed Nov 2, 2022

View reviewed changes

Sanketh7 added 2 commits November 2, 2022 10:24

Merge branch 'main' into mypy-flags

cb58600

updated return type of _is_each_row_float

1eb64ae

JGSweets reviewed Nov 2, 2022

View reviewed changes

dataprofiler/profilers/base_column_profilers.py Show resolved Hide resolved

taylorfturner reviewed Nov 2, 2022

View reviewed changes

dataprofiler/profilers/float_column_profile.py Outdated Show resolved Hide resolved

taylorfturner reviewed Nov 2, 2022

View reviewed changes

dataprofiler/profilers/float_column_profile.py Outdated Show resolved Hide resolved

taylorfturner removed the Bug Something isn't working label Nov 2, 2022

updated mypy hook to include numpy as a dependency and fixed relevant…

0ad2a38

… errors

taylorfturner reviewed Nov 2, 2022

View reviewed changes

dataprofiler/data_readers/structured_mixins.py Outdated Show resolved Hide resolved

dataprofiler/profilers/float_column_profile.py Outdated Show resolved Hide resolved

dataprofiler/profilers/float_column_profile.py Outdated Show resolved Hide resolved

updated _is_each_row_float

0313deb

taylorfturner enabled auto-merge (squash) November 2, 2022 22:03

updated _get_data_as_records

2d5307f

auto-merge was automatically disabled November 2, 2022 22:08
Head branch was pushed to by a user without write access

taylorfturner self-requested a review November 2, 2022 22:09

JGSweets reviewed Nov 4, 2022

View reviewed changes

Merge branch 'main' into mypy-flags

06762be

JGSweets reviewed Nov 7, 2022

View reviewed changes

Sanketh7 added 2 commits November 7, 2022 16:59

update np_type_to_type return type

25094c2

changed float to float64 where needed

1ee80eb

Sanketh7 commented Nov 7, 2022

View reviewed changes

taylorfturner reviewed Nov 8, 2022

View reviewed changes

Sanketh7 added 4 commits November 8, 2022 10:23

updated _estimate_mode_from_histogram to not use Union

a9d097b

Merge branch 'main' into mypy-flags

d7ee476

return 0.0 instead of 0

841adf5

Merge branch 'main' into mypy-flags

9c2d636

taylorfturner enabled auto-merge (squash) November 9, 2022 16:03

taylorfturner previously approved these changes Nov 9, 2022

View reviewed changes

taylorfturner requested a review from JGSweets November 9, 2022 16:40

revert AutoSubRegistrationMeta changes

78f57fa

auto-merge was automatically disabled November 9, 2022 20:24
Head branch was pushed to by a user without write access

Sanketh7 dismissed taylorfturner’s stale review via 78f57fa November 9, 2022 20:24

taylorfturner enabled auto-merge (squash) November 9, 2022 20:27

taylorfturner previously approved these changes Nov 9, 2022

View reviewed changes

Update base_model.py

ffbcb43

taylorfturner dismissed their stale review via ffbcb43 November 9, 2022 22:50

taylorfturner added 3 commits November 9, 2022 18:03

Update base_model.py

e417189

isort fix

ec2745b

DS_Store fix

e6399be

taylorfturner self-requested a review November 9, 2022 23:36

taylorfturner disabled auto-merge November 9, 2022 23:36

tyfarnan approved these changes Nov 9, 2022

View reviewed changes

JGSweets approved these changes Nov 9, 2022

View reviewed changes

JGSweets merged commit 5cf3784 into capitalone:main Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated setup.cfg mypy flags and resolved related errors. #703

Updated setup.cfg mypy flags and resolved related errors. #703

Sanketh7 commented Nov 1, 2022 •

edited by taylorfturner

Loading

taylorfturner commented Nov 1, 2022

taylorfturner left a comment •

edited

Loading

taylorfturner commented Nov 2, 2022 •

edited

Loading

taylorfturner left a comment

JGSweets Nov 4, 2022

Sanketh7 Nov 7, 2022

Sanketh7 Nov 8, 2022

JGSweets Nov 7, 2022

taylorfturner Nov 8, 2022

Sanketh7 Nov 8, 2022 •

edited

Loading

JGSweets Nov 8, 2022

Sanketh7 Nov 9, 2022

taylorfturner Nov 9, 2022 •

edited

Loading

JGSweets Nov 9, 2022

taylorfturner Nov 9, 2022

taylorfturner Nov 9, 2022

Sanketh7 Nov 9, 2022

JGSweets Nov 7, 2022

JGSweets Nov 7, 2022

Sanketh7 Nov 7, 2022

taylorfturner Nov 9, 2022

Sanketh7 Nov 7, 2022

taylorfturner Nov 8, 2022

taylorfturner Nov 8, 2022

Sanketh7 Nov 8, 2022

JGSweets Nov 8, 2022

Sanketh7 Nov 8, 2022

JGSweets Nov 8, 2022

taylorfturner Nov 8, 2022

taylorfturner left a comment

taylorfturner left a comment

Updated setup.cfg mypy flags and resolved related errors. #703

Updated setup.cfg mypy flags and resolved related errors. #703

Conversation

Sanketh7 commented Nov 1, 2022 • edited by taylorfturner Loading

taylorfturner commented Nov 1, 2022

taylorfturner left a comment • edited Loading

Choose a reason for hiding this comment

taylorfturner commented Nov 2, 2022 • edited Loading

taylorfturner 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

Sanketh7 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taylorfturner Nov 9, 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

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

taylorfturner left a comment

Choose a reason for hiding this comment

taylorfturner left a comment

Choose a reason for hiding this comment

Sanketh7 commented Nov 1, 2022 •

edited by taylorfturner

Loading

taylorfturner left a comment •

edited

Loading

taylorfturner commented Nov 2, 2022 •

edited

Loading

Sanketh7 Nov 8, 2022 •

edited

Loading

taylorfturner Nov 9, 2022 •

edited

Loading