Removed ABCs from pandas._typing #27424

WillAyd · 2019-07-16T20:33:16Z

closes Remove ABC Usage from pandas._typing #27146
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

This reveals a few new typing issues so won't pass CI but submitting as draft

Here's one way I think that works to avoid import cycles. Basically any internal imports in pandas._typing need to be set in a TYPE_CHECKING block to prevent their runtime evaluation from causing import cycles.

To illustrate the effectiveness like this if you put something like this at the end of pandas._typing:

def func(obj: FrameOrSeries) -> None:
    reveal_type(obj)

It would previously give you

pandas/_typing.py:43: note: Revealed type is 'Any'

Whereas now gives:

pandas/_typing.py:44: note: Revealed type is 'pandas.core.series.Series*'
pandas/_typing.py:44: note: Revealed type is 'pandas.core.frame.DataFrame*'

@topper-123 any way to check VSCode to see if this works with code completion?

In the long run this may not be needed with PEP 563:

https://www.python.org/dev/peps/pep-0563/

But I think this hack gets us there in the meantime

WillAyd · 2019-07-16T22:10:56Z

So the mypy complaints may be pointing to unexpected behavior with ExtensionArrays since casting is not part of their signature. To illustrate:

>>> import pandas as pd
>>> from pandas.core.dtypes.common import ensure_int_or_float

>>> ensure_int_or_float(pd.Series(range(3), dtype='Int64'))
0    0
1    1
2    2
dtype: int64

# Directly against PandasArray now
>>> ensure_int_or_float(pd.Series(range(3), dtype='Int64').array)
array([0., 1., 2.])  # returns float

Can ignore for now but bringing up in case it yields any insights @jreback @TomAugspurger

The IntervalIndex complaint may or may not be a bug as well @jschendel

topper-123 · 2019-07-16T22:31:26Z

I've tried it using reveal_type on on klass in pandas/groupby/generic.py, and it works.

PyCharm doesn't give code completions on this though, but that could possibly be that TypeVars are not implemented in PyCharm, maybe. I haven't myself gotten TypeVars to work in PyCharm, and I've tried several versions.

I don't use VSCode, so I don't if it gives completions there

pandas/_typing.py

setup.cfg

pandas/_typing.py

pandas/core/dtypes/common.py

WillAyd · 2019-07-20T20:09:18Z

Yea no problem - makes sense

…

Sent from my iPhone

