mypy contrib plugin #5172

tdyas · 2017-12-06T00:39:25Z

This plugin runs the mypy static type analyzer.

tdyas · 2017-12-06T00:40:36Z

This PR will likely fail because mypy requires Python 3.x, even though it can analyze Python 2.7 sources. Namely, the attempt to install Python 3.x if it is in the PATH will fail. The task runs nicely if Python 3.x is available.

Where should we discuss optionally making Python 3.x available from within the Pants venv? I can open a separate issue for that.

kwlzn

awesome! this lgtm w/ a few comments.

a basic integration test would also be good.

kwlzn · 2017-12-06T01:47:18Z

contrib/mypy/src/python/pants/contrib/mypy/BUILD

+    'src/python/pants/goal:task_registrar',
+  ],
+  distribution_name='pantsbuild.pants.contrib.mypy',
+  description='RPM package builder for pants',


wrong description

kwlzn · 2017-12-06T01:55:02Z

build-support/pants_venv

@@ -21,6 +21,9 @@ function activate_venv() {

 function create_venv() {
  rm -rf "$(venv_dir)"
+  if which python3; then
+    python3 -m venv "$(venv_dir)"
+  fi


intended? probably best to rely on interpreter/compatibility constraints in pants vs constructing the venv with python3 here.

I thought I had to get python3 into the venv. Plugin works with the python3 I have on my PATH.

kwlzn · 2017-12-06T01:56:04Z

contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py

+
+
+class MypyTask(PythonTask):
+  """Invoke the mypy static type analzyer for Python."""


kwlzn · 2017-12-06T01:56:31Z

contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py

+    return True
+
+  def __init__(self, *args, **kwargs):
+    super(MypyTask, self).__init__(*args, **kwargs)


probably better to just remove this __init__ method.

kwlzn · 2017-12-06T02:04:11Z

contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py

+    pex_info = PexInfo.default()
+    pex_info.entry_point = 'mypy'
+    chroot = self.cached_chroot(interpreter=py3_interpreter, pex_info=pex_info, targets=[],
+      extra_requirements=[PythonRequirement('mypy==0.550')])


this probably deserves either an option to override the version (e.g. --mypy-version w default='0.550') or a tool address (e.g. --mypy-tool-address w default='3rdparty/python:mypy').

kwlzn · 2017-12-06T02:16:19Z

@tdyas would file an issue for the python3 venv discussion and we can all chime in there.

tdyas · 2017-12-06T21:16:38Z

I'll need to make some adjustments to the plugin. I have to expose the Python environment expected by the source code to mypy via the MYPYPATH environment variable.

benjyw · 2017-12-06T23:49:23Z

contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py

+      if directory:
+        roots.add(directory)
+
+    import pdb ; pdb.set_trace()


Debugging leftover. Pushed too early. Made better progress locally. Will push when the integration test is working.

benjyw · 2017-12-06T23:51:21Z

contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py

+      )
+    return list(sources)
+
+  def _calculate_python_package_roots(self, targets):


Doesn't {target.target_base for target in targets} give you what you want?

Yeah I switched to that after getting that from John on Slack general.

tdyas · 2017-12-07T00:33:29Z

Ok I pushed my current version. I added an integration test but it can't really do anything because the CI environment may not provide Python 3.x? How should I go about getting Py3 into the CI environment to ensure a proper integration test?

jsirois · 2017-12-07T00:42:10Z

... because the CI environment may not provide Python 3.x?

CI does provide 3.x (https://docs.travis-ci.com/user/reference/trusty/#Python-images), but a guard is good sanity since Pants developers may not have this.

Inspiration for the guard here: https://github.com/jsirois/pants/blob/master/tests/python/pants_test/python/test_interpreter_selection_integration.py#L24

benjyw

LGTM! Just one typo.

benjyw · 2017-12-07T00:50:28Z

contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py

+      self.context.log.debug('No Python sources to check.')
+      return
+
+    # Determine interprter used by the sources so we can tell mypy.


s/interprter/interpreter/

tdyas · 2017-12-07T01:05:53Z

I modified the integration test to call self.has_python_version('3'). That succeeds but the integration test still fails with the mypy task failing to find a py3-compatible interpreter. (I have not pushed this change to the PR yet.)

--- a/contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/test_mypy_integration.py
+++ b/contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/test_mypy_integration.py
@@ -11,6 +11,12 @@ from pants_test.pants_run_integration_test import PantsRunIntegrationTest

 class MypyIntegrationTest(PantsRunIntegrationTest):
   def test_mypy(self):
-    with self.pants_results(['mypy', 'contrib/mypy/tests/python/pants_test/contrib/mypy::']) as pants_run:
-      self.assert_failure(pants_run)
-      self.assertTrue('Unable to find a Python 3.x interpreter (required for mypy)' in pants_run.stdout_data)
+    if self.has_python_version('3'):
+      # Python 3.x is available. Test that we see an error in this integration test.
+      with self.pants_results(['mypy', 'contrib/mypy/tests/python/pants_test/contrib/mypy::']) as pants_run:
+        self.assert_success(pants_run)
+    else:
+      # Python 3.x was not found. Test whether mypy task fails for that reason.
+      with self.pants_results(['mypy', 'contrib/mypy/tests/python/pants_test/contrib/mypy::']) as pants_run:
+        self.assert_failure(pants_run)
+        self.assertTrue('Unable to find a Python 3.x interpreter (required for mypy)' in pants_run.stdout_data)

jsirois · 2017-12-07T02:13:40Z

contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py

+    return True
+
+  def find_py3_interpreter(self):
+    interpreters = self._interpreter_cache.setup(filters=['Python>=3'])


s/Python/CPython/ or else s/Python//

kwlzn · 2017-12-07T08:13:23Z

@tdyas fwiw, @CMLivingston recently added this utility to pex for materializing version-specific interpreters on demand in tests: pex-tool/pex@df326ff#diff-b619e563eafa44775c3411bd4c79e252R283

and its likely that we'll want to land something similar in pants soon as part of other python3 work - but it might be worth pow-wowing with him on Slack to see if it would make sense to break the landing of that utility into a separate earlier PR.

kwlzn

lgtm!

kwlzn · 2017-12-07T08:20:00Z

contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py

+        # Construct the MYPYPATH from both the source roots and targets' sys.path.
+        mypy_path = ':'.join([os.path.join(get_buildroot(), root) for root in source_roots])
+        mypy_path += ':'
+        mypy_path += pythonpath_for_targets


maybe:

mypy_path = ':'.join((mypy_path, pythonpath_for_targets)

I also could combine the two lines into mypy_path += ':' + pythonpath_for_targets. Using .join here seems too fancy for plain old string concatenation, unless the style here is to avoid mutability?

No strong opinion on join, but it's easy enough to use os.pathsep instead of a raw :.

Good idea. Will do that. (os.pathsep)

This plugin runs the `mypy` static type analyzer.

tdyas · 2017-12-21T07:14:44Z

Anything else remaining for me to do with this PR?

benjyw · 2018-02-05T03:45:02Z

contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py

+      return
+
+    # Determine interpreter used by the sources so we can tell mypy.
+    interpreter_for_targets = self._interpreter_cache.select_interpreter_for_targets(self.context.target_roots)


@tdyas : I'm deprecating PythonTask, as it's part of the old pipeline (should have noticed you were relying on it in this change, my bad). So quick question - why are you selecting the interpreter only for self.context.target_roots rather than all targets (later you collect source roots for all targets, which I assume means that mypy operates on all of them)? Does this matter, or can I have this select an interpreter for all targets (and then use the already-computed product for that)?

I chose to use target_roots since I only wanted mypy to operate on the targets passed on the pants command-line. I'm not wedded to that idea though. What is expected of a Pants code linting task?

As for switching to another Python pipeline, just point me at what I should be using.

I'm still figuring out how this would fit into the new pipeline, since it has some unusual usage patterns (e.g., having to select two interpreters, one for running mypy and one for discovering the python version of the input files).

benjyw · 2018-02-05T04:07:52Z

And another question: don't the external requirements need to be on the MYPYPATH for mypy to work properly? E.g., if the code being checked relies on some code from 3rdparty, doesn't mypy need to know that?

And while I have your attention, why do the input source roots need to be on MYPYPATH? The documentation states that "The directories containing the sources given on the command line" are searched for imports anyway, so it seems superfluous to also add them to MYPYPATH?

benjyw · 2018-02-05T04:13:34Z

Hmm, https://mypy.readthedocs.io/en/latest/basics.html says

You may be tempted to point MYPYPATH to the standard library or to the site-packages directory where your 3rd party packages are installed. This is almost always a bad idea – you will likely get tons of error messages about code you didn’t write and that mypy can’t analyze all that well yet, and in the worst case scenario mypy may crash due to some construct in a 3rd party package that it didn’t expect.

So what are you supposed to do about 3rdparty code? Either create typeshed stubs, or give up on that information?

tdyas · 2018-02-05T04:22:38Z

I'm not actually sure what the "right" answer should be for third party code. In an earlier version of the task, I had the task compute the locations within the pex for third party modules and then added those paths to the MYPYPATH. This hit exactly the issue you're quoting where mypy tries to analyze third party code that it really shouldn't be analyzing. One answer is to write typeshed stubs for those projects or find some other way for mypy to find those stubs. Another is to set --follow-imports=skip or --follow-imports=silent to basically have mypy ignore imports for which it can't find source or type stubs. (It basically then treats those modules as type Any which is compatible with everything.)

I'm still trying to understand the recommended way to use mypy. Don't assume I've figured that out yet!

tdyas · 2018-02-05T04:27:35Z

And another question: don't the external requirements need to be on the MYPYPATH for mypy to work
properly? E.g., if the code being checked relies on some code from 3rdparty, doesn't mypy need to
know that?

This was true in a prior version of the task. See comment above for more info. I basically found it too verbose for issues that the user couldn't fix, so removed the code that put the third party code on the MYPYPATH.

tdyas · 2018-02-05T04:30:30Z

And while I have your attention, why do the input source roots need to be on MYPYPATH? The documentation states that "The directories containing the sources given on the command line" are searched for imports anyway, so it seems superfluous to also add them to MYPYPATH?

So that mypy can find imports correctly of modules that were not specified on the command-line, but that are imported from code that was part of target_roots. Obviously the need for this changes if all Python code (and not just target_roots) is checked by mypy.

tdyas · 2018-02-05T04:45:36Z

FYI I just found PEP 561 which proposes how to distribute type information with modules.

stuhood requested review from benjyw and kwlzn December 6, 2017 01:25

kwlzn reviewed Dec 6, 2017

View reviewed changes

tdyas force-pushed the mypy_contrib_plugin branch from a4f2cd4 to 233cb63 Compare December 6, 2017 19:42

benjyw requested changes Dec 6, 2017

View reviewed changes

tdyas force-pushed the mypy_contrib_plugin branch from 233cb63 to 290c181 Compare December 7, 2017 00:32

benjyw approved these changes Dec 7, 2017

View reviewed changes

tdyas force-pushed the mypy_contrib_plugin branch from 290c181 to f58e75a Compare December 7, 2017 01:08

jsirois reviewed Dec 7, 2017

View reviewed changes

kwlzn approved these changes Dec 7, 2017

View reviewed changes

tdyas force-pushed the mypy_contrib_plugin branch 3 times, most recently from f5cee4b to 6f36f34 Compare December 12, 2017 21:13

mypy contrib plugin

620b35e

This plugin runs the `mypy` static type analyzer.

tdyas force-pushed the mypy_contrib_plugin branch from 6f36f34 to 620b35e Compare December 19, 2017 21:03

stuhood merged commit dec8a9a into pantsbuild:master Dec 25, 2017

benjyw reviewed Feb 5, 2018

View reviewed changes

tdyas deleted the mypy_contrib_plugin branch February 20, 2023 20:35



		class MypyTask(PythonTask):
		"""Invoke the mypy static type analzyer for Python."""

mypy contrib plugin #5172

mypy contrib plugin #5172

Conversation

tdyas commented Dec 6, 2017

tdyas commented Dec 6, 2017 • edited Loading

kwlzn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwlzn commented Dec 6, 2017

tdyas commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdyas commented Dec 7, 2017

jsirois commented Dec 7, 2017

benjyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdyas commented Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwlzn commented Dec 7, 2017

kwlzn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdyas Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

tdyas commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw commented Feb 5, 2018

benjyw commented Feb 5, 2018

tdyas commented Feb 5, 2018

tdyas commented Feb 5, 2018 • edited Loading

tdyas commented Feb 5, 2018

tdyas commented Feb 5, 2018

tdyas commented Dec 6, 2017 •

edited

Loading

tdyas commented Dec 7, 2017 •

edited

Loading

tdyas Dec 11, 2017 •

edited

Loading

tdyas commented Feb 5, 2018 •

edited

Loading