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

mabdi3 · 2019-07-23T19:17:19Z

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:

Filter out non-whitelisted target root
Check if all targets now in context are whitelisted, if not we throw an error
Continue as normal, collecting python source files and running mypy

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 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 pantsbuild#7886 and pantsbuild#6742 for more context

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

Eric-Arellano

Thanks Mohammed!

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

contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/BUILD

contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/test_mypy.py

Eric-Arellano · 2019-07-24T00:33:15Z

build-support/bin/mypy.py

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


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.

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.

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

Change tests to reflect new logic General code review fixes

mabdi3 · 2019-07-24T22:40:32Z

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

+  deprecated_options_scope = 'mypy'
+  deprecated_options_scope_removal_version = '1.20.0.dev2'
+
+  WARNING_MESSAGE = None


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.

You can hardcode the value here.

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

stuhood · 2019-07-26T00:15:31Z

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

mabdi3 · 2019-07-26T04:16:24Z

@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!

wisechengyi

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

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

wisechengyi · 2019-07-26T04:35:22Z

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

+  deprecated_options_scope = 'mypy'
+  deprecated_options_scope_removal_version = '1.20.0.dev2'
+
+  WARNING_MESSAGE = None


You can hardcode the value here.

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

contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/test_mypy.py

…serting a msg

stuhood

Thanks! Looks great.

contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/BUILD

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

wisechengyi · 2019-07-29T16:46:53Z

will merge once the linty issue is fixed.