On Jul 20, 2019, at 1:05 PM, Jeff Reback ***@***.***> wrote: @jreback commented on this pull request. In pandas/core/dtypes/common.py: > @@ -168,11 +168,11 @@ def ensure_int_or_float(arr: ArrayLike, copy=False) -> np.array: will remain unchanged. """ try: - return arr.astype("int64", copy=copy, casting="safe") + return arr.astype("int64", copy=copy, casting="safe") # type: ignore ok, so mypy is failing on this then? I agree might be a problem, but hiding it is not the answer (or if you do then pls open an issue and point to that issue so can address elsewhere). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

pep8speaks · 2019-07-21T17:21:47Z

Hello @WillAyd! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-24 15:11:11 UTC

simonjayhawkins · 2019-07-22T13:50:30Z

pandas/_typing.py

 FilePathOrBuffer = Union[str, Path, IO[AnyStr]]

-FrameOrSeries = TypeVar("FrameOrSeries", ABCSeries, ABCDataFrame)
+FrameOrSeries = TypeVar("FrameOrSeries", "Series", "DataFrame")


Suggested change

FrameOrSeries = TypeVar("FrameOrSeries", "Series", "DataFrame")

FrameOrSeries = Union["Series", "DataFrame"]

quote from https://mypy.readthedocs.io/en/latest/generics.html...
"User-defined generics are a moderately advanced feature and you can get far without ever using them..."

Thanks but this just loosens the type system rather than actually fixing anything. TypeVar is going to be generally more useful for checking functions that can be fully generic in nature.

Might just change the return of this one and see how many others require Union in the future

makes sense. Union[Series, DataFrame] might be better written as NDFrame anyway?

Also the "user-defined generics" you are referring to are more applicable to containers not TypeVars. Right now we just use a blanket Series as a return object, though in the future we could do something like Series[int] and Series[str], etc...; the Series would be the user-defined generic in that case

The TypeVar in the docs you linked is just a way of parametrizing that user-defined generic, so that a Series[int] would have to stay as a Series[int] through it's lifecycle; without that parametrization we allow Series[int] to become Series[str] without any complaints from mypy today

We are probably a ways off of doing user-defined generics but this is great that you looked into it. Certainly open to ideas on that front if you think of a good way to implement as we get more familiar with these annotations

makes sense. Union[Series, DataFrame] might be better written as NDFrame anyway?

Hmm that would work though we don't typically import NDFrame anywhere so I don't think want to start here

I would leave as FrameOrSeries as its more descriptive

simonjayhawkins · 2019-07-22T13:54:00Z

@WillAyd

taken a quick look as requested in #27512 (comment)

There's some failures showing up in there in the io.formats file so may uncover some others here as well since that's a central module getting touched

there is only one mypy error pandas/core/window.py:266: error: Incompatible return value type (got "Series", expected "DataFrame") and that can be fixed by using a Union instead of TypeVar.

simonjayhawkins · 2019-07-22T16:24:06Z

pandas/core/indexes/interval.py

@@ -906,35 +906,35 @@ def get_indexer(
            )
            raise InvalidIndexError(msg)

-        target = ensure_index(target)
+        target_as_index = ensure_index(target)


if you also make AnyArrayLike an Alias instead of a TypeVar, then this module doesn't need to be changed at all.

Same thing though - this just loosens the type checking which isn't desired. Actually moved towards TypeVars for reasons described in #26453 (comment)

Might update #27050 to include some of that info

Actually moved towards TypeVars for reasons described in #26453 (comment)

i don't think that applies here.

target does not need to be a TypeVar, the type of target does not need to be maintained.

target can be any of ["ExtensionArray", "Index", "Series", "SparseSeries", np.ndarray]

ensure_index is not yet typed. but presumably will have a return type of Index.

so reassigning with an Index to a variable that has a type of Union of ["ExtensionArray", "Index", "Series", "SparseSeries", np.ndarray] is perfectly valid.

We could add Union alternatives for each TypeVar in the central module but I think that confounds the point of generic programming and/or makes our type system weaker. Another option would be to allow redefinition of variables which mypy supplies a setting for:

https://mypy.readthedocs.io/en/latest/command_line.html?highlight=allow-redefinition#miscellaneous-strictness-flags

But I also think that makes for a weaker type system, and generally there's not a lot of downside to creating a separate variable here instead of allowing it's type to implicitly be altered by the return of ensure_index

Another option would be to allow redefinition of variables which mypy supplies a setting for:

https://mypy.readthedocs.io/en/latest/command_line.html?highlight=allow-redefinition#miscellaneous-strictness-flags

But I also think that makes for a weaker type system

definitely. we should rule this out.

generally there's not a lot of downside to creating a separate variable here instead of allowing it's type to implicitly be altered by the return of ensure_index

disagree. the return type of ensure_index would need to conform to the type of target.

is creating new variables not weakening the type system?

WillAyd · 2019-07-22T16:28:17Z

@simonjayhawkins thanks for checking these. Actually was getting an error locally in io.formats which is why I pinged you on this but doesn't appear on CI here so might have just been a local merge issue. I'll push an update to the issue here today to see how it looks

WillAyd · 2019-07-22T17:13:03Z

pandas/core/window.py

@@ -240,7 +240,7 @@ def _prep_values(self, values: Optional[np.ndarray] = None) -> np.ndarray:

        return values

-    def _wrap_result(self, result, block=None, obj=None) -> FrameOrSeries:
+    def _wrap_result(self, result, block=None, obj=None):


Not ideal to remove FrameOrSeries here but mypy was complaining since this function is not fully generic (i.e. a conditional branch could determine the returned object)

Options discussed were:

Use Union["Series, "DataFrame"] as a return type

Use NDFrame as a return type

Remove

I looked at option 1 first it caused a lot of flake8 complaints with other imports in the module where we import Series directly within a function, so would add a lot of noqa directives to the file to make it work. Option 2 was suggested by @simonjayhawkins and would be viable but I figured just remove for now and can open up a follow up issue to try and make this function generic.

It was type checking to Any before this change anyway so we aren't loosing any inference. Open to thoughts

It was type checking to Any before this change anyway so we aren't loosing any inference.

adding Any (or not typing) is not so bad. There is a network effect benefit from typing, so the quicker we add type hints to modules, the quicker we see a benefit.

having Any can easily be picked up in a future pass with one of many cli flags to mypy.

so I don't think any PRs adding type annotations should be held pending perfection (and especially unrelated PRs with "Any interest in adding an annotation while you are looking at this?" . xref #26795)

Just so we are on the same page anything without an annotation is implicitly Any so typing it as such doesn't change anything.

How do you view the Any annotations currently in the code? I interpret that as "this has already been reviewed and really should allow anything" versus "I just added Any to get this to pass". I don't see a lot of value add to the latter since that's what happens implicitly without an annotation anyway but certainly a point for discussion

I interpret that as "this has already been reviewed and really should allow anything" versus "I just added Any to get this to pass". I don't see a lot of value add to the latter since that's what happens implicitly without an annotation

+1 for this. I strongly prefer to leave something un-typed as an invitation to the next person to try to figure it out.

+1 for this. I strongly prefer to leave something un-typed as an invitation to the next person to try to figure it out.

just done an experiment.. it appears that Any is considered a type annotation and so the code gets checked, whereas leaving it out causes the type checker to not check the code.

so -1 for not adding Any to un-typed function signatures.

What's a case where having a function typed with Any everywhere is valuable?

What's a case where having a function typed with Any everywhere is valuable?

not everywhere!

just where they are currently omitted because the union is of too many types. such as key in indexing.

it appears that Any is considered a type annotation and so the code gets checked, whereas leaving it out causes the type checker to not check the code.

Have to double check but it's not the field you were actually annotated that throws the error as much as something else right? I think mypy might skip a function if it has no annotations so adding Any may cause it to throw errors for inferences of local variables it can't make.

With that said I really don't think adding Any is a great approach. If we wanted the former there is a --check-untyped-def flag we could enable.

This thread is probably best as a separate issue

Have to double check but it's not the field you were actually annotated that throws the error as much as something else right? I think mypy might skip a function if it has no annotations so adding Any may cause it to throw errors for inferences of local variables it can't make.

that's correct. or even incorrect calls to typed functions.

With that said I really don't think adding Any is a great approach.

disagree.

If we wanted the former there is a --check-untyped-def flag we could enable.

ok locally for a module, but how to set-up on the CI? if we are typing gradually we are not yet ready for this.

Adding Any to a function, indicates the body of that function is ready to be type checked.

This thread is probably best as a separate issue

i'm opening another PR shortly, where Any is added for all unions of 5 or more items. https://monkeytype.readthedocs.io/en/latest/generation.html#monkeytype.typing.RewriteLargeUnion so we can continue the discusion there.

pandas/_typing.py

jreback · 2019-07-23T22:20:03Z

@WillAyd can you rebase

jreback · 2019-07-24T12:09:08Z

@WillAyd lgtm. can you rebase once more (as merged bunch of things). looks good to merge (I know you had some open quesitons, but if ok can address in followup)

WillAyd · 2019-07-24T16:43:02Z

Thanks for review @jreback and @simonjayhawkins

Removed ABCs from pandas._typing

9c6e616

WillAyd added the Typing type annotations, mypy/pyright type checking label Jul 16, 2019

Fix CI

6490263

WillAyd marked this pull request as ready for review July 16, 2019 23:20

Fixed type ignore location

cce9afb

jbrockmendel reviewed Jul 17, 2019

View reviewed changes

pandas/_typing.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Jul 17, 2019

View reviewed changes

pandas/_typing.py Outdated Show resolved Hide resolved

WillAyd added 2 commits July 17, 2019 09:07

Merge remote-tracking branch 'upstream/master' into remove-abcs

1fe6059

Excluded from coveraged and updated tslib imports

5f2fa6a

jreback reviewed Jul 18, 2019

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

jreback reviewed Jul 20, 2019

View reviewed changes

pandas/_typing.py Outdated Show resolved Hide resolved

jreback requested changes Jul 20, 2019

View reviewed changes

pandas/core/dtypes/common.py Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into remove-abcs

72d3e2c

WillAyd mentioned this pull request Jul 21, 2019

ensure_int_or_float Surprising Behavior with Int64 / ExtensionArrays #27506

Closed

WillAyd added 3 commits July 21, 2019 09:40

Merge branch 'remove-abcs' of github.com:WillAyd/pandas into remove-abcs

c3bd6df

Comments in typing

5a8d35a

Removed shadowing of target variable

5889715

Blackify

ed2ee7f

WillAyd mentioned this pull request Jul 22, 2019

more type hints for io/formats/format.py #27512

Merged

simonjayhawkins reviewed Jul 22, 2019

View reviewed changes

Removed failing annotation

095aed4

WillAyd commented Jul 22, 2019

View reviewed changes

simonjayhawkins reviewed Jul 22, 2019

View reviewed changes

pandas/_typing.py Outdated Show resolved Hide resolved

Removed unused imports

4bf254c

Merge remote-tracking branch 'upstream/master' into remove-abcs

8f4a7a1

jreback added this to the 1.0 milestone Jul 24, 2019

jreback approved these changes Jul 24, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into remove-abcs

5e77b75

WillAyd merged commit 3b96ada into pandas-dev:master Jul 24, 2019

WillAyd deleted the remove-abcs branch July 24, 2019 16:43

quintusdias pushed a commit to quintusdias/pandas_dev that referenced this pull request Aug 16, 2019

Removed ABCs from pandas._typing (pandas-dev#27424)

1ef0861

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed ABCs from pandas._typing #27424

Removed ABCs from pandas._typing #27424

WillAyd commented Jul 16, 2019

WillAyd commented Jul 16, 2019

topper-123 commented Jul 16, 2019

WillAyd commented Jul 20, 2019 via email

pep8speaks commented Jul 21, 2019 •

edited

Loading

simonjayhawkins Jul 22, 2019

WillAyd Jul 22, 2019

simonjayhawkins Jul 22, 2019

WillAyd Jul 22, 2019

WillAyd Jul 22, 2019

jreback Jul 23, 2019

simonjayhawkins commented Jul 22, 2019

simonjayhawkins Jul 22, 2019

WillAyd Jul 22, 2019

simonjayhawkins Jul 22, 2019

WillAyd Jul 22, 2019

simonjayhawkins Jul 22, 2019

WillAyd commented Jul 22, 2019

WillAyd Jul 22, 2019

simonjayhawkins Jul 22, 2019

WillAyd Jul 22, 2019

jbrockmendel Jul 22, 2019

TomAugspurger Jul 22, 2019

simonjayhawkins Jul 22, 2019

TomAugspurger Jul 22, 2019

simonjayhawkins Jul 22, 2019

WillAyd Jul 22, 2019

simonjayhawkins Jul 22, 2019

jreback commented Jul 23, 2019

jreback commented Jul 24, 2019

WillAyd commented Jul 24, 2019

	FrameOrSeries = TypeVar("FrameOrSeries", "Series", "DataFrame")
	FrameOrSeries = Union["Series", "DataFrame"]

Removed ABCs from pandas._typing #27424

Removed ABCs from pandas._typing #27424

Conversation

WillAyd commented Jul 16, 2019

WillAyd commented Jul 16, 2019

topper-123 commented Jul 16, 2019

WillAyd commented Jul 20, 2019 via email

pep8speaks commented Jul 21, 2019 • edited Loading

Comment last updated at 2019-07-24 15:11:11 UTC

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

simonjayhawkins commented Jul 22, 2019

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

WillAyd commented Jul 22, 2019

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

jreback commented Jul 23, 2019

jreback commented Jul 24, 2019

WillAyd commented Jul 24, 2019

pep8speaks commented Jul 21, 2019 •

edited

Loading