Skip to content

MyPy.ini Make Import Machinery Explicit #26645

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

Closed
wants to merge 5 commits into from

Conversation

WillAyd
Copy link
Member

@WillAyd WillAyd commented Jun 4, 2019

Now that we've wound down a lot of the mistyped annotations in the code base the next logical review point is our import machinery. Previously we used ignore_missing_imports for everything in the code base, but this is suggested by Mypy only as a last resort:

https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports

This PR makes the missing imports explicit. Third party libraries I think we'll have to keep until they become available in typeshed. Internal imports may be resolved through either stubfiles or internal import machinery (would need review as follow ups)

@WillAyd WillAyd added the Typing type annotations, mypy/pyright type checking label Jun 4, 2019
@WillAyd
Copy link
Member Author

WillAyd commented Jun 4, 2019

cc @gwrome @vaibhavhrt

@@ -57,7 +57,7 @@ def in_interactive_session():

def check_main():
try:
import __main__ as main
import __main__ as main # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

This was ignored and won't be fixed as stated in mypy python/mypy#658, though perhaps a better solution would be to just use sys.stdin.isatty() (follow up PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it would be nice to put a link the related mypy issue as comment too, for people in future who might be wonder why it's been ignored.
But l feel isattay() is indeed the better solution.

@WillAyd WillAyd added this to the 0.25.0 milestone Jun 4, 2019
@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #26645 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26645      +/-   ##
==========================================
- Coverage   91.87%   91.87%   -0.01%     
==========================================
  Files         174      174              
  Lines       50692    50692              
==========================================
- Hits        46575    46572       -3     
- Misses       4117     4120       +3
Flag Coverage Δ
#multiple 90.41% <100%> (ø) ⬆️
#single 41.8% <100%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/io/formats/console.py 78.12% <100%> (ø) ⬆️
pandas/io/gbq.py 78.94% <0%> (-10.53%) ⬇️
pandas/core/frame.py 97% <0%> (-0.12%) ⬇️
pandas/util/testing.py 90.91% <0%> (+0.1%) ⬆️

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 c07d71d...78e0cbf. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #26645 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26645      +/-   ##
==========================================
- Coverage   91.87%   91.87%   -0.01%     
==========================================
  Files         174      174              
  Lines       50692    50692              
==========================================
- Hits        46575    46572       -3     
- Misses       4117     4120       +3
Flag Coverage Δ
#multiple 90.41% <100%> (ø) ⬆️
#single 41.8% <100%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/io/formats/console.py 78.12% <100%> (ø) ⬆️
pandas/io/gbq.py 78.94% <0%> (-10.53%) ⬇️
pandas/core/frame.py 97% <0%> (-0.12%) ⬇️
pandas/util/testing.py 90.91% <0%> (+0.1%) ⬆️

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 c07d71d...78e0cbf. Read the comment docs.

@jbrockmendel
Copy link
Member

Side-note: will mypy look for config in setup.cfg? A lot of tools are moving to support that, and its nice to keep it all in one place.

@WillAyd
Copy link
Member Author

WillAyd commented Jun 4, 2019

Yea we could migrate this to that file as well:

https://mypy.readthedocs.io/en/latest/config_file.html

@vaibhavhrt
Copy link
Contributor

Yeah keeping all configs at same place i.e. setup.cfg is a good idea.

@vaibhavhrt
Copy link
Contributor

Whew a lot of external dependencies needs to be ignored and typeshed repo doesn't looks too active. Maybe I will add one of the our deps to typeshed. Let me know if you have one in mind(a small and simple one preferably).

[mypy-pandas.io.msgpack._unpacker]
ignore_missing_imports = True

[mypy-pandas.io.sas._sas]
Copy link
Contributor

@jreback jreback Jun 5, 2019

Choose a reason for hiding this comment

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

why do the cython libs have to be here?

Copy link
Member Author

Choose a reason for hiding this comment

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

These throw errors like error: No library stub file for module ‘<the_pyx_file>’ - I imagine the annotations don't find anything that gets imported.

I think the only way around this may be to add stub files for the internally used aspects of the .pyx files but haven't found anything definitive yet. Also asked if anyone had experience with this on the typing Gitter but no luck yet. Stub files might not be a terrible fallback if that's all that works

@jreback
Copy link
Contributor

jreback commented Jun 6, 2019

why is this a good idea at this time? shouldn't we type more things first?

@WillAyd
Copy link
Member Author

WillAyd commented Jun 6, 2019 via email

@jreback
Copy link
Contributor

jreback commented Jun 8, 2019

Is this actually the recommended solution by mypy? it seems very strange since no libraries (that we need) actually have stubs.

@jreback jreback removed this from the 0.25.0 milestone Jun 8, 2019
@WillAyd
Copy link
Member Author

WillAyd commented Jun 9, 2019

Calling it the recommended solution might be a little strong but our current method is the discouraged approach. Citing link from the OP:

If the module is a third party library, but you cannot find any existing type hints nor have time to write your own, you can silence the errors:

  1. To silence a single missing import error, add a # type: ignore at the end of the line containing the import.
  2. To silence all missing import imports errors from a single library, add a section to your mypy config file for that library setting ignore_missing_imports to True. For example, suppose your codebase makes heavy use of an (untyped) library named foobar. You can silence all import errors associated with that library and that library alone by adding the following section to your config file:
    ...
  3. To silence all missing import errors for all libraries in your codebase, invoke mypy with the --ignore-missing-imports command line flag or set the ignore_missing_imports config file option to True in the global section of your mypy config file:
    ...
    We recommend using this approach only as a last resort: it’s equivalent to adding a # type: ignore to all unresolved imports in your codebase.

So this PR would essentially move us from option 3 to option 2 above

@jreback
Copy link
Contributor

jreback commented Jun 9, 2019

I am probably ok with option 2 for external libraries

but what about our internal ones? (the _libs) these we should ‘fix’

@WillAyd
Copy link
Member Author

WillAyd commented Jun 9, 2019

Yea agreed on internal. Not saying we just keep these as is but we are ignoring them with the current mypy file. This PR makes it explicit and gives a checklist of items we can prioritize with the community to tackle

@WillAyd
Copy link
Member Author

WillAyd commented Jul 2, 2019

Closing for now to keep queue clean. Can always revisit

@WillAyd WillAyd closed this Jul 2, 2019
@WillAyd WillAyd mentioned this pull request Aug 25, 2019
@WillAyd WillAyd deleted the mypy-ini-cleanup branch January 16, 2020 00:34
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
Development

Successfully merging this pull request may close these issues.

4 participants