Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow generics in mypy typing check #4052

Closed
wants to merge 1 commit into from

Conversation

zundertj
Copy link
Collaborator

Aim: fulfill task disallow_any_generics in #4044.

This PR is far from complete. There are over 100 warnings, and I ran into several issues. We should decide on those first before going through all issues.

  1. LazyFrame has due to Preserve DataFrame type after LazyFrame roundtrips #2862 and related PR's become a GenericType, to allow for nice typing synergy where one can subclass DataFrame and get the LazyFrame typed version as well. This is implemented (sorry for non-precise language here) by carrying around the type of DataFrame that LazyFrame refers to, or in other words, LazyFrame wraps around the DataFrame. This does mean that the specific DataFrame is a type input into LazyFrame, and mypy does not accept the default value: Unable to specify a default value for a generic parameter python/mypy#3737 So the solution for now is for each type annotation in the polars code base that involves LazyFrame to write LazyFrame[Any] I would think that we could also use LazyFrame[DF] instead, but ran into issues with that. Open to suggestions

  2. numpy has been strongly improving their typing, and where we currently use np.ndarray as the type annotation, it seems we should move towards numpy.typing.NDArray (https://numpy.org/doc/stable/reference/typing.html#numpy.typing.NDArray) However, this was only introduced in numpy 1.21, released in June 2021. We have always been very reluctant to impose version constraints on dependencies (as our usage of numpy is mostly high level). If we go down this path, we will have to. Otherwise, what I have done so far in some of the PR, is to simply add ignores.

Most of the other issues are us not typing built-in generics such as list, tuple, dict, Callable, Sequence etc, those should be easy to fix.

@github-actions github-actions bot added the python Related to Python Polars label Jul 17, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #4052 (b631e86) into master (6695cce) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4052      +/-   ##
==========================================
+ Coverage   62.84%   62.86%   +0.01%     
==========================================
  Files         449      449              
  Lines       74509    74510       +1     
==========================================
+ Hits        46828    46841      +13     
+ Misses      27681    27669      -12     
Impacted Files Coverage Δ
py-polars/polars/internals/construction.py 97.66% <ø> (ø)
py-polars/polars/internals/lazy_functions.py 94.51% <ø> (ø)
py-polars/polars/datatypes_constructor.py 96.77% <100.00%> (ø)
py-polars/polars/internals/anonymous_scan.py 58.97% <100.00%> (+1.07%) ⬆️
py-polars/polars/internals/expr.py 95.38% <100.00%> (ø)
py-polars/polars/internals/functions.py 97.38% <100.00%> (ø)
py-polars/polars/utils.py 85.31% <100.00%> (ø)
...s/polars-core/src/chunked_array/ops/take/traits.rs 51.61% <0.00%> (-5.38%) ⬇️
...olars-lazy/src/physical_plan/executors/scan/csv.rs 96.07% <0.00%> (-1.97%) ⬇️
polars/polars-arrow/src/utils.rs 74.15% <0.00%> (-1.13%) ⬇️
... and 9 more

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 6695cce...b631e86. Read the comment docs.

@zundertj zundertj marked this pull request as draft July 17, 2022 14:26
@zundertj zundertj changed the title WIP: Disallow generics in mypy typing check Disallow generics in mypy typing check Jul 17, 2022
@stinodego
Copy link
Member

With #4320 merged, we can now close this PR. Thanks @zundertj and @matteosantama ! Another victory for our code quality 😄 🚀

@stinodego stinodego closed this Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants