-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
MAINT, CI: mypy: fix typing errors + add mypy to CI #13613
Conversation
This fixes all the typing errors present on master.
This adds a very basic CI run for type checking on ubuntu-latest in the github actions pipelines.
f2819fe
to
219ea19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI addition looks reasonable at first glance.
The failure may need another exemption for mypy
?
scipy/_lib/_uarray/_backend.py:96: error: syntax error in type comment [syntax]
Found 1 error in 1 file (checked 662 source files)
I don't see any errors with Python 3.8.0 and I think this may be related to python/mypy#8614 but need some confirmation to be completely sure.
Edit: Oh, sorry! Syntax errors can't be ignored in mypy. Any other thoughts? |
This just adds `# type: ignore` to the line in `scipy/_lib/_uarray/_backend.py` to ignore the syntax error detected by `mypy`. This error seems to be present only on Python 3.9.0a5 with mypy 0.770.
358eee7
to
657a20d
Compare
`mypy` 0.770 was broken on Python 3.9.0a5. Hence, the requirement file is altered to always download the latest `mypy` version. See https://mail.python.org/pipermail/scipy-dev/2021-February/024580.html Also, latest mypy version reported some more typing errors which have been resolved in this commit.
Thanks, @rgommers for the quick reply on the mailing list. I have made the changes to use the latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tirthasheshpatel, this looks very good. All type annotation changes look correct to me. Regarding CI, there is one issue: if you do it as a completely separate job, then that job takes about 16 minutes to build SciPy, just in order to run mypy. Instead, it would be better to just add this as a separate step in the test_nightly
job, the same command would be really fast because invoking runtests.py
twice in a row reuses the same build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now. CI is green: Success: no issues found in 671 source files
.
Merging, thanks @tirthasheshpatel.
Thanks for the reviews and help, @rgommers! |
Sorry, I was asleep - the CI changes do not take effect within a PR (for safety reasons, that's how GH Actions is designed). There are a lot of failures on the next PR that got this new CI:
So I will revert this.
If it passes on your own fork, it should pass here too. |
See comment on scipygh-13613 for details.
Oh, sorry that that happened.
Ok, I will try that and let you know if it passes. |
Reference issue
Closes #13606
What does this implement/fix?
I have tried to fix all the errors described in gh-13606. Also, I have also attempted to add a separate CI job for
mypy
on ubuntu-latest in the GitHub Actions pipelines. I am not very familiar with CI so criticism/opinions on those changes are welcome!Additional information
I am not very familiar with the macOS GitHub Actions job and so haven't added
mypy
test for it currently. I can try adding something for macOS too with some further assistance!Also, should we wait for gh-13448? It fixes some errors present in
scipy.spatial.distance
.