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

Conversation

@mabdi3
Copy link
Contributor

commented Jul 23, 2019

Problem

We want to move MyPy into the lint goal and allow for users to opt-in into type_checking
for the new whitelisting/opt-in logic, we can make use of the lint goal capabilities instead of rebuilding the wheel
See #7886 and #6742 for more context.

Solution

Registers MyPy as a 'lint' goal, and implements opt-in 'whitelist' system for type checking.
Targets to be type checked are tagged according to the --whitelisted-tag-name option, defaults to type_checked as used from #7886. Add new mypy_tests for new logic, and update old tests and mypy instances to reflect lint goal change.

New mypy logic added to accomodate new opt-in strategy. Mypy will acts according to the following:

  1. Filter out non-whitelisted target root
  2. Check if all targets now in context are whitelisted, if not we throw an error
  3. Continue as normal, collecting python source files and running mypy
Mohamed Abdi
[WIP] Move MyPy from isoltated goal into 'lint' goal
Registers MyPy as a 'lint' goal, and implements opt-in 'whitelist' system for type checking.
Targets to be type checked are tagged according to the '--whitelisted-tag-name' option, defaults to `type_checked`
as used from #7886. Add new mypy_tests for new logic, and update old tests and mypy instances to reflect lint goal change.

New mypy logic added to accomodate new opt-in strategy. Mypy will acts according to the following:
1. Filter out non-whitelisted target root
2. Check if all targets now in context are whitelisted, if not we throw an error
3. Continue as normal, collecting python source files and running mypy

See #7886 and #6742 for more context

@mabdi3 mabdi3 changed the title [WIP] Move MyPy from isoltated goal into 'lint' goal [WIP] Move MyPy from isoltated goal into 'lint' goal and add MyPy whitelist/opt-in type checking Jul 23, 2019

@mabdi3 mabdi3 changed the title [WIP] Move MyPy from isoltated goal into 'lint' goal and add MyPy whitelist/opt-in type checking [WIP] Move MyPy from isolated goal into 'lint' goal and add MyPy whitelist/opt-in type checking Jul 23, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Thanks Mohammed!

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

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 24, 2019

Contributor

Nit: period after this sentence.

