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

Support mypy plugins and 3rdpary type definitions. #8328

Merged
merged 1 commit into from
Sep 26, 2019

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Sep 24, 2019

Add support to the Pants mypy contrib plugin for loading type
definitions and mypy plugins from requirements in the transitive closure
of targets being checked.

Fixes #8263.

@jsirois
Copy link
Contributor Author

jsirois commented Sep 24, 2019

NB: Depends on #8327 which contains 6069e15. Reviewers need only focus on 574e8b7 and any subsequent changes.

@jsirois
Copy link
Contributor Author

jsirois commented Sep 24, 2019

@gtrak if you have time to review, thanks in advance.

@gtrak
Copy link

gtrak commented Sep 24, 2019

The code looks reasonable given 0 experience on this codebase. Is there a way I can point the pants script to this particular commit and run mypy against my product code repo?

@jsirois
Copy link
Contributor Author

jsirois commented Sep 24, 2019

Is there a way I can point the pants script to this particular commit and run it against my product code repo?

@gtrak Yes. See 'Running from sources' here: https://www.pantsbuild.org/howto_develop.html

To encapsulate that a bit, this script in your repo with a PANTS_SOURCE value that makes sense and the right list of plugins filled in should do:

#!/usr/bin/env bash

set -euo pipefail

cd "$(git rev-parse --show-toplevel)"

# Run pants from sources.  Useful for debugging.
# Assumes you have the pantsbuild/pants repo checked out in a sibling dir of this dir, named
# 'pants' but overridable with PANTS_SOURCE.

PANTS_SOURCE="${PANTS_SOURCE:-../pants}"

# When running pants from sources you are likely to be modifying those sources, so
# you won't want pantsd running.  You can override this by setting ENABLE_PANTSD=true.
ENABLE_PANTSD="${ENABLE_PANTSD:-false}"

backend_packages=(
  "pants.contrib.mypy"
)

pythonpath=(
  "${PANTS_SOURCE}/contrib/mypy/src/python"
)

# A list of strings matching the [GLOBAL] plugins in your pants.ini
# save for the one represented by live sources above.
plugins=(
)

function string_list() {
  eval local -r list_variable="\${$1[@]}"

  echo -n "["
  for item in ${list_variable}; do
    echo -n "\"${item}\","
  done
  echo -n "]"
}

export PANTS_VERSION="$(cat "${PANTS_SOURCE}/src/python/pants/VERSION")"
export PANTS_PLUGINS="$(string_list plugins)"
export PANTS_PYTHONPATH="+$(string_list pythonpath)"
export PANTS_BACKEND_PACKAGES="+$(string_list backend_packages)"
export PANTS_ENABLE_PANTSD="${ENABLE_PANTSD}"
export no_proxy=*

exec "${PANTS_SOURCE}/pants" "--no-verify-config" "$@"

@jsirois jsirois requested a review from benjyw September 24, 2019 19:44
@gtrak
Copy link

gtrak commented Sep 25, 2019

Tried a couple things and got stuck:

pantsbuild.pants.contrib.mypy in the plugins section:

code-repo$ ./pants_sourced mypy player:lib
Scrubbed PYTHONPATH=/home/gary/dev/pants/src/python: from the environment.
01:25:03 [INFO] Resolving new plugins...:
  pantsbuild.pants.contrib.mypy
/home/gary/dev/pants/build-support/virtualenvs/Linux/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pex/pep425tags.py:274: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
  import imp
timestamp: 2019-09-24T20:25:31.183400
Exception caught: (pkg_resources.ContextualVersionConflict) (backtrace omitted)
Exception message: (pex 1.6.11 (/home/gary/dev/pants/build-support/virtualenvs/Linux/pants_dev_deps.py37.venv/lib/python3.7/site-packages), Requirement.parse('pex==1.6.8'), {'pantsbuild.pants'})

Again with just 'pants.contrib.mypy' in the script plugins section minus the pantsbuild. prefix

code-repo$ ./pants_sourced mypy
Scrubbed PYTHONPATH=/home/gary/dev/pants/src/python: from the environment.
01:25:40 [INFO] Resolving new plugins...
  pants.contrib.mypy
/home/gary/dev/pants/build-support/virtualenvs/Linux/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pex/pep425tags.py:274: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
  import imp
timestamp: 2019-09-24T20:25:41.607528
Exception caught: (pex.resolver.Unsatisfiable) (backtrace omitted)
Exception message: Could not satisfy all requirements for pants.contrib.mypy:
    pants.contrib.mypy

pants.contrib.mypy in the 'backend' section:
Seems like it 'works', but it's not actually running mypy.

$ time ./pants_sourced mypy web:lib
Scrubbed PYTHONPATH=/home/gary/dev/pants/src/python: from the environment.
01:33:58 [WARN] Globs did not match. Excludes were: ["web/**/*.swp"]. Unmatched globs were: ["web/static/*", "web/templates/*.html"].
01:33:58 [WARN] /home/gary/dev/pants/build-support/virtualenvs/Linux/pants_dev_deps.py37.venv/lib/python3.7/site-packages/twitter/common/collections/__init__.py:41: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working
  from collections import Iterable


20:33:57 00:00 [main]
               (To run a reporting server: ./pants server)
20:33:58 00:01   [setup]
20:33:58 00:01     [parse]
               Executing tasks in goals: mypy
20:33:58 00:01   [mypy]
20:33:58 00:01   [complete]
               SUCCESS

real	0m1.835s
user	0m1.230s
sys	0m0.530s

Whereas the main pants binary takes about 5 seconds with different output:

