TYP: fix reportInvalidTypeVarUse from pyright #42367

twoertwein · 2021-07-04T04:26:25Z

This fixes the following warning from pyright: TypeVar "..." appears only once in generic function signature (reportInvalidTypeVarUse).

It would be great to enable pyright on the CI. It takes only ~90sec to run pyright on the entire codebase with the following configuration in pyproject.toml (ignores most type errors for now):

[tool.pyright]
pythonVersion = "3.8"
include = ["pandas"]
ignore = ["pandas/tests"]
reportOptionalMemberAccess = false
reportOptionalOperand = false
reportGeneralTypeIssues = false
reportMissingImports = false
reportUnboundVariable = false
reportMissingModuleSource = false
reportOptionalSubscript = false
reportOptionalCall = false
reportOptionalIterable = false
reportPropertyTypeMismatch = false

The following errors would need to be taken care of before then:
pandas/pandas/core/frame.py:4575:9 - error: Method "_reindex_axes" cannot override final method defined in class "NDFrame"
pandas/pandas/core/frame.py:10736:9 - error: Method "mask" cannot override final method defined in class "NDFrame"
pandas/pandas/core/series.py:5449:9 - error: Method "mask" cannot override final method defined in class "NDFrame"
pandas/pandas/core/computation/expr.py:268:26 - error: TypeVar bound type cannot be generic
pandas/pandas/core/groupby/generic.py:676:9 - error: Method "describe" cannot override final method defined in class "GroupBy"
pandas/pandas/core/groupby/generic.py:856:9 - error: Method "pct_change" cannot override final method defined in class "GroupBy"

simonjayhawkins · 2021-07-04T10:33:09Z

Thanks @twoertwein for the PR. I have merged just merged #41955, so need to change FrameOrSeriesUnion -> DataFrame | Series

A bit of history here...

originally we just had the FrameOrSeries alias and later needed the FrameOrSeriesUnion alias

so some uses of FrameOrSeries remain where only FrameOrSeriesUnion was needed.

I think mypy does pick up some cases where FrameOrSeries (i.e. TypeVar) is used and is unbounded, but not all.

pandas/core/arrays/_mixins.py

pandas/core/arrays/interval.py

pandas/core/generic.py

pandas/core/resample.py

twoertwein · 2021-07-04T16:39:33Z

I should have replaced FrameOrSeries with NDFrame and not with Series | DataFrame as

FrameOrSeries = TypeVar("FrameOrSeries", bound="NDFrame")

If FrameOrSeries is only ever used for DataFrames and Series, it should probably be

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

simonjayhawkins · 2021-07-04T17:10:32Z

If FrameOrSeries is only ever used for DataFrames and Series, it should probably be

originally, that was what was used. but that created issues with core.generic (and some of the groupby IIRC)

I should have replaced FrameOrSeries with NDFrame and not with Series | DataFrame as

probably only in needed in core.generic and maybe groupby. elsewhere Series | DataFrame is probably fine.

simonjayhawkins · 2021-07-04T17:16:19Z

I should have replaced FrameOrSeries with NDFrame and not with Series | DataFrame as
FrameOrSeries = TypeVar("FrameOrSeries", bound="NDFrame")
If FrameOrSeries is only ever used for DataFrames and Series, it should probably be
FrameOrSeries = TypeVar("FrameOrSeries", DataFrame, Series)

There is another subtle difference which maybe significant in some situations. TypeVar("FrameOrSeries", bound="NDFrame") maintains types for subclasses, whereas the TypeVar("FrameOrSeries", DataFrame, Series) variant does not. Internally, this is not really an issue, but for parts of the public api, this difference could be important to a group of users.

twoertwein · 2021-07-04T23:24:50Z

TypeVar("FrameOrSeries", bound="NDFrame") maintains types for subclasses, whereas the TypeVar("FrameOrSeries", DataFrame, Series) variant does not. Internally, this is not really an issue, but for parts of the public api, this difference could be important to a group of users.

