Skip to content

Conversation

twoertwein
Copy link
Member

@twoertwein twoertwein commented Oct 1, 2021

Some of the many type errors unveiled by cache_readonly might be easily fixable. This PR simply ignores all the uncovered errors to quickly enable mypy's disallow_untyped_decorators to prevent future untyped decorators.

I enabled mypy's disallow_untyped_decorators but pyright's reportUntypedFunctionDecorator finds the following three functions that are not found by mypy, so I didn't enable reportUntypedFunctionDecorator (can't add ignore statements, otherwise mypy will complain about unused ignore statements, edit: using pyright's file-based ignore comments):

  • pandas/conftest.py:1505
  • pandas/core/util/numba_.py:93
  • pandas/core/window/numba_.py:244

@jreback jreback added the Typing type annotations, mypy/pyright type checking label Oct 2, 2021
@twoertwein
Copy link
Member Author

The failure from the web-docs:

pandas/pandas/core/indexes/datetimes.py:docstring of pandas.core.indexes.datetimes.DatetimeIndex:124: WARNING: autosummary: stub file not found 'pandas.DatetimeIndex.std'. Check your autosummary_generate setting.

seems unrelated, but it looks legit. DatetimeIndex inherits the std name through a decorator, but the class from which the method is copied has no doc-string for it (pandas/core/arrays/datetimes.py:1917).

@jbrockmendel
Copy link
Member

seems unrelated, but it looks legit. DatetimeIndex inherits the std name through a decorator, but the class from which the method is copied has no doc-string for it (pandas/core/arrays/datetimes.py:1917).

yah there are several methods near the top of DatetimeIndex that are there instead of using the decorator for this reason, most of the "# methods that dispatch to DatetimeArray and wrap result" section

@twoertwein
Copy link
Member Author

yah there are several methods near the top of DatetimeIndex that are there instead of using the decorator for this reason, most of the "# methods that dispatch to DatetimeArray and wrap result" section

So the solution is to write a doc-string?

I'm surprised that this warning/error occurs only for this PR. The 1.3.3 documentation of DatetimeIndex has the same issue:
image

@jbrockmendel
Copy link
Member

So the solution is to write a doc-string?

IIRC the motivation was for mypy, not docstrings. the decorator should handle docstrings correctly.

@twoertwein
Copy link
Member Author

twoertwein commented Oct 13, 2021

The decorator seems to handle doc-strings correctly, all other attributes (for example tz) have their doc-string string in the web-documentation. The main difference between tz and std is that DatetimeArray.tz has a doc-string while DatetimeArray.std has none: DatetimeIndex.tz has therefore a doc-string but DatetimeIndex.std doesn't have a doc-string.

I will write a doc-string in a separate PR.

But I don't understand why this PR causes this issue.

@twoertwein
Copy link
Member Author

Should be ready for review :)

@jreback jreback added this to the 1.4 milestone Oct 18, 2021
@jreback
Copy link
Contributor

jreback commented Oct 21, 2021

cc @simonjayhawkins

return result.view(self.dtype)
# error: Incompatible return value type (got "Union[ExtensionArray,
# ndarray[Any, Any]]", expected "PeriodArray")
return result.view(self.dtype) # type: ignore[return-value]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in pandas/core/arrays/datetimelike.py we overloaded view for DatetimeLikeArrayMixin. maybe could do the same for PeriodArray. (suggestion, not necessarily for this PR)

# expected "Index"
levels = [ping.group_index for ping in self.grouper.groupings] + [
lev # type: ignore[list-item]
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, we have a Union return type from factorize.

Over time we should avoid Union return types, either through refactoring code or using overloads.

from https://github.com/python/typeshed/blob/master/CONTRIBUTING.md#conventions

avoid union return types: python/mypy#1693;

(comment, no action needed for this PR)

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @twoertwein very nice. some comments and suggestions but none blockers.

@simonjayhawkins
Copy link
Member

I enabled mypy's disallow_untyped_decorators but pyright's reportUntypedFunctionDecorator finds the following three functions that are not found by mypy, so I didn't enable reportUntypedFunctionDecorator (can't add ignore statements, otherwise mypy will complain about unused ignore statements):

  • pandas/conftest.py:1505
  • pandas/core/util/numba_.py:93
  • pandas/core/window/numba_.py:244

maybe open an issue after this is merged.

@simonjayhawkins
Copy link
Member

(or use module level pyright comment syntax)

@simonjayhawkins
Copy link
Member

Some of the many type errors unveiled by cache_readonly might be easily fixable. This PR simply ignores all the uncovered errors to quickly enable mypy's disallow_untyped_decorators to prevent future untyped decorators.

SGTM

@twoertwein
Copy link
Member Author

Rebased and addressed the big comments.

If it is green, it should be ready for merging from my perspective.

@simonjayhawkins
Copy link
Member

If it is green, it should be ready for merging from my perspective.

sure. @jreback @jbrockmendel any further comments?

@twoertwein
Copy link
Member Author

There is something failing related due to the numba wrapper, will debug that

@twoertwein
Copy link
Member Author

I think numba analyzes the source code to JIT it. I think the errors are a results of jitting a function that itself has a jitted function where the inner function is jitted with typed_numba_jit.

@twoertwein
Copy link
Member Author

I think I have to revert the numba-wrapper: https://numba.pydata.org/numba-doc/dev/reference/pysupported.html#functions-as-arguments. It would be great to clean up all the ignore statement for numba.jit calls but I'm te wrong person for that - not familiar with numba and how pandas uses it.

@simonjayhawkins let me know if you see a simple non-intrusive way that could avoid the ignore statements for this PR. Otherwise, I will revert the commit that added typed_numba_jit.

@simonjayhawkins
Copy link
Member

@simonjayhawkins let me know if you see a simple non-intrusive way that could avoid the ignore statements for this PR. Otherwise, I will revert the commit that added typed_numba_jit.

no problem leaving that out of this PR. The method I'm working with would not prevent the ignores but does providing typing for the "pandas lib api". I use that term loosely as meaning anything coded in Cython, as this is otherwise not formalized. https://github.com/simonjayhawkins/pandas/blob/ec8f0397b5c4a232e8e3e7abfbec91e9ea33016a/pandas/_libs_numba/algos.py#L245-L255

It'll probably be better to write out own stubs for now and include them in the main repo

@twoertwein
Copy link
Member Author

I removed the numba wrapper and rebased

@twoertwein
Copy link
Member Author

The isinstance change and rebased after #44144

@simonjayhawkins
Copy link
Member

@twoertwein can you resolve conflicts

@simonjayhawkins
Copy link
Member

I removed the numba wrapper and rebased

i've opened #44233 that we could maybe merge after this.

@twoertwein
Copy link
Member Author

i've opened #44233 that we could maybe merge after this.

Great! I'll rebase this PR to resolve the conflicts with master.

@pep8speaks
Copy link

pep8speaks commented Oct 30, 2021

Hello @twoertwein! 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 2021-10-30 11:55:31 UTC

@jreback jreback merged commit e394c53 into pandas-dev:master Oct 30, 2021
@jreback
Copy link
Contributor

jreback commented Oct 30, 2021

nice thanks @twoertwein and @simonjayhawkins for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
5 participants