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

Move MyPy from isolated goal into 'lint' goal and add MyPy whitelist/opt-in type checking #8099

Merged
merged 26 commits into from Aug 7, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
8b75dec
[WIP] Move MyPy from isoltated goal into 'lint' goal
Jul 23, 2019
ca47558
Change logic to warn users instead of blocking users with errors
Jul 24, 2019
73c24fd
filter targets to exclude tags that may be described in pants.ini
Jul 26, 2019
6822729
update tests to mock out function and assert its called instead of as…
Jul 26, 2019
bca96d2
code review changes
Jul 26, 2019
60a8638
no warning if whitelist option not specified and style/code review ch…
Jul 29, 2019
d908bee
refactor test cases
Jul 29, 2019
c758e59
add required header
Jul 30, 2019
2186531
add year to copyright note/comment
Jul 30, 2019
80c666b
format import order
Jul 30, 2019
0cb69f4
skip my lint check in ci
Jul 30, 2019
b45a94d
move ci mypy check to be under lint
Jul 30, 2019
dff9175
specify mypy lint tag name and add default tag in pants.ini
Jul 30, 2019
27adf2b
python style issues
Jul 31, 2019
3cfb3fa
restore previous ci mypy checking, add new verbose option for mypy an…
Aug 1, 2019
f8e90c1
style issues
Aug 1, 2019
dd9909a
Merge branch 'mabdi/move-mypy-to-lint-goal' of https://github.com/mab…
wisechengyi Aug 2, 2019
90cd103
Merge branch 'master' into mabdi3-mabdi/move-mypy-to-lint-goal
wisechengyi Aug 2, 2019
a3c163a
fix lint ci shard
Aug 2, 2019
3f41b85
minor changes to ci , pants.ini and mypy task
Aug 5, 2019
f70ac76
convert pants ini option into command line option
Aug 5, 2019
1049a84
refactor integration test
Aug 5, 2019
7282342
update depcrecated_options_scope
Aug 6, 2019
27ad8a5
test
Aug 6, 2019
98dd205
update deprecated options scope
Aug 6, 2019
2d72f08
update timeout for failing test
Aug 6, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion build-support/bin/mypy.py
Expand Up @@ -6,7 +6,7 @@


def main() -> None:
subprocess.run(["./pants", "--tag=+type_checked", "mypy", "::"], check=True)
subprocess.run(["./pants", "--lint-skip", "--no-lint-mypy-skip", "lint", "::"], check=True)


if __name__ == '__main__':
Expand Down
2 changes: 1 addition & 1 deletion contrib/mypy/src/python/pants/contrib/mypy/register.py
Expand Up @@ -7,4 +7,4 @@


def register_goals():
task(name='mypy', action=MypyTask).install('mypy')
task(name='mypy', action=MypyTask).install('lint')
1 change: 1 addition & 0 deletions contrib/mypy/src/python/pants/contrib/mypy/tasks/BUILD
Expand Up @@ -15,5 +15,6 @@ python_library(
'src/python/pants/task',
'src/python/pants/util:contextutil',
'src/python/pants/util:memo',
'src/python/pants/build_graph'
],
)
57 changes: 49 additions & 8 deletions contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py
Expand Up @@ -5,6 +5,7 @@
import subprocess

from pants.backend.python.interpreter_cache import PythonInterpreterCache
from pants.build_graph.target import Target
from pants.backend.python.targets.python_binary import PythonBinary
from pants.backend.python.targets.python_library import PythonLibrary
from pants.backend.python.targets.python_target import PythonTarget
Expand All @@ -18,14 +19,33 @@
from pex.interpreter import PythonInterpreter
from pex.pex import PEX
from pex.pex_info import PexInfo
from pants.task.lint_task_mixin import LintTaskMixin
from typing import List
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved


class MypyTask(ResolveRequirementsTaskBase):
"""Invoke the mypy static type analyzer for Python."""
class MypyTaskError(TaskError):
"""Indicates a TaskError from a failing MyPy run."""