Thanks for all the information! I will prefer NDFrame in public functions and see whether that leaves any DataFrame | Series (currently it is the opposite way: NDFrame where necessary - I will change that).

pandas/core/window/rolling.py

simonjayhawkins

Thanks @twoertwein generally lgtm.

pandas/core/groupby/grouper.py

pandas/_libs/tslibs/dtypes.pyi

pandas/_testing/_io.py

pandas/core/groupby/grouper.py

pandas/core/reshape/tile.py

pandas/core/generic.py

pandas/io/pytables.py

twoertwein · 2021-07-06T19:13:11Z

I think this PR is ready from my perspective (happy to remove the pyright config in pyproject.toml). Let me know what you think @simonjayhawkins

simonjayhawkins

Thanks @twoertwein comment about comments for ignores otherwise lgtm.

simonjayhawkins · 2021-07-07T11:53:32Z

pandas/core/groupby/grouper.py

@@ -297,7 +303,7 @@ def ax(self) -> Index:
            raise ValueError("_set_grouper must be called before ax is accessed")
        return index

-    def _get_grouper(self, obj: FrameOrSeries, validate: bool = True):
+    def _get_grouper(self, obj: NDFrame, validate: bool = True):


I suspect that this Typevar may be needed to maintain the return type, but the return is not typed, so I guess ok to use NDFrame for now to silence pyright.

You are right! I think the return type should be tuple[Any, ops.BaseGrouper, FrameOrSeries] I'm not sure what self.binner is supposed to be.

Mypy thinks that all three variables should be None. So I would need to add an ignore comment.

I "partially" typed _get_grouper

def _get_grouper( self, obj: FrameOrSeries, validate: bool = True ) -> tuple[Any, ops.BaseGrouper, FrameOrSeries]:

The Any is not great, but it is definitely better than before.

pandas/core/generic.py

simonjayhawkins · 2021-07-07T13:40:37Z

@twoertwein can you avoid squashing and force pushing, it makes reviewing updates more difficult.

github-actions · 2021-08-07T00:02:22Z

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

twoertwein · 2021-08-07T20:33:50Z

Rebased (but this time not squashed)

alimcmaster1 · 2021-08-31T22:37:53Z

@twoertwein whenever you have a second mind merging master

twoertwein · 2021-09-01T01:11:44Z

@alimcmaster1 rebased. Let me know if I should remove some parts that might be controversial (pyright settings in pyproject.toml?).

jreback · 2021-09-20T13:14:41Z

It would be great to enable pyright on the CI. It takes only ~90sec to run pyright on the entire codebase with the following configuration in pyproject.toml (ignores most type errors for now):

@twoertwein can you setup this?

twoertwein · 2021-09-20T14:54:19Z

It would be great to enable pyright on the CI. It takes only ~90sec to run pyright on the entire codebase with the following configuration in pyproject.toml (ignores most type errors for now):

@twoertwein can you setup this?

I will look into that. Should that be part of this PR or a followup (it might take me some time to play around with the CI).

Where in the CI should pyright be integrated (I assume in "CI / Checks")? Should pyright also be part of the pre-commit? There doesn't seem to be a pre-commit interface from pyright, except this third-party wrapper: https://github.com/necaris/pre-commit-pyright.

Other points to consider:

