Adding mypy annotations throughout the codebase

[pradyunsg]

We adopted the use of mypy in #4545. This issue is for discussing and tracking how we actually want to utilize mypy in pip's codebase.

[agronholm]

Is this really worth the effort, considering pip has no public API and the fact that it increases the maintenance load?

[agronholm]

If the annotations could be added directly to the code, that wouldn't be a problem, but as long as .pyi files must be used...

[pradyunsg]

Is this really worth the effort, considering pip has no public API and the fact that it increases the maintenance load?

Yes. It's a way to catch bugs and serves as useful insight into what a function expects; which helps with the what-is-going-where problem in the codebase.

but as long as .pyi files must be used...

From what I gather, # type: ... comments are enough. I don't see any fancy uses in pip's codebase that would mandate a .pyi. :)

[agronholm]

Alright, that would work then I guess.

[pradyunsg]

As an aside, @agronholm you might want to take a look at how the configuration module is annotated. :)

[pradyunsg]

I've gone ahead and generated stubs for pip via MonkeyType and put them at https://github.com/pradyunsg/pip/tree/experiment/monkeytype-annotations/stubs/pip for now.

I believe they won't be completely correct since they were generated automatically and only using on the code paths covered by the tests. They should, however, serve as a good reference for someone working to add # type: ... comments to the codebase. :)

[pradyunsg]

This issue is a good starting point for anyone who wants to help out with pip's development -- it's simple and the process of fixing this should be a good introduction to pip's development workflow. See the discussion above to understand what the desired fix is.

[pradyunsg]

Dropping a reminder comment that someone needs to update OP based on the merged PRs from the Bloomberg sprint.

I'll try to come around to it sooner than later.

[pradyunsg]

I've gone ahead and generated stubs for pip via MonkeyType and put them at pradyunsg/pip:monkeytype-annotations/stubs/pip@experiment

#4748 (comment)

@chrahunt ^ :)

[mkurnikov]

@pradyunsg I'll help with strict_optional as soon as #6704 is merged.

[chrahunt]

it's faster just to finish it manually instead of trying to make generator work + merging it with the current annotations

There are still some 300+ functions missing annotations that I saw setting disallow_untyped_defs = False globally. I am not quick enough to do that by hand without spending a full day on it. :)

@pradyunsg, I saw that, the feature of pyannotate I thought may be useful is its ability to merge the output from a test run with the code itself as type annotation comments.

Locally I have been able to generate the output JSON files for test runs based on the main pytest process and subprocess executions. We'll see if the output can be merged easily or if it would be as much work as hand-merging from separate annotation files.

[pradyunsg]

Yep yep. I've become familiar with both these tools to an unreasonable extent over the past few weeks. 🌈

My take that no one asked for: It's definitely worth exploring automation here, though since this is a one-off thing (it's not like we're going to add annotations post-facto again), I'm not sure where the cut off lies to just doing them manually vs using automation so I'll defer to whoever is doing the work to choose. 🙃

[cjerdonek]

I just merged PR #6704 FYI (progress on strict_optional=True).

Also, to help with the strict optional stuff, I think it would be helpful to add to utils/typing.py a typed cast_away_optional() function like that described here: python/typing#645 (comment)

I also think it would be good to put a typed version of cast() there too.

[pradyunsg]

@cjerdonek Turns out mypy supports inline configuration on a per-file basis and we should probably adopt that.

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

[pradyunsg]

File that fail with disallow_untyped_defs = False:

pip/_internal/__init__
pip/_internal/build_env
pip/_internal/cli/autocompletion
pip/_internal/cli/base_command
pip/_internal/cli/cmdoptions
pip/_internal/cli/parser
pip/_internal/commands/__init__
pip/_internal/commands/check
pip/_internal/commands/completion
pip/_internal/commands/configuration
pip/_internal/commands/debug
pip/_internal/commands/download
pip/_internal/commands/freeze
pip/_internal/commands/hash
pip/_internal/commands/help
pip/_internal/commands/install
pip/_internal/commands/list
pip/_internal/commands/search
pip/_internal/commands/show
pip/_internal/commands/uninstall
pip/_internal/commands/wheel
pip/_internal/distributions/base
pip/_internal/distributions/installed
pip/_internal/distributions/source
pip/_internal/distributions/wheel
pip/_internal/download
pip/_internal/exceptions
pip/_internal/index
pip/_internal/legacy_resolve
pip/_internal/locations
pip/_internal/models/candidate
pip/_internal/models/format_control
pip/_internal/models/link
pip/_internal/models/search_scope
pip/_internal/operations/check
pip/_internal/operations/freeze
pip/_internal/utils/appdirs
pip/_internal/utils/compat
pip/_internal/utils/compat
pip/_internal/utils/deprecation
pip/_internal/utils/hashes
pip/_internal/utils/logging
pip/_internal/utils/misc
pip/_internal/utils/models
pip/_internal/utils/temp_dir
pip/_internal/utils/ui
pip/_internal/vcs/bazaar
pip/_internal/vcs/git
pip/_internal/vcs/mercurial
pip/_internal/vcs/subversion
pip/_internal/vcs/versioncontrol
pip/_internal/wheel

Files that fail with strict_optional = True:

pip/_internal/build_env
pip/_internal/cache
pip/_internal/cli/base_command
pip/_internal/cli/cmdoptions
pip/_internal/configuration
pip/_internal/index
pip/_internal/legacy_resolve
pip/_internal/locations
pip/_internal/models/format_control
pip/_internal/operations/check
pip/_internal/operations/freeze
pip/_internal/operations/prepare
pip/_internal/pep425tags
pip/_internal/req
pip/_internal/req/constructors
pip/_internal/req/req_file
pip/_internal/req/req_install
pip/_internal/req/req_set
pip/_internal/req/req_tracker
pip/_internal/utils/encoding
pip/_internal/utils/glibc
pip/_internal/utils/misc
pip/_internal/utils/ui
pip/_internal/wheel

[pradyunsg]

Turns out mypy supports inline configuration on a per-file basis and we should probably adopt that.

#6750 does this now. :)

[pradyunsg]

Right now, what we'd want to tackle is the removal of # mypy: from the various files the workflow for which is:

• Run tox -e lint to make sure the mypy checks pass.
• Remove the comments in one of the files.
• Run tox -e lint to see what are the failures.
• Modify the annotations / code to resolve the failure.
• Run tox -e lint to ensure that the changes fix the issue.

Ideally, we'd do one-file-per-PR for this, since that's much easier to review that big PRs.

[pradyunsg]

Annd, this is done now. Thanks everyone who has helped drive this to completion! ^>^