class MypyTask(LintTaskMixin, ResolveRequirementsTaskBase):
"""Invoke the mypy static type analyzer for Python.

Mypy lint task filters out target_roots that are not properly tagged according to
--whitelisted-tag-name (defaults to None, and no filtering occurs if this option is 'None'),
and executes MyPy on targets in context from whitelisted target roots.
Next, if any transitive targets from the filtered roots are not whitelisted, a warning
will be printed.

'In context' meaning in the sub-graph where a whitelisted target is the root
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this message might be better moved inline. Out of context, I was confused.

One possibility: end the sentence at line 34 and then line 35 has parentheses with this message.

"""

stuhood marked this conversation as resolved.
Show resolved Hide resolved
_MYPY_COMPATIBLE_INTERPETER_CONSTRAINT = '>=3.5'
_PYTHON_SOURCE_EXTENSION = '.py'

deprecated_options_scope = 'mypy'
deprecated_options_scope_removal_version = '1.20.0.dev2'
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved

WARNING_MESSAGE = "[WARNING]: All targets in context should be whitelisted for mypy to run"

@classmethod
def prepare(cls, options, round_manager):
super().prepare(options, round_manager)
Expand All @@ -36,6 +56,8 @@ def register_options(cls, register):
register('--mypy-version', default='0.710', help='The version of mypy to use.')
register('--config-file', default=None,
help='Path mypy configuration file, relative to buildroot.')
register('--whitelist-tag-name', default=None,
help='Tag name to identify python targets to execute MyPy')

@classmethod
def supports_passthru_args(cls):
Expand All @@ -60,9 +82,30 @@ def is_non_synthetic_python_target(target):
def is_python_target(target):
return isinstance(target, PythonTarget)

def _calculate_python_sources(self, targets):
"""Generate a set of source files from the given targets."""
python_eval_targets = filter(self.is_non_synthetic_python_target, targets)
def _is_tagged_target(self, target: Target) -> bool:
return self.get_options().whitelist_tag_name in target.tags

def _is_tagged_non_synthetic_python_target(self, target: Target) -> bool:
return (self.is_non_synthetic_python_target(target) and
self._is_tagged_target(target))

def _all_targets_are_whitelisted(self, targets: List[Target], all_targets: List[Target]) -> bool:
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved
return len(targets) == 0 or len(targets) == len(all_targets)

def _whitelist_warning(self) -> None:
self.context.log.warn(self.WARNING_MESSAGE)

def _calculate_python_sources(self, target_roots: List[Target]):
"""Filter targets to generate a set of source files from the given targets."""
if self.get_options().whitelist_tag_name:
white_listed_targets = self._filter_targets(Target.closure_for_targets(list(filter(self._is_tagged_target, target_roots))))
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved
python_eval_targets = [tgt for tgt in list(white_listed_targets) if self._is_tagged_non_synthetic_python_target(tgt)]
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved
if not self._all_targets_in_context_are_whitelisted(python_eval_targets, white_listed_targets):
self._whitelist_warning()
else:
python_eval_targets = self._filter_targets(list(filter(self.is_non_synthetic_python_target, Target.closure_for_targets(target_roots))))
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved
self._whitelist_warning()
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved

sources = set()
for target in python_eval_targets:
sources.update(
Expand Down Expand Up @@ -121,7 +164,6 @@ def execute(self):
with open(sources_list_path, 'w') as f:
for source in sources:
f.write('{}\n'.format(source))

# Construct the mypy command line.
cmd = ['--python-version={}'.format(interpreter_for_targets.identity.python)]
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved
if self.get_options().config_file:
Expand All @@ -135,7 +177,6 @@ def execute(self):
source_roots = self._collect_source_roots()

mypy_path = os.pathsep.join([os.path.join(get_buildroot(), root) for root in source_roots])

# Execute mypy.
with self.context.new_workunit(
name='check',
Expand All @@ -146,4 +187,4 @@ def execute(self):
returncode = self._run_mypy(mypy_interpreter, cmd,
env={'MYPYPATH': mypy_path}, stdout=workunit.output('stdout'), stderr=subprocess.STDOUT)
if returncode != 0:
raise TaskError('mypy failed: code={}'.format(returncode))
raise MypyTaskError('mypy failed: code={}'.format(returncode))
Eric-Arellano marked this conversation as resolved.
Show resolved Hide resolved
11 changes: 10 additions & 1 deletion contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/BUILD
Expand Up @@ -9,4 +9,13 @@ python_tests(
'tests/python/pants_test:interpreter_selection_utils',
],
tags={'integration'},
)
)