Pyright depends on node.js
Pyright fixes bugs at a rapid speed: it has ~1 release per week (probably need to pin the version)
Mypy and pyright might disagree (I don't think this is a concern right now, since most type checking rules need to be disabled to pass pyright).

edit: pyright can apparently be easily integrated in pre-commit: https://github.com/microsoft/pyright/blob/main/docs/ci-integration.md

jreback

It would be great to enable pyright on the CI. It takes only ~90sec to run pyright on the entire codebase with the following configuration in pyproject.toml (ignores most type errors for now):

@twoertwein can you setup this?

I will look into that. Should that be part of this PR or a followup (it might take me some time to play around with the CI).

Where in the CI should pyright be integrated (I assume in "CI / Checks")? Should pyright also be part of the pre-commit? There doesn't seem to be a pre-commit interface from pyright, except this third-party wrapper: https://github.com/necaris/pre-commit-pyright.

Other points to consider:

Pyright depends on node.js

Pyright fixes bugs at a rapid speed: it has ~1 release per week (probably need to pin the version)

Mypy and pyright might disagree (I don't think this is a concern right now, since most type checking rules need to be disabled to pass pyright).

absolutely separate PR. in fact you might want to get the job in and can mark it failures_allowed for now, then PRs to fix up issues to get it to pass (and a todo list if needed).

Pyright depends on node.js
prob need to add setup instructions in the docs then on how to run it

Pyright fixes bugs at a rapid speed: it has ~1 release per week (probably need to pin the version)
sounds fine to pin

.circleci/config.yml

jreback · 2021-09-21T13:37:07Z

@twoertwein happy to merge this when ready

twoertwein · 2021-09-21T15:26:18Z

@twoertwein happy to merge this when ready

I partially typed _get_grouper in the latest commit (contains an Any) to keep the intended TypeVar as pointed out by @simonjayhawkins

Apart for double-checking that, it should be ready from my side.

twoertwein · 2021-09-25T15:23:54Z

Removed the pyright configuration (will be added in #43672).

twoertwein · 2021-09-25T16:29:33Z

Rebased (includes #43672) and enabled checks for reportInvalidTypeVarUse and reportSelfClsParameterName.

jreback · 2021-09-25T21:25:58Z

pandas/core/apply.py

@@ -1337,7 +1337,7 @@ def _make_unique_kwarg_list(


 def relabel_result(
-    result: FrameOrSeries,
+    result: DataFrame | Series,


why are we not able to use the alias, this is the entire point of it (i mean if we want to switch exclusively to DataFrame | Series i guess its ok, but we have an alias on purpose.

FrameOrSeries = TypeVar("FrameOrSeries", bound="NDFrame")
A TypeVar needs to be bound which isn't the case here.

Might be helpful to rename FrameOrSeries to NDFrameT

ahh ok, then lets do that

in this PR or a followup?

I don't mind adding more (I'm not the one reviewing it)

follow-up ok. we have the naming for historical reasons and IIRC other reviewers have strong views on the naming, so IMO best not to mix in anything that could be controversial.

jreback · 2021-09-25T21:26:13Z

pandas/core/arrays/interval.py

-    def shift(
-        self: IntervalArrayT, periods: int = 1, fill_value: object = None
-    ) -> IntervalArray:
+    def shift(self, periods: int = 1, fill_value: object = None) -> IntervalArray:


why are these not valid?

same here:
IntervalArrayT = TypeVar("IntervalArrayT", bound="IntervalArray")
if the return value was of type IntervalArrayT, it would be valid.

simonjayhawkins

Thanks @twoertwein very nice cleanup!

we may want to try to replace more NDFrame usages outside of core (core.generic/core.groupby) where possible now that the invalid use of the inappropriately named FrameOrSeries typvar is caught by pyright.

simonjayhawkins reviewed Jul 4, 2021

View reviewed changes

simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Jul 4, 2021

twoertwein marked this pull request as draft July 4, 2021 15:50

simonjayhawkins mentioned this pull request Jul 4, 2021

CLN: Replace FrameOrSeriesUnion annotation by DataFrame | Series #41955

Merged

4 tasks

twoertwein commented Jul 5, 2021

View reviewed changes

pandas/core/window/rolling.py Show resolved Hide resolved

simonjayhawkins mentioned this pull request Jul 5, 2021

CI: Prefer conda-forge over defaults #42377

Merged

4 tasks

simonjayhawkins reviewed Jul 5, 2021

View reviewed changes

twoertwein marked this pull request as ready for review July 5, 2021 18:38

simonjayhawkins approved these changes Jul 7, 2021

View reviewed changes

github-actions bot added the Stale label Aug 7, 2021

mroeschke removed the Stale label Aug 7, 2021

mroeschke added this to the 1.4 milestone Aug 7, 2021

jreback requested changes Sep 20, 2021

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

TYP: fix reportInvalidTypeVarUse from pyright

f0383cd

twoertwein added 5 commits September 25, 2021 12:17

two more reportInvalidTypeVarUse

8202866

one more reportInvalidTypeVarUse

3925744

restore .circleci/config.yml

eca8fd1

partially type _get_grouper

22ae867

enable reportInvalidTypeVarUse and reportSelfClsParameterName

4ab71a3

twoertwein mentioned this pull request Sep 25, 2021

TYP: Track progress of pyright rules #43742

Closed

32 tasks

jreback requested changes Sep 25, 2021

View reviewed changes

simonjayhawkins approved these changes Sep 26, 2021

View reviewed changes

simonjayhawkins merged commit 49d3713 into pandas-dev:master Sep 26, 2021

twoertwein mentioned this pull request Sep 26, 2021

TYP: rename FrameOrSeries to NDFrameT #43752

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TYP: fix reportInvalidTypeVarUse from pyright #42367

TYP: fix reportInvalidTypeVarUse from pyright #42367

twoertwein commented Jul 4, 2021 •

edited

Loading

simonjayhawkins commented Jul 4, 2021

twoertwein commented Jul 4, 2021

simonjayhawkins commented Jul 4, 2021

simonjayhawkins commented Jul 4, 2021

twoertwein commented Jul 4, 2021

simonjayhawkins left a comment

twoertwein commented Jul 6, 2021

simonjayhawkins left a comment

simonjayhawkins Jul 7, 2021

twoertwein Jul 7, 2021

twoertwein Sep 21, 2021

simonjayhawkins commented Jul 7, 2021

github-actions bot commented Aug 7, 2021

twoertwein commented Aug 7, 2021

alimcmaster1 commented Aug 31, 2021

twoertwein commented Sep 1, 2021

jreback commented Sep 20, 2021

twoertwein commented Sep 20, 2021 •

edited

Loading

jreback left a comment

jreback commented Sep 21, 2021

twoertwein commented Sep 21, 2021

twoertwein commented Sep 25, 2021

twoertwein commented Sep 25, 2021

jreback Sep 25, 2021

twoertwein Sep 25, 2021

twoertwein Sep 25, 2021

jreback Sep 25, 2021

twoertwein Sep 25, 2021

simonjayhawkins Sep 26, 2021

jreback Sep 25, 2021

twoertwein Sep 25, 2021

simonjayhawkins left a comment

TYP: fix reportInvalidTypeVarUse from pyright #42367

TYP: fix reportInvalidTypeVarUse from pyright #42367

Conversation

twoertwein commented Jul 4, 2021 • edited Loading

simonjayhawkins commented Jul 4, 2021

twoertwein commented Jul 4, 2021

simonjayhawkins commented Jul 4, 2021

simonjayhawkins commented Jul 4, 2021

twoertwein commented Jul 4, 2021

simonjayhawkins left a comment

Choose a reason for hiding this comment

twoertwein commented Jul 6, 2021

simonjayhawkins 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

simonjayhawkins commented Jul 7, 2021

github-actions bot commented Aug 7, 2021

twoertwein commented Aug 7, 2021

alimcmaster1 commented Aug 31, 2021

twoertwein commented Sep 1, 2021

jreback commented Sep 20, 2021

twoertwein commented Sep 20, 2021 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

jreback commented Sep 21, 2021

twoertwein commented Sep 21, 2021

twoertwein commented Sep 25, 2021

twoertwein commented Sep 25, 2021

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

simonjayhawkins left a comment

Choose a reason for hiding this comment

twoertwein commented Jul 4, 2021 •

edited

Loading

twoertwein commented Sep 20, 2021 •

edited

Loading