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 untyped calls in mypy check #4053

Merged
merged 12 commits into from
Jul 19, 2022

Conversation

zundertj
Copy link
Collaborator

Fixes mypy "disallow_untyped_calls" in #4044 .

@github-actions github-actions bot added the python Related to Python Polars label Jul 17, 2022
@zundertj
Copy link
Collaborator Author

Similar issue as in #4052 with numpy. The Python 3.7 CI run installs numpy 1.21.6, as numpy 1.22 and beyond drop Python 3.7 support (https://numpy.org/devdocs/release/1.22.0-notes.html). This means that there is no way to satisfy type annotations on older numpy, which is not fully type annotated and therefore triggers mypy's no-untyped-call, and at the same time also not use typing.cast or type ignore to satisfy the Python 3.10 run. So we have to make a choice here, do we want to please mypy on all possible Python versions we support, or are we happy to target the latest version(s) only? Not arguing for dropping Python 3.7 altogether, it seems it is from a testing perspective and the ability to use future imports, to not be material.

@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2022

Codecov Report

Merging #4053 (c6f0c8e) into master (bc1a498) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4053   +/-   ##
=======================================
  Coverage   62.89%   62.89%           
=======================================
  Files         451      451           
  Lines       74585    74585           
=======================================
+ Hits        46908    46911    +3     
+ Misses      27677    27674    -3     
Impacted Files Coverage Δ
py-polars/polars/internals/series.py 94.71% <100.00%> (ø)
polars/polars-io/src/csv/read_impl.rs 79.17% <0.00%> (-2.89%) ⬇️
...s/polars-core/src/chunked_array/upstream_traits.rs 64.92% <0.00%> (-0.87%) ⬇️
polars/polars-core/src/frame/asof_join/groups.rs 77.47% <0.00%> (-0.18%) ⬇️
polars/polars-core/src/series/mod.rs 53.48% <0.00%> (+0.15%) ⬆️
polars/polars-core/src/chunked_array/mod.rs 55.18% <0.00%> (+0.19%) ⬆️
...lars/polars-core/src/series/arithmetic/borrowed.rs 35.77% <0.00%> (+0.20%) ⬆️
polars/polars-arrow/src/kernels/take.rs 70.37% <0.00%> (+0.24%) ⬆️
...lars/polars-core/src/chunked_array/ops/take/mod.rs 51.49% <0.00%> (+0.33%) ⬆️
polars/polars-core/src/utils/mod.rs 52.77% <0.00%> (+0.35%) ⬆️
... and 5 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 bc1a498...c6f0c8e. Read the comment docs.

@zundertj zundertj marked this pull request as draft July 17, 2022 14:10
@stinodego
Copy link
Member

Hm, tough one. I think we want to keep mypy on Python 3.7 in our CI; I'll do some checks.

Maybe we can merge these changes, but keep the flag disabled until we come up with a solution?

@stinodego
Copy link
Member

stinodego commented Jul 18, 2022

Nice solution to override the mypy flag in the CI 👍 if the CI is happy, we can merge this.

(CI checks are broken now though, it seems 😄 )

@zundertj
Copy link
Collaborator Author

zundertj commented Jul 18, 2022

It is not ideal though. The real issue is the numpy version, rather than the Python version, so this does not work for anyone that develops with numpy < 1.22, even if at Python 3.10. I don't think this will bite much in practice, but my apologies in advance for anyone's time lost due to this little hack.

@zundertj zundertj marked this pull request as ready for review July 18, 2022 21:00
@stinodego
Copy link
Member

The Python test CI checks are still not running, it seems.

@zundertj
Copy link
Collaborator Author

Tried to fix again, but it seems I am hitting this: https:munity/t/matrix-cannot-be-used-in-jobs-level-if/17177/12. I.e., this is not possible, as the if-statement is evaluated before the matrix. The only solution I see now is to not use the matrix anymore, and thus duplicate the job effectively for Python 3.7.

Open for anyone (with more GH Actions experience) to suggest a better solution.

@ritchie46
Copy link
Member

Thanks @zundertj and @stinodego. Merging it in.

@ritchie46 ritchie46 merged commit bd6ede0 into pola-rs:master Jul 19, 2022
@ritchie46
Copy link
Member

I had to revert this one because the github action yaml was invalid. We need to run the action if the action file has changed, I shall open an issue for this.

@zundertj can you reopen the PR?

@zundertj
Copy link
Collaborator Author

Cannot re-open, see #4099.

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.

4 participants