python_tests(
name='mypy',
source = 'test_mypy.py',
dependencies=[
'tests/python/pants_test:task_test_base',
'contrib/mypy/src/python/pants/contrib/mypy/tasks',
],
)
@@ -0,0 +1,54 @@
from pants.contrib.mypy.tasks.mypy_task import MypyTask, MypyTaskError
from pants_test.task_test_base import TaskTestBase
from pants.backend.python.targets.python_library import PythonLibrary
from unittest.mock import MagicMock


class MyPyTests(TaskTestBase):

@classmethod
def task_type(cls):
return MypyTask

def test_throws_no_warning_on_all_whitelisted_target_roots(self) -> None:
t1 = self.make_target('t1', PythonLibrary, tags=['type_checked'])
t2 = self.make_target('t2', PythonLibrary, tags=['type_checked'])
task = self.create_task(self.context(target_roots=[t1, t2]))
self.set_options(whitelist_tag_name='type_checked')
task.execute()

def test_throws_no_warning_on_some_whitelisted_target_roots_but_all_whitelisted_in_context(self) -> None:
t1 = self.make_target('t1', PythonLibrary)
t2 = self.make_target('t2', PythonLibrary, tags=['type_checked'])
t3 = self.make_target('t3', PythonLibrary, tags=['type_checked'], dependencies=[t2])
task = self.create_task(self.context(target_roots=[t1, t3]))
self.set_options(whitelist_tag_name='type_checked')
task.execute()

def test_throws_warning_on_some_whitelisted_target_roots_but_all_whitelisted_in_context(self) -> None:
t1 = self.make_target('t1', PythonLibrary)
t2 = self.make_target('t2', PythonLibrary, tags=['something_else'])
t3 = self.make_target('t3', PythonLibrary, tags=['type_checked'], dependencies=[t2])
task = self.create_task(self.context(target_roots=[t1, t3]))
self.set_options(whitelist_tag_name='type_checked')
task._whitelist_warning = MagicMock()
task.execute()
task._whitelist_warning.assert_called()

def test_throws_warning_on_all_whitelisted_target_roots_but_some_whitelisted_transitive_targets(self) -> None:
t1 = self.make_target('t1', PythonLibrary, tags=['type_checked'])
t2 = self.make_target('t2', PythonLibrary, tags=['something_else'])
t3 = self.make_target('t3', PythonLibrary, tags=['type_checked'], dependencies=[t2])
t4 = self.make_target('t4', PythonLibrary, tags=['type_checked'], dependencies=[t3])
task = self.create_task(self.context(target_roots=[t1, t4]))
self.set_options(whitelist_tag_name='type_checked')
task._whitelist_warning = MagicMock()
task.execute()
task._whitelist_warning.assert_called()

def test_throws_warning_if_no_whitelist_specified(self) -> None:
t1 = self.make_target('t1', PythonLibrary)
task = self.create_task(self.context(target_roots=[t1]))
task._whitelist_warning = MagicMock()
task.execute()
task._whitelist_warning.assert_called()
Expand Up @@ -8,7 +8,9 @@
class MypyIntegrationTest(PantsRunIntegrationTest):
def test_mypy(self):
cmd = [
'mypy',
'--lint-skip',
'--no-lint-mypy-skip',
'lint',
'contrib/mypy/tests/python/pants_test/contrib/mypy::',
'--',
'--follow-imports=silent'
Expand Down
2 changes: 1 addition & 1 deletion pants.ini
Expand Up @@ -302,7 +302,7 @@ skip: True
[pycheck-newstyle-classes]
skip: True

[mypy]
[lint.mypy]
config_file: build-support/mypy/mypy.ini
Copy link
Contributor

Choose a reason for hiding this comment

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

See above about reverting this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree, a lot of these changes were to subvert a failing lint shard in ci, but this way I don't think mypy works as intended anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mypy isn't used much in pants, so I think we're sticking with this for now. We should eventually find a way to avoid the c++ resolving issue(s).

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think that we do actually need to configure this right now.

If we were going to temporarily stop configuring it, the right way to do that would be with a TODO here that disables it and points to a ticket for re-enabling it. But as Eric said, I don't think we should do that without more discussion.


[scala]
Expand Down