$ ./pants mypy web:lib
01:35:30 [WARN] Globs did not match. Excludes were: ["web/**/*.swp"]. Unmatched globs were: ["web/static/*", "web/templates/*.html"].
01:35:30 [WARN] /home/gary/.cache/pants/setup/bootstrap-Linux-x86_64/1.18.0_py37/lib/python3.7/site-packages/twitter/common/collections/__init__.py:41: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working
  from collections import Iterable


20:35:29 00:00 [main]
               (To run a reporting server: ./pants server)
20:35:30 00:01   [setup]
20:35:30 00:01     [parse]
               Executing tasks in goals: gen -> pyprep -> mypy
20:35:30 00:01   [gen]
20:35:30 00:01     [thrift-py]
20:35:30 00:01     [py-thrift-namespace-clash-check]
20:35:30 00:01   [pyprep]
20:35:30 00:01     [interpreter]
20:35:30 00:01     [build-local-dists]
20:35:30 00:01     [requirements]
20:35:30 00:01     [sources]
20:35:30 00:01   [mypy]
20:35:30 00:01     [mypy]
20:35:30 00:01       [check]

Notably, it has a [mypy] [mypy] [check] goal tree, but I can't seem to make the other script do that.

@jsirois
Copy link
Contributor Author

jsirois commented Sep 25, 2019

@gtrak I think I just caused un-needed confusion. Leave plugins=() empty in the script and try again.

@gtrak
Copy link

gtrak commented Sep 25, 2019

That matches my scenario 3 I've tried already. It still doesn't actually seem to be running mypy. I went back to pants master and had the same result, so it seems unrelated to the code change in the PR.

Running it with:

backend_packages=(
    "pants.contrib.mypy"
)
plugins=()

@jsirois
Copy link
Contributor Author

jsirois commented Sep 25, 2019

@gtrak I did not read closely enough. The mypy task is installed in the umbrella lint goal (along with linters for other languages):

def register_goals():
task(name='mypy', action=MypyTask).install('lint')

So run ./pants_sourced lint ... not ./pants_sourced mypy ....

@gtrak
Copy link

gtrak commented Sep 25, 2019

It works!

Here's proof, when I have an incorrect config, it is loading the plugin:

09:30:42 00:00     [sources]
09:30:42 00:00   [lint]
09:30:42 00:00     [mypy]
09:30:43 00:01       [create_mypy_pex]
09:30:43 00:01       [check]
Error constructing plugin instance of NewSemanalDjangoPlugin

mypy.ini:0: error: 'django_settings_module' is not set: no section [mypy.plugins.django-stubs]

FAILURE: mypy failed: code=2


               Waiting for background workers to finish.
09:30:43 00:01   [complete]
               FAILURE

real	0m3.078s
user	0m2.254s

When I add back to mypy.ini:

[mypy.plugins.django-stubs]
django_settings_module = web.settings
09:32:14 00:01     [requirements]
09:32:14 00:01     [sources]
09:32:14 00:01   [lint]
09:32:14 00:01     [mypy]
09:32:14 00:01       [create_mypy_pex]
09:32:14 00:01       [check]

               Waiting for background workers to finish.
09:32:16 00:03   [complete]
               SUCCESS

real	0m3.889s

When I remove the django plugin from mypy.ini, I get all the old type errors as expected.

Is there anything else that would be helpful to test here?

Curiously, I tried to remove the django-stubs dep from my project, but mypy still works. Is that intentional?

@gtrak
Copy link

gtrak commented Sep 25, 2019

Looking at the code again, it seems we are hardcoding django-stubs as a dep for the mypy task. I'm not sure if that can have any ill effects down the line. It seems to work for me.

@jsirois
Copy link
Contributor Author

jsirois commented Sep 25, 2019

Curiously, I tried to remove the django-stubs dep from my project, but mypy still works. Is that intentional?

That should not be the case.

Looking at the code again, it seems we are hardcoding django-stubs as a dep for the mypy task. I'm not sure if that can have any ill effects down the line. It seems to work for me.

This is not true. Can you provide a pointer to this? Keep in mind the contrib/mypy/examples tree is used only in tests, the plugin code is all contained in contrib/mypy/src/python.

Add support to the Pants mypy contrib plugin for loading type
definitions and mypy plugins from requirements in the transitive closure
of targets being checked.

Fixes pantsbuild#8263.
@jsirois
Copy link
Contributor Author

jsirois commented Sep 25, 2019

Re-based to resolve conflict. No code changes.

@gtrak
Copy link

gtrak commented Sep 25, 2019

Hmm, you're right, I was looking at /examples. Still, I'm not passing the flag, so it makes sense that removing django-stubs from requirements would have no effect.

    register('--include-requirements', type=bool, default=False,
             help='Whether to include the transitive requirements of targets being checked. This is'
                  'useful if those targets depend on mypy plugins or distributions that provide '
                  'type stubs that should be active in the check.') 

I'll dig in later and try to see where it's grabbing the plugin from.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Yay, thank you!

@jsirois jsirois merged commit b7e0aa1 into pantsbuild:master Sep 26, 2019
@jsirois jsirois deleted the issues/8263/fix branch September 26, 2019 05:18
@gtrak
Copy link

gtrak commented Sep 27, 2019

Just to close the loop on this, I'm not really sure about my issue in #8328 (comment) . I thought it was caching related, initially. However, I'm finding that this command reliably works: ./pants_sourced lint --lint-mypy-include-requirements web:lib, and the less clear behavior was around not adding --lint-mypy-include-requirements. /shrug

Thanks for fixing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Django mypy plugin cannot be used with pantsbuild mypy plugin.
3 participants