@@ -36,6 +49,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('--whitelisted-tag-name', default='type_checked',

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 24, 2019

Contributor

We'll want this to be an optional feature. You should be allowed to ignore this whitelist feature and just run ./pants lint.mypy ::. For example, if the entire codebase has 100% type coverage, a whitelist wouldn't make sense for that user.

To do this, set the default to None and change the below logic to only filter if this whitelist option is provided.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 24, 2019

Contributor

Also, maybe call this --whitelist-tag-name.

@@ -60,9 +75,22 @@ def is_non_synthetic_python_target(target):
def is_python_target(target):
return isinstance(target, PythonTarget)

def _is_tagged_target(self, target):

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 24, 2019

Contributor

Type hints would be awesome for this new code.

Suggested change
def _is_tagged_target(self, target):
def _is_tagged_target(self, target: Target) -> bool:
whitelisted_targets = Target.closure_for_targets(list(filter(self._is_tagged_target, targets)))
python_eval_targets = list(filter(self._is_tagged_non_synthetic_python_target, list(whitelisted_targets)))
if not self._all_targets_in_context_are_whitelisted(python_eval_targets, whitelisted_targets):
raise MypyTaskError("All targets in context need to be whitelisted for mypy to run")

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 24, 2019

Contributor

As decided in the meeting, this should not be the case. At most, log a warning. We don't want this because it makes it far harder for upstream users of some python_library to be able to use type checks, until their downstream dependency has fully type checked. In general, I think we will want to stick with MyPy's default of untyped code using type Any and not logging anything extra about it. We could choose to add those warnings if there's a strong push for it, but shouldn't do it otherwise.

python_eval_targets = filter(self.is_non_synthetic_python_target, targets)
"""Filter targets to generate a set of source files from the given targets."""
whitelisted_targets = Target.closure_for_targets(list(filter(self._is_tagged_target, targets)))
python_eval_targets = list(filter(self._is_tagged_non_synthetic_python_target, list(whitelisted_targets)))

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 24, 2019

Contributor
Suggested change
python_eval_targets = list(filter(self._is_tagged_non_synthetic_python_target, list(whitelisted_targets)))
python_eval_targets =[tgt for tgt in white_listed_targets if self._is_tagged_non_synthetic_python_target(tgt)]

python_tests(
name='mypy_tests',

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 24, 2019

Contributor

Convention in Pants is to call this mypy.

def task_type(cls):
return MypyTask

def test_raises_no_error_on_all_whitelisted_target_roots(self):

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 24, 2019

Contributor
Suggested change
def test_raises_no_error_on_all_whitelisted_target_roots(self):
def test_raises_no_error_on_all_whitelisted_target_roots(self) -> None:
@@ -6,7 +6,7 @@


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

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 24, 2019

Contributor

I think you can instead set this to lint.mypy and ignore the --lint-skip etc. Also, with your new whitelist feature, the --tag=+type_checked should be removed.

This comment has been minimized.

Copy link
@mabdi3

mabdi3 Jul 24, 2019

Author Contributor

Turns out lint.mypy isn't enough to just run mypy–one must specify to skip all lint tasks except mypy. Feel free to correct me if that's not the case, though a quick way to see this is to run ./pants lint.mypy versus ./pants --lint-skip --no-lint-mypy-skip lint.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 26, 2019

Member

Yep, @mabdi3 is right: all tasks in a goal run if any do.

Mohamed Abdi
Change logic to warn users instead of blocking users with errors
Change tests to reflect new logic
General code review fixes
deprecated_options_scope = 'mypy'
deprecated_options_scope_removal_version = '1.20.0.dev2'

WARNING_MESSAGE = None

This comment has been minimized.

Copy link
@mabdi3

mabdi3 Jul 24, 2019

Author Contributor

added this field for testing purposes, didn't seem worth spending a lot of time trying to figure out a way to keep track of logging/warning messages, so I used this workaround for now.

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Jul 26, 2019

Contributor

You can hardcode the value here.

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

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

@mabdi3 : Are you looking for review on this one? The title still says WIP, so confirming.

@mabdi3

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

@mabdi3 : Are you looking for review on this one? The title still says WIP, so confirming.

Yeah, just took the [WIP] off. Thanks for the heads up!

@mabdi3 mabdi3 changed the title [WIP] Move MyPy from isolated goal into 'lint' goal and add MyPy whitelist/opt-in type checking Move MyPy from isolated goal into 'lint' goal and add MyPy whitelist/opt-in type checking Jul 26, 2019

@wisechengyi
Copy link
Contributor

left a comment

Thanks @mabdi3! Looks mostly good. Just some minor issues.

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.

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Jul 26, 2019

Contributor

nit Warning. -> warning

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 26, 2019

Member

And warnings aren't "thrown": ... "rendered"/"printed" maybe?

deprecated_options_scope = 'mypy'
deprecated_options_scope_removal_version = '1.20.0.dev2'

WARNING_MESSAGE = None

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Jul 26, 2019

Contributor

You can hardcode the value here.

WARNING_MESSAGE = "[WARNING]: All targets in context should be whitelisted for mypy to run"
task = self.create_task(self.context(target_roots=[t1, t3]))
self.set_options(whitelist_tag_name='type_checked')
task.execute()
self.assertTrue(task.WARNING_MESSAGE)

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Jul 26, 2019

Contributor
  1. asserting True on str is relatively anti-pattern.
  2. I think a good alternative here regarding your comment

added this field (WARNING_MESSAGE ) for testing purposes, didn't seem worth spending a lot of time trying to figure out a way to keep track of logging/warning messages, so I used this workaround for now.

is to mock out _whitelist_warning, then assert_called(). E.g.

def test_second_noop_does_not_invoke_coursier(self):
junit_jar_lib = self._make_junit_target()
with self._temp_workdir():
# Initial resolve does a resolve and populates elements.
initial_context = self.context(target_roots=[junit_jar_lib])
task = self.execute(initial_context)
# If self.runjava has been called, that means coursier is called
task.runjava = MagicMock()
task.execute()
task.runjava.assert_not_called()

@stuhood
Copy link
Member

left a comment

Thanks! Looks great.

Show resolved Hide resolved contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/BUILD Outdated
Show resolved Hide resolved contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py Outdated
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.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 26, 2019

Member

And warnings aren't "thrown": ... "rendered"/"printed" maybe?

Mohamed Abdi
@wisechengyi

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

will merge once the linty issue is fixed.

[31mERROR: All .py files other than __init__.py should start with the header:
Show resolved Hide resolved contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py Outdated
Show resolved Hide resolved contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py Outdated
Show resolved Hide resolved contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py Outdated
Show resolved Hide resolved contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py Outdated
Show resolved Hide resolved contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py Outdated
Show resolved Hide resolved contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py Outdated
@@ -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))

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 29, 2019

Contributor

f-string

Show resolved Hide resolved contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py Outdated

@stuhood stuhood added this to the 1.19.x milestone Jul 29, 2019

Mohamed Abdi added some commits Jul 29, 2019

Mohamed Abdi
Mohamed Abdi
@Eric-Arellano

This comment has been minimized.

Should be 2019 instead of YYYY

Mohamed Abdi added some commits Jul 30, 2019

Mohamed Abdi
Mohamed Abdi
Mohamed Abdi
Mohamed Abdi

@mabdi3 mabdi3 force-pushed the mabdi3:mabdi/move-mypy-to-lint-goal branch from f1e6697 to dff9175 Jul 30, 2019

Mohamed Abdi added some commits Jul 31, 2019

Mohamed Abdi
Mohamed Abdi
Mohamed Abdi
Mohamed Abdi
@wisechengyi
Copy link
Contributor

left a comment

Thanks! just a nit thing

@@ -62,7 +62,7 @@ if git rev-parse --verify "${MERGE_BASE}" &>/dev/null; then
echo "* Checking lint"
./pants --exclude-target-regexp='testprojects/.*' --changed-parent="${MERGE_BASE}" lint || exit 1

echo "* Checking types for targets marked \`type_checked\`"
echo "* Checking types for targets marked \`type_checked\`"

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Aug 2, 2019

Contributor

nit: let's leave this this file untouched.

@Eric-Arellano
Copy link
Contributor

left a comment

Please do not merge until reverting the changes to default --config-file. Not formally requesting changes because I won't be able to re-review promptly with teaching.

Almost there 🎉!

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

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 3, 2019

Contributor

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.


_MYPY_COMPATIBLE_INTERPETER_CONSTRAINT = '>=3.5'
_PYTHON_SOURCE_EXTENSION = '.py'

deprecated_options_scope = 'mypy'
deprecated_options_scope_removal_version = '1.20.0.dev2'

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 3, 2019

Contributor

This needs to be later now that 1.20.0.dev2 was cut, I believe. Think this should be 1.21.0.dev0? cc @wisechengyi.

@@ -34,8 +55,12 @@ def prepare(cls, options, round_manager):
@classmethod
def register_options(cls, register):
register('--mypy-version', default='0.710', help='The version of mypy to use.')
register('--config-file', default=None,
register('--config-file', default='build-support/mypy/mypy.ini',

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 3, 2019

Contributor

Bad change. This should default to None still. We don't expect every Pants user to have a file at build-support/mypy/mypy.ini. This is how Pants itself happens to do it, but should only be configured in our local pants.ini - not in the default settings that get used by everyone.

help='Path mypy configuration file, relative to buildroot.')
register('--whitelist-tag-name', default='type_checked',

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 3, 2019

Contributor

Did you mean for the default to be None? I think you had this at one point.

I believe it should be, so that the default is to have no filtering (our prior behavior).

@@ -62,7 +62,6 @@ backend_packages: +[
"pants.contrib.googlejavaformat",
"pants.contrib.jax_ws",
"pants.contrib.scalajs",
"pants.contrib.mypy",

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 3, 2019

Contributor

Why make this change?

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Aug 3, 2019

Contributor

This is needed because the existing

./pants --exclude-target-regexp='testprojects/.*' --changed-parent="${MERGE_BASE}" lint

will fail on c++ related code on resolving different markdown versions, and we don't have a good way to avoid it. You can sanity check by

./pants --pants-backends=pants.contrib.mypy --exclude-target-regexp='testprojects/.*' lint ::

Internally we can still enable it by default because we don't lint/test a bunch unrelated c++ targets in the same context.

@@ -302,9 +301,6 @@ skip: True
[pycheck-newstyle-classes]
skip: True

[mypy]
config_file: build-support/mypy/mypy.ini

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 3, 2019

Contributor

See above about reverting this.

This comment has been minimized.

Copy link
@mabdi3

mabdi3 Aug 3, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@mabdi3

mabdi3 Aug 5, 2019

Author Contributor

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).

This comment has been minimized.

Copy link
@stuhood

stuhood Aug 5, 2019

Member

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.

Mohamed Abdi added some commits Aug 5, 2019

@wisechengyi wisechengyi merged commit 1c85b46 into pantsbuild:master Aug 7, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

stuhood added a commit to twitter/pants that referenced this pull request Aug 7, 2019

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

### Problem

We want to move MyPy into the lint goal and allow for users to opt-in into type_checking
for the new whitelisting/opt-in logic, we can make use of the lint goal capabilities instead of rebuilding the wheel
See pantsbuild#7886 and pantsbuild#6742 for more context.

### Solution

Registers MyPy as a 'lint' goal, and implements opt-in 'whitelist' system for type checking.
Targets to be type checked are tagged according to the `--whitelisted-tag-name` option, defaults to `type_checked` as used from pantsbuild#7886. Add new mypy_tests for new logic, and update old tests and mypy instances to reflect lint goal change.

New mypy logic added to accommodate new opt-in strategy. Mypy will acts according to the following:
1. Filter out non-whitelisted target roots
2. Check if the transitive deps from the filtered roots are whitelisted, if not we throw a warning
3. Continue as normal, collecting python source files and running mypy

wisechengyi added a commit that referenced this pull request Aug 8, 2019

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

### Problem

We want to move MyPy into the lint goal and allow for users to opt-in into type_checking
for the new whitelisting/opt-in logic, we can make use of the lint goal capabilities instead of rebuilding the wheel
See #7886 and #6742 for more context.

### Solution

Registers MyPy as a 'lint' goal, and implements opt-in 'whitelist' system for type checking.
Targets to be type checked are tagged according to the `--whitelisted-tag-name` option, defaults to `type_checked` as used from #7886. Add new mypy_tests for new logic, and update old tests and mypy instances to reflect lint goal change.

New mypy logic added to accommodate new opt-in strategy. Mypy will acts according to the following:
1. Filter out non-whitelisted target roots
2. Check if the transitive deps from the filtered roots are whitelisted, if not we throw a warning
3. Continue as normal, collecting python source files and running mypy
'contrib/mypy/src/python/pants/contrib/mypy/tasks',
],
tags = {'integration_test'},

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 17, 2019

Contributor

@wisechengyi do you know why this change was introduced?

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Aug 17, 2019

Contributor

Do you mean the tag? It was probably because it started out as integration tests, but we kept unit in the end, so please feel free to remove.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.