[31mERROR: All .py files other than __init__.py should start with the header:

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

…anges

…d include verbose in ci lint check

…di3/pants into mabdi3-mabdi/move-mypy-to-lint-goal

build-support/githooks/pre-commit

wisechengyi

Thanks! just a nit thing

wisechengyi · 2019-08-02T23:07:09Z

build-support/githooks/pre-commit

@@ -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\`"  


nit: let's leave this this file untouched.

Eric-Arellano

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 🎉!

Eric-Arellano · 2019-08-03T01:16:37Z

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

+  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


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.

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

Eric-Arellano · 2019-08-03T01:18:43Z

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

@@ -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',


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.

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

Eric-Arellano · 2019-08-03T01:20:44Z

pants.ini

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


Why make this change?

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.

Eric-Arellano · 2019-08-03T01:20:57Z

pants.ini

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

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


See above about reverting this.

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.

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

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.

…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

…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

Eric-Arellano · 2019-08-17T06:02:43Z

contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/BUILD

+    'contrib/mypy/src/python/pants/contrib/mypy/tasks',
+  ],
+  tags = {'integration_test'},


@wisechengyi do you know why this change was introduced?

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.

Refactors #8099. This fixes always printing a warning no matter what about untyped targets. Now we only print this when there actually are untyped targets.

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

stuhood requested review from stuhood, Eric-Arellano and wisechengyi July 23, 2019 23:23

stuhood reviewed Jul 23, 2019

View reviewed changes

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

Eric-Arellano reviewed Jul 24, 2019

View reviewed changes

Change logic to warn users instead of blocking users with errors

ca47558

Change tests to reflect new logic General code review fixes

mabdi3 commented Jul 24, 2019

View reviewed changes

filter targets to exclude tags that may be described in pants.ini

73c24fd

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 reviewed Jul 26, 2019

View reviewed changes

update tests to mock out function and assert its called instead of as…

6822729

…serting a msg

stuhood approved these changes Jul 26, 2019

View reviewed changes

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 Show resolved Hide resolved

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

code review changes

bca96d2

Eric-Arellano reviewed Jul 29, 2019

View reviewed changes

no warning if whitelist option not specified and style/code review ch…

60a8638

…anges

stuhood added the needs-cherrypick label Jul 29, 2019

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

Mohamed Abdi added 7 commits July 29, 2019 16:08

refactor test cases

d908bee

add required header

c758e59

add year to copyright note/comment

2186531

format import order

80c666b

skip my lint check in ci

0cb69f4

move ci mypy check to be under lint

b45a94d

specify mypy lint tag name and add default tag in pants.ini

dff9175

mabdi3 force-pushed the mabdi/move-mypy-to-lint-goal branch from f1e6697 to dff9175 Compare July 30, 2019 23:21

Mohamed Abdi added 3 commits July 30, 2019 20:44

python style issues

27adf2b

restore previous ci mypy checking, add new verbose option for mypy an…

3cfb3fa

…d include verbose in ci lint check

style issues

f8e90c1

cosmicexplorer mentioned this pull request Aug 1, 2019

pin tensorflow example to 1.13.1 #8135

Merged

stuhood mentioned this pull request Aug 1, 2019

tests/python/pants_test/base:exception_sink_integration is flaky #8127

Closed

wisechengyi added 2 commits August 1, 2019 21:19

Merge branch 'mabdi/move-mypy-to-lint-goal' of https://github.com/mab…

dd9909a

…di3/pants into mabdi3-mabdi/move-mypy-to-lint-goal

Merge branch 'master' into mabdi3-mabdi/move-mypy-to-lint-goal

90cd103

wisechengyi reviewed Aug 2, 2019

View reviewed changes

build-support/githooks/pre-commit Outdated Show resolved Hide resolved

fix lint ci shard

a3c163a

wisechengyi approved these changes Aug 2, 2019

View reviewed changes

Eric-Arellano reviewed Aug 3, 2019

View reviewed changes

Mohamed Abdi added 2 commits August 5, 2019 10:27

minor changes to ci , pants.ini and mypy task

3f41b85

convert pants ini option into command line option

f70ac76

wisechengyi mentioned this pull request Aug 5, 2019

fix conflicting markdown lib for c++ targets so mypy task can be registered by default #8142

Closed

Mohamed Abdi added 5 commits August 5, 2019 14:03

refactor integration test

1049a84

update depcrecated_options_scope

7282342

test

27ad8a5

update deprecated options scope

98dd205

update timeout for failing test

2d72f08

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

stuhood removed the needs-cherrypick label Aug 7, 2019

Eric-Arellano reviewed Aug 17, 2019

View reviewed changes

Eric-Arellano mentioned this pull request Oct 4, 2019

Refactor MyPy whitelist mechanism from #8099 #8394

Merged

Eric-Arellano added a commit that referenced this pull request Oct 5, 2019

Refactor MyPy whitelist mechanism from #8099 (#8394)

93e0e16

Refactors #8099. This fixes always printing a warning no matter what about untyped targets. Now we only print this when there actually are untyped targets.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mabdi3 commented Jul 23, 2019 •

edited

Loading

Eric-Arellano left a comment

Eric-Arellano Jul 24, 2019

mabdi3 Jul 24, 2019 •

edited

Loading

stuhood Jul 26, 2019

mabdi3 Jul 24, 2019

wisechengyi Jul 26, 2019

stuhood commented Jul 26, 2019

mabdi3 commented Jul 26, 2019

wisechengyi left a comment

wisechengyi Jul 26, 2019

stuhood left a comment

wisechengyi commented Jul 29, 2019

wisechengyi left a comment

wisechengyi Aug 2, 2019

Eric-Arellano left a comment

Eric-Arellano Aug 3, 2019

Eric-Arellano Aug 3, 2019

Eric-Arellano Aug 3, 2019

wisechengyi Aug 3, 2019

Eric-Arellano Aug 3, 2019

mabdi3 Aug 3, 2019

mabdi3 Aug 5, 2019

stuhood Aug 5, 2019

Eric-Arellano Aug 17, 2019

wisechengyi Aug 17, 2019

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

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

Conversation

mabdi3 commented Jul 23, 2019 • edited Loading

Problem

Solution

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mabdi3 Jul 24, 2019 • edited Loading

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

stuhood commented Jul 26, 2019

mabdi3 commented Jul 26, 2019

wisechengyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

wisechengyi commented Jul 29, 2019

wisechengyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano 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

mabdi3 commented Jul 23, 2019 •

edited

Loading

mabdi3 Jul 24, 2019 •

edited

Loading