CLN: remove util._decorators.make_signature and make related changes #26819

topper-123 · 2019-06-12T20:56:14Z

util._decorators.make_signature is only used in core.groupby.base.whitelist_method_generator.

However, in that function it is only used inside an if statement, that is conditional upon an attribute being a method. That however can not happen, as the function parameter only takes classes as an input, so that if-statement is never true and its content never executed. The net effect is that a nice chunk of code can just be removed, which is nice.

In the future, we should just use jus use inspect.signature or inspect.getfullargspecs for signature inspection.

codecov · 2019-06-12T21:10:44Z

Codecov Report

Merging #26819 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26819      +/-   ##
==========================================
+ Coverage   91.86%   91.86%   +<.01%     
==========================================
  Files         179      179              
  Lines       50700    50679      -21     
==========================================
- Hits        46576    46557      -19     
+ Misses       4124     4122       -2

Flag	Coverage Δ
#multiple	`90.45% <100%> (ø)`	⬆️
#single	`41.13% <100%> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
pandas/util/_decorators.py	`92.13% <ø> (+0.78%)`	⬆️
pandas/core/groupby/base.py	`100% <ø> (+8.16%)`	⬆️
pandas/core/groupby/generic.py	`89.33% <100%> (+0.16%)`	⬆️
pandas/_typing.py	`100% <100%> (ø)`	⬆️
pandas/io/gbq.py	`88.88% <0%> (-11.12%)`	⬇️
pandas/core/frame.py	`96.88% <0%> (-0.12%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8374a1d...bf07b51. Read the comment docs.

topper-123 · 2019-06-12T22:23:10Z

The failures are unrelated to this PR

topper-123 · 2019-06-12T22:53:18Z

pandas/tests/util/test_util.py

-                       ["old_arg_name", "new_arg_name",
-                        "mapping", "stacklevel"]))
-])
-def test_make_signature(func, expected):


It can be noted that validate_kwargs and deprecate_kwarg have their own test modules, so removing here it not an issue.

WillAyd · 2019-06-13T01:17:35Z

Makes sense to me. I think merging master should get rid of the failure.

Also annotations while you are looking at this would be great if they are easy to add

topper-123 · 2019-06-13T09:11:01Z

I've rebased and added type hints.

Wrt. the type hints, I've added the typing specific internal imports inside a if TYPE_CHECKING statement in order to minimize the risk of circular import problems. I'm however not 100 % strong in correct typing usage, so I see that as point for discussion, not ny final view on pure type importing.

WillAyd · 2019-06-13T11:15:06Z

pandas/core/groupby/base.py


 from pandas.core.dtypes.common import is_list_like, is_scalar

+if TYPE_CHECKING:


Was this actually causing circular imports? We import DataFrame and Series elsewhere within this directory didn't think those would cause issues.

Would like to avoid the TYPE_CHECKING variable generally

Note this probably won't matter in light of other comments below

No, it wasn't. Just my nervousness that it might cause one in a future commit.

as an FYI you can always use the quoted form of these rather than direct imports, e.g. for 'GroupBy'

I don't understand you here. Could you be more explicit?

where a type is expected, for example DataFrame, you can simply use the string 'DataFrame' rather than importing it (this only works for types :->) e.g.

def func(df: 'DataFrame) -> 'DataFrame': ...

is legit. Of course we prefer to import, but we need to avoid circular deps (e.g. in groupby in particular).

Yes, it's legit, but it won't be infer correctly without the import?

At least in PyCharm it needs an import to recognize the type hint types do proper code completions, even when quoted.

EDIT: Likewise, mypy will complain if the imports aren't there.

I've checked again, and actually if I remove the if TYPE_CHECKING statement, it will cause a circular import error.

So I think the if-statement must remain in order to do type checking. Also, the DataFrame rather than ABCDataFrame gives better results for PyCharm, without negativ results for mypy, so I propose keeping it that way.

Although adding type hints does feel like walking in uncharted territory as far as figuring out what best-practice is.

Yea I think this will be required for "GroupBy" since it is not in the same module. FWIW you can do this by putting reveal_type(base) in the body of whitelist_method_generator.

Just trying Type['GroupBy'] as the annotation without this import yields the below errors:

$ mypy pandas/core/groupby/base.py pandas/core/groupby/base.py:93: error: Name 'GroupBy' is not defined pandas/core/groupby/base.py:124: error: Revealed type is 'Type[Any]'

Whereas Type[GroupBy] with this import for mypy infers correctly:

$ mypy pandas/core/groupby/base.py pandas/core/groupby/base.py:127: error: Revealed type is 'Type[pandas.core.groupby.groupby.GroupBy]'

So I think this is correct for GroupBy (separate conversations still for DataFrame and Series)

WillAyd · 2019-06-13T11:16:27Z

pandas/core/groupby/base.py

@@ -93,19 +94,21 @@ def _gotitem(self, key, ndim, subset=None):
                                   'idxmax'])


-def whitelist_method_generator(base, klass, whitelist):
+def whitelist_method_generator(base: 'Type[GroupBy]',


While this is probably just used for GroupBy probably OK to just type this as Type, i.e. it could theoretically accept any Type not just a GroupBy

this is pretty descriptive here, if the more specific types work that is good

OK sure. Though I think should be Type['GroupBy'] then (note difference in quoting)

My preference is an all-or-nothing approach to quoting. Else it's more work to check if a annotation is quoted or not.

Not sure I totally follow your point but the intention of quoting is to provide forward references to objects before they are defined. There's no reason to do this with builtin typing objects

Here's the mypy documentation on that. You'll see a particular example where they populate a List with a forward reference, where the quoting only apply to the type variable. That's what I'm asking for here:

https://mypy.readthedocs.io/en/latest/kinds_of_types.html#class-name-forward-references

Though may not even want this at all:

String literal types must be defined (or imported) later in the same module. They cannot be used to leave cross-module references unresolved. (For dealing with import cycles, see Import cycles.)

Don't need this to be a blocker

Another option here if we don't like the import machinery is to just move this to generic where it is actually used. I don't see a very strong reason why this needs to be in base

I've moved whitelist_method_generator to generic.py. That does make the typing simpler. I've also added the TypeVar in _typing.

However, I suggest that unless we're fixing relatively simple typing issues (no new defined types in _typing.py etc.), that typing changes in the future go in separate commits?

separate commits are certainly fine for typing but not a PR unless major work needs to be done; we get much more mileage out of adding typing as we going in changed code (granted sometimes this will be slightly painful at the beginning)

WillAyd · 2019-06-13T11:19:31Z

pandas/core/groupby/base.py

@@ -93,19 +94,21 @@ def _gotitem(self, key, ndim, subset=None):
                                   'idxmax'])


-def whitelist_method_generator(base, klass, whitelist):
+def whitelist_method_generator(base: 'Type[GroupBy]',
+                               klass: 'Union[Type[DataFrame], Type[Series]]',


This should probably be a TypeVar instead of a Union (i.e. we parametrize on one or the other of these, but don't use interchangeably within the function).

This is going to be a common idiom - could you add FrameOrSeries = TypeVar('FrameOrSeries', ABCSeries, ABCDataFrame) to pandas._typing and use that here instead (open to other naming suggestions)

I don't understand TypeVars all too well, but my understanding is it it ensures that the same type comes out as came in. So, if a Series came in, and an object of the same TypeVar comes out, that means the returnm type is a Series (and not DataFrame). In that case, it will not matter in this case if we use Union or TypeVar, isn't that true?

Not opposed to using TypeVar, just trying to understand their use cases.

Yea you've got that right! This comment might also be helpful

#26453 (comment)

So I don't think will make a difference here - just looking to get some kind of consistency in place as our annotations are still relatively immature

Ok. It will be actually be more concise to use that TypeVar here (fewer type-specific imports), and a TypeVar('FrameOrSeries', Series, DataFrame) would make sense for Pandas in general.

In Pycharm TypeVar('FrameOrSeries', ABCSeries, ABCDataFrame) doesn't give the correct completions, while TypeVar('FrameOrSeries', Series, DataFrame) does give the right ones. any specific reason to use ABCSeries and ABCDataFrame over the actual classes?

I'm also thinking that the best version would be actually TypeVar('FrameOrSeries', NDFrame, Series, DataFrame), to be able to add type relevant hints to the NDFrame class. Any thoughts on this?

CORRECTION: TypeVar('FrameOrSeries', Series, DataFrame) doesn't give the right completions either, so that point doesn't matter. It still stands that ABCDataFrame does not give any completions, while using DataFrame as a type hint does give the correct completions.

CORRECTION no. 2: the above means that Union[DataFrame, Series] currently gives better completions in PyCharn than TypeVar. Is this just a limitation of PyCharm, or is my understanding of TyprVar wrong? Anyone using VSCode can say if they get the correct type hints from using TypVars?

ABCSeries and ABCDataFrame are supposed to circumvent issues with import cycles. Unfortunately I don't use PyCharm so I can't speak to their support for those w.r.t. code completion.

I also don't think NDFrame should be a part of that as it's not a type that is used directly (i.e. typically only used as a superclass)

ABCSeries and ABCDataFrame are supposed to circumvent issues with import cycles.

To clarify though, if you see a better way of doing this certainly open to suggestions. I can't claim total expertise in our comprehensive usage of the ABC classes. @jreback

@WillAyd, do you use VSCode? Do you get correct type hints from TypeVars?

No. Haven't used annotations for code completion so any insights you can offer there would be appreciated

topper-123 · 2019-06-13T19:40:36Z

Given that a Union works as well as a TypeVar in this case, and the required TypeVar would actually be TypeVar('FrameOrSeries', Type[ABCSeries], Type[ABCDataFrame]) (notice the extra Type), and this TypeVar would be used very infrequently, I propose just accepting it like is it ATM (a Union).

WillAyd · 2019-06-13T22:47:37Z

Might be mixing up a few things here. The TypeVar would look as follows:

FrameOrSeries = TypeVar('FrameOrSeries', ABCSeries, ABCDataFrame)

The annotation in question would then be Type[FrameOrSeries] as opposed to Type[Union[Series, DataFrame]]] as is now (perhaps some slight variations).

WillAyd · 2019-06-14T12:30:08Z

Change looks good not sure what's going on with Travis / NumPy failure

jreback · 2019-06-14T12:35:09Z

pandas/_typing.py

@@ -22,3 +22,5 @@
                             Timedelta)
 Dtype = Union[str, np.dtype, ExtensionDtype]
 FilePathOrBuffer = Union[str, Path, IO[AnyStr]]


@WillAyd we should put a line comment on when to use these / what these are if not completely obvious (and then maybe even so).

future PR of course

Yea I'm going to take another look at this module soon to see if there's a better way of doing things. Also wondering if we should use if TYPE_CHECKING imports in that module instead of the ABCs. Will check it out and post separately

Yea I'm going to take another look at this module soon to see if there's a better way of doing things. Also wondering if we should use if TYPE_CHECKING imports in that module instead of the ABCs. Will check it out and post separately

-1 on using TYPE_CHECCKING. I think the way you have it setup is very nice actually, unless I am missing something big.

My concern is that the ABC* classes aren't getting type checked correctly. Adding reveal_type(klass) to this PR where the TypeVar is used yields the following:

$ mypy pandas/core/groupby/generic.py pandas/core/groupby/generic.py:83: error: Revealed type is 'Type[Any]'

That coupled with @topper-123 's comment around code completion makes me think the ABCs might all be Type[Any] since they are generated by create_pandas_abc_type, a function without any annotations itself (so implicitly Type[Any])

Going to investigate and open a separate issue accordingly

ok fair. I just don't want to introduce overhead for contributors, which typing especially with 'non-standard' things (e.g. cast) can do; we really want to minimize what people have to do (nothwithstanding maybe we have magic that makes things easy).

jreback · 2019-06-14T12:36:51Z

lgtm. I restarted the job, should pass. merge on green.

jreback · 2019-06-14T14:22:46Z

thanks @topper-123

topper-123 force-pushed the remove_make_signature branch from b945b74 to 3b9bcbe Compare June 12, 2019 20:56

gfyoung added the Clean label Jun 12, 2019

topper-123 force-pushed the remove_make_signature branch from 3b9bcbe to a0a15b4 Compare June 12, 2019 21:31

topper-123 commented Jun 12, 2019

View reviewed changes

WillAyd added this to the 0.25.0 milestone Jun 13, 2019

topper-123 force-pushed the remove_make_signature branch 2 times, most recently from c3841fa to ddcf052 Compare June 13, 2019 08:07

WillAyd requested changes Jun 13, 2019

View reviewed changes

WillAyd added the Typing type annotations, mypy/pyright type checking label Jun 14, 2019

topper-123 added 2 commits June 14, 2019 12:04

CLN: remove util._decorators.make_signature and make related changes

7941788

Add typing information

0833dd6

topper-123 force-pushed the remove_make_signature branch from ddcf052 to 17aa408 Compare June 14, 2019 10:37

moved base.whitelist_method_generator to .generic

bf07b51

topper-123 force-pushed the remove_make_signature branch from 17aa408 to bf07b51 Compare June 14, 2019 11:07

jreback reviewed Jun 14, 2019

View reviewed changes

jreback merged commit 430f0fd into pandas-dev:master Jun 14, 2019

topper-123 deleted the remove_make_signature branch June 14, 2019 15:54


		from pandas.core.dtypes.common import is_list_like, is_scalar

		if TYPE_CHECKING:

CLN: remove util._decorators.make_signature and make related changes #26819

CLN: remove util._decorators.make_signature and make related changes #26819

Conversation

topper-123 commented Jun 12, 2019 • edited Loading

codecov bot commented Jun 12, 2019 • edited Loading

Codecov Report

topper-123 commented Jun 12, 2019

Choose a reason for hiding this comment

WillAyd commented Jun 13, 2019

topper-123 commented Jun 13, 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

topper-123 Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd Jun 13, 2019 • 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

WillAyd Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

topper-123 Jun 14, 2019 • 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

topper-123 Jun 13, 2019 • 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

topper-123 commented Jun 13, 2019

WillAyd commented Jun 13, 2019

WillAyd commented Jun 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jun 14, 2019

jreback commented Jun 14, 2019

topper-123 commented Jun 12, 2019 •

edited

Loading

codecov bot commented Jun 12, 2019 •

edited

Loading

topper-123 Jun 13, 2019 •

edited

Loading

WillAyd Jun 13, 2019 •

edited

Loading

WillAyd Jun 13, 2019 •

edited

Loading

topper-123 Jun 14, 2019 •

edited

Loading

topper-123 Jun 13, 2019 •

edited

Loading

jreback Jun 14, 2019 •

edited

Loading