Respect .ini for flake8 and mypy (#57752)

Summary:
Previously `make quicklint` would lint all changed files for both mypy `ini`s, regardless of whether that file was actually supposed to be run under that configuration. This PR fixes that so we are using `tools/mypy_wrapper.py` to check if files should be included.

There's a similar change for `flake8` so that it now only outputs errors once and correctly excludes the paths in `.flake8`.

This also adds a bunch of tests to ensure that `make lint` and `make quicklint` both work and that `make quicklint` is excluding and including what it should.

Fixes #57644
](https://our.intern.facebook.com/intern/diff/28259692/)
Pull Request resolved: https://github.com/pytorch/pytorch/pull/57752

Pulled By: driazati

Reviewed By: samestep

Differential Revision: D28259692

fbshipit-source-id: 233d355781230f11f98a6f61e2c07e9f5e737e24
This commit is contained in:
davidriazati@fb.com
2021-05-07 15:20:56 -07:00
committed by Facebook GitHub Bot
parent 18fed3dfbe
commit c88167d2ed
5 changed files with 408 additions and 174 deletions

View File

@ -6,12 +6,6 @@ on:
- master
pull_request:
# note [local lint]
# LOCAL_FILES in the jobs below is intended to be filled by local (i.e. on a
# developer's machine) job in tools/actions_local_runner.py to pass only the
# changed files to whatever tool is consuming the files. This helps speed up
# local lints while keeping the linting code unified between local / CI.
jobs:
quick-checks:
runs-on: ubuntu-18.04
@ -196,13 +190,9 @@ jobs:
pip install -r requirements-flake8.txt
flake8 --version
- name: Run flake8
env:
# See note [local lint]
LOCAL_FILES: ""
run: |
set -eux
# shellcheck disable=SC2086
flake8 $LOCAL_FILES | tee "${GITHUB_WORKSPACE}"/flake8-output.txt
flake8 | tee "${GITHUB_WORKSPACE}"/flake8-output.txt
- name: Translate annotations
if: github.event_name == 'pull_request'
env:
@ -383,10 +373,6 @@ jobs:
time python -mtools.codegen.gen -s aten/src/ATen -d build/aten/src/ATen
time python -mtools.pyi.gen_pyi --native-functions-path aten/src/ATen/native/native_functions.yaml --deprecated-functions-path "tools/autograd/deprecated.yaml"
- name: Run mypy
env:
# See note [local lint]
LOCAL_FILES: ""
run: |
set -eux
# shellcheck disable=SC2086
for CONFIG in mypy*.ini; do mypy --config="$CONFIG" $LOCAL_FILES; done
for CONFIG in mypy*.ini; do mypy --config="$CONFIG"; done

View File

@ -25,6 +25,7 @@ jobs:
run: |
set -eux
pip install -r requirements.txt
pip install boto3==1.16.34 mypy==0.812
pip install boto3==1.16.34
make setup_lint
- name: Run tests
run: python -m unittest discover -vs tools/test -p 'test_*.py'

View File

@ -39,9 +39,16 @@ setup_lint:
python tools/actions_local_runner.py --file .github/workflows/lint.yml \
--job 'mypy' --step 'Install dependencies' --no-quiet
# TODO: This is broken on MacOS (it downloads a Linux binary)
python tools/actions_local_runner.py --file .github/workflows/lint.yml \
--job 'quick-checks' --step 'Install ShellCheck' --no-quiet
@if [ "$$(uname)" = "Darwin" ]; then \
if [ -z "$$(which brew)" ]; then \
echo "'brew' is required to install ShellCheck, get it here: https://brew.sh "; \
exit 1; \
fi; \
brew install shellcheck; \
else \
python tools/actions_local_runner.py --file .github/workflows/lint.yml \
--job 'quick-checks' --step 'Install ShellCheck' --no-quiet; \
fi
pip install jinja2
quick_checks:
@ -67,30 +74,15 @@ quick_checks:
flake8:
@python tools/actions_local_runner.py \
--file .github/workflows/lint.yml \
--file-filter '.py' \
$(CHANGED_ONLY) \
--job 'flake8-py3' \
--step 'Run flake8'
@python tools/actions_local_runner.py \
--file .github/workflows/lint.yml \
--file-filter '.py' \
$(CHANGED_ONLY) \
--job 'flake8-py3' \
--step 'Fail if there were any warnings'
--job 'flake8-py3'
mypy:
@if [ -z "$(CHANGED_ONLY)" ]; then \
python tools/actions_local_runner.py --file .github/workflows/lint.yml --job 'mypy' --step 'Run autogen'; \
else \
echo "mypy: Skipping typestub generation"; \
fi
@python tools/actions_local_runner.py \
--file .github/workflows/lint.yml \
--file-filter '.py' \
$(CHANGED_ONLY) \
--job 'mypy' \
--step 'Run mypy'
--job 'mypy'
cmakelint:
@python tools/actions_local_runner.py \

View File

@ -1,4 +1,5 @@
#!/usr/bin/env python3
# -*- coding: utf-8 -*-
import subprocess
import sys
@ -8,11 +9,15 @@ import yaml
import asyncio
import shutil
import re
from typing import List, Dict, Any, Optional
import fnmatch
import shlex
import configparser
from typing import List, Dict, Any, Optional, Tuple, Union
REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
class col:
HEADER = "\033[95m"
BLUE = "\033[94m"
@ -25,7 +30,10 @@ class col:
def color(the_color: str, text: str) -> str:
return col.BOLD + the_color + str(text) + col.RESET
if hasattr(sys.stdout, "isatty") and sys.stdout.isatty():
return col.BOLD + the_color + str(text) + col.RESET
else:
return text
def cprint(the_color: str, text: str) -> None:
@ -70,18 +78,152 @@ def find_changed_files() -> List[str]:
return [x.strip() for x in all_files if x.strip() != ""]
async def run_step(step: Dict[str, Any], job_name: str, files: Optional[List[str]], quiet: bool) -> bool:
def print_results(job_name: str, passed: bool, streams: List[str]) -> None:
header(job_name, passed)
for stream in streams:
stream = stream.strip()
if stream != "":
print(stream)
async def shell_cmd(
cmd: Union[str, List[str]],
env: Optional[Dict[str, Any]] = None,
redirect: bool = True,
) -> Tuple[bool, str, str]:
if isinstance(cmd, list):
cmd_str = shlex.join(cmd) # type: ignore[attr-defined]
else:
cmd_str = cmd
proc = await asyncio.create_subprocess_shell(
cmd_str,
shell=True,
cwd=REPO_ROOT,
env=env,
stdout=subprocess.PIPE if redirect else None,
stderr=subprocess.PIPE if redirect else None,
executable=shutil.which("bash"),
)
stdout, stderr = await proc.communicate()
passed = proc.returncode == 0
if not redirect:
return passed, "", ""
return passed, stdout.decode().strip(), stderr.decode().strip()
def header(name: str, passed: bool) -> None:
PASS = color(col.GREEN, "")
FAIL = color(col.RED, "x")
icon = PASS if passed else FAIL
print(f"{icon} {color(col.BLUE, name)}")
def get_flake_excludes() -> List[str]:
config = configparser.ConfigParser()
config.read(os.path.join(REPO_ROOT, ".flake8"))
excludes = re.split(r',\s*', config["flake8"]["exclude"].strip())
excludes = [e.strip() for e in excludes if e.strip() != ""]
return excludes
async def run_flake8(files: Optional[List[str]], quiet: bool) -> bool:
cmd = ["flake8"]
excludes = get_flake_excludes()
def should_include(name: str) -> bool:
for exclude in excludes:
if fnmatch.fnmatch(name, pat=exclude):
return False
if name.startswith(exclude) or ("./" + name).startswith(exclude):
return False
return True
if files is not None:
files = [f for f in files if should_include(f)]
if len(files) == 0:
print_results("flake8", True, [])
return True
# Running quicklint, pass in an explicit list of files (unlike mypy,
# flake8 will still use .flake8 to filter this list by the 'exclude's
# in the config
cmd += files
passed, stdout, stderr = await shell_cmd(cmd)
print_results("flake8", passed, [stdout, stderr])
return passed
async def run_mypy(files: Optional[List[str]], quiet: bool) -> bool:
if files is not None:
# Running quick lint, use mypy-wrapper instead so it checks that the files
# actually should be linted
stdout = ""
stderr = ""
passed = True
# Pass each file to the mypy_wrapper script
# TODO: Fix mypy wrapper to mock mypy's args and take in N files instead
# of just 1 at a time
for f in files:
f = os.path.join(REPO_ROOT, f)
f_passed, f_stdout, f_stderr = await shell_cmd(
[sys.executable, "tools/mypy_wrapper.py", f]
)
if not f_passed:
passed = False
if f_stdout != "":
stdout += f_stdout + "\n"
if f_stderr != "":
stderr += f_stderr + "\n"
print_results("mypy (skipped typestub generation)", passed, [stdout, stderr])
return passed
# Not running quicklint, so use lint.yml
_, _, _ = await shell_cmd(
[
sys.executable,
"tools/actions_local_runner.py",
"--job",
"mypy",
"--file",
".github/workflows/lint.yml",
"--step",
"Run autogen",
],
redirect=False,
)
passed, _, _ = await shell_cmd(
[
sys.executable,
"tools/actions_local_runner.py",
"--job",
"mypy",
"--file",
".github/workflows/lint.yml",
"--step",
"Run mypy",
],
redirect=False,
)
return passed
async def run_step(
step: Dict[str, Any], job_name: str, files: Optional[List[str]], quiet: bool
) -> bool:
env = os.environ.copy()
env["GITHUB_WORKSPACE"] = "/tmp"
if files is None:
env["LOCAL_FILES"] = ""
else:
env["LOCAL_FILES"] = " ".join(files)
script = step["run"]
PASS = color(col.GREEN, '\N{check mark}')
FAIL = color(col.RED, 'x')
if quiet:
# TODO: Either lint that GHA scripts only use 'set -eux' or make this more
# resilient
@ -89,46 +231,46 @@ async def run_step(step: Dict[str, Any], job_name: str, files: Optional[List[str
script = re.sub(r"^time ", "", script, flags=re.MULTILINE)
name = f'{job_name}: {step["name"]}'
def header(passed: bool) -> None:
icon = PASS if passed else FAIL
print(f"{icon} {color(col.BLUE, name)}")
passed, stderr, stdout = await shell_cmd(script, env=env)
print_results(name, passed, [stdout, stderr])
try:
proc = await asyncio.create_subprocess_shell(
script,
shell=True,
cwd=REPO_ROOT,
env=env,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
executable=shutil.which("bash"),
)
stdout_bytes, stderr_bytes = await proc.communicate()
header(passed=proc.returncode == 0)
except Exception as e:
header(passed=False)
print(e)
return False
stdout = stdout_bytes.decode().strip()
stderr = stderr_bytes.decode().strip()
if stderr != "":
print(stderr)
if stdout != "":
print(stdout)
return proc.returncode == 0
return passed
async def run_steps(steps: List[Dict[str, Any]], job_name: str, files: Optional[List[str]], quiet: bool) -> None:
async def run_steps(
steps: List[Dict[str, Any]], job_name: str, files: Optional[List[str]], quiet: bool
) -> bool:
coros = [run_step(step, job_name, files, quiet) for step in steps]
await asyncio.gather(*coros)
return all(await asyncio.gather(*coros))
def grab_specific_steps(steps_to_grab: List[str], job: Dict[str, Any]) -> List[Dict[str, Any]]:
def relevant_changed_files(file_filters: Optional[List[str]]) -> Optional[List[str]]:
changed_files: Optional[List[str]] = None
try:
changed_files = sorted(find_changed_files())
except Exception:
# If the git commands failed for some reason, bail out and use the whole list
print(
"Could not query git for changed files, falling back to testing all files instead",
file=sys.stderr,
)
return None
if file_filters is None:
return changed_files
else:
relevant_files = []
for f in changed_files:
for file_filter in file_filters:
if f.endswith(file_filter):
relevant_files.append(f)
break
return relevant_files
def grab_specific_steps(
steps_to_grab: List[str], job: Dict[str, Any]
) -> List[Dict[str, Any]]:
relevant_steps = []
for step in steps_to_grab:
for actual_step in job["steps"]:
@ -142,83 +284,76 @@ def grab_specific_steps(steps_to_grab: List[str], job: Dict[str, Any]) -> List[D
return relevant_steps
def grab_all_steps_after(last_step: str, job: Dict[str, Any]) -> List[Dict[str, Any]]:
relevant_steps = []
found = False
for step in job["steps"]:
if found:
relevant_steps.append(step)
if step["name"].lower().strip() == last_step.lower().strip():
found = True
return relevant_steps
def main() -> None:
parser = argparse.ArgumentParser(
description="Pull shell scripts out of GitHub actions and run them"
)
parser.add_argument("--file", help="YAML file with actions", required=True)
parser.add_argument("--file-filter", help="only pass through files with this extension", default='')
parser.add_argument("--changed-only", help="only run on changed files", action='store_true', default=False)
parser.add_argument("--job", help="job name", required=True)
parser.add_argument("--no-quiet", help="output commands", action='store_true', default=False)
parser.add_argument("--step", action="append", help="steps to run (in order)")
parser.add_argument("--file", help="YAML file with actions")
parser.add_argument(
"--all-steps-after", help="include every step after this one (non inclusive)"
"--file-filter",
help="only pass through files with this extension",
nargs="*",
)
parser.add_argument(
"--changed-only",
help="only run on changed files",
action="store_true",
default=False,
)
parser.add_argument("--job", help="job name", required=True)
parser.add_argument(
"--no-quiet", help="output commands", action="store_true", default=False
)
parser.add_argument("--step", action="append", help="steps to run (in order)")
args = parser.parse_args()
relevant_files = None
quiet = not args.no_quiet
if args.changed_only:
changed_files: Optional[List[str]] = None
try:
changed_files = find_changed_files()
except Exception:
# If the git commands failed for some reason, bail out and use the whole list
print(
"Could not query git for changed files, falling back to testing all files instead",
file=sys.stderr
relevant_files = relevant_changed_files(args.file_filter)
if args.file is None:
# If there is no .yml file provided, fall back to the list of known
# jobs. We use this for flake8 and mypy since they run different
# locally than in CI due to 'make quicklint'
if args.job not in ad_hoc_steps:
raise RuntimeError(
f"Job {args.job} not found and no .yml file was provided"
)
if changed_files is not None:
relevant_files = []
for f in changed_files:
for file_filter in args.file_filter:
if f.endswith(file_filter):
relevant_files.append(f)
break
future = ad_hoc_steps[args.job](relevant_files, quiet)
else:
if args.step is None:
raise RuntimeError("1+ --steps must be provided")
if args.step is None and args.all_steps_after is None:
raise RuntimeError("1+ --steps or --all-steps-after must be provided")
action = yaml.safe_load(open(args.file, "r"))
if "jobs" not in action:
raise RuntimeError(f"top level key 'jobs' not found in {args.file}")
jobs = action["jobs"]
if args.step is not None and args.all_steps_after is not None:
raise RuntimeError("Only one of --step and --all-steps-after can be used")
if args.job not in jobs:
raise RuntimeError(f"job '{args.job}' not found in {args.file}")
action = yaml.safe_load(open(args.file, "r"))
if "jobs" not in action:
raise RuntimeError(f"top level key 'jobs' not found in {args.file}")
jobs = action["jobs"]
job = jobs[args.job]
if args.job not in jobs:
raise RuntimeError(f"job '{args.job}' not found in {args.file}")
job = jobs[args.job]
if args.step is not None:
# Pull the relevant sections out of the provided .yml file and run them
relevant_steps = grab_specific_steps(args.step, job)
else:
relevant_steps = grab_all_steps_after(args.all_steps_after, job)
future = run_steps(relevant_steps, args.job, relevant_files, quiet)
if sys.version_info > (3, 7):
if sys.version_info >= (3, 8):
loop = asyncio.get_event_loop()
loop.run_until_complete(run_steps(relevant_steps, args.job, relevant_files, quiet))
loop.run_until_complete(future)
else:
raise RuntimeError("Only Python >3.7 is supported")
raise RuntimeError("Only Python >=3.8 is supported")
# These are run differently locally in order to enable quicklint, so dispatch
# out to special handlers instead of using lint.yml
ad_hoc_steps = {
"mypy": run_mypy,
"flake8-py3": run_flake8,
}
if __name__ == "__main__":
try:
main()

View File

@ -1,60 +1,180 @@
# -*- coding: utf-8 -*-
import textwrap
import unittest
import sys
import contextlib
import io
import os
import subprocess
import multiprocessing
from typing import List, Dict, Any
from tools import actions_local_runner
if __name__ == '__main__':
if sys.version_info >= (3, 8):
# actions_local_runner uses asyncio features not available in 3.6, and
# IsolatedAsyncioTestCase was added in 3.8, so skip testing on
# unsupported systems
class TestRunner(unittest.IsolatedAsyncioTestCase):
def run(self, *args: List[Any], **kwargs: List[Dict[str, Any]]) -> Any:
return super().run(*args, **kwargs)
if sys.version_info >= (3, 8):
# actions_local_runner uses asyncio features not available in 3.6, and
# IsolatedAsyncioTestCase was added in 3.8, so skip testing on
# unsupported systems
class TestRunner(unittest.IsolatedAsyncioTestCase):
def run(self, *args: List[Any], **kwargs: List[Dict[str, Any]]) -> Any:
return super().run(*args, **kwargs)
def test_step_extraction(self) -> None:
fake_job = {
"steps": [
{
"name": "test1",
"run": "echo hi"
},
{
"name": "test2",
"run": "echo hi"
},
{
"name": "test3",
"run": "echo hi"
},
]
}
actual = actions_local_runner.grab_specific_steps(["test2"], fake_job)
expected = [
def test_step_extraction(self) -> None:
fake_job = {
"steps": [
{
"name": "test1",
"run": "echo hi"
},
{
"name": "test2",
"run": "echo hi"
},
{
"name": "test3",
"run": "echo hi"
},
]
self.assertEqual(actual, expected)
}
async def test_runner(self) -> None:
fake_step = {
"name": "say hello",
actual = actions_local_runner.grab_specific_steps(["test2"], fake_job)
expected = [
{
"name": "test2",
"run": "echo hi"
}
f = io.StringIO()
with contextlib.redirect_stdout(f):
await actions_local_runner.run_steps([fake_step], "test", None)
},
]
self.assertEqual(actual, expected)
result = f.getvalue()
self.assertIn("say hello", result)
self.assertIn("hi", result)
async def test_runner(self) -> None:
fake_step = {
"name": "say hello",
"run": "echo hi"
}
f = io.StringIO()
with contextlib.redirect_stdout(f):
await actions_local_runner.run_steps([fake_step], "test", None, True)
result = f.getvalue()
self.assertIn("say hello", result)
self.assertIn("hi", result)
class TestEndToEnd(unittest.TestCase):
expected = [
"✓ quick-checks: Extract scripts from GitHub Actions workflows",
"✓ cmakelint: Run cmakelint",
"✓ quick-checks: Ensure no direct cub include",
"✓ quick-checks: Ensure no unqualified type ignore",
"✓ quick-checks: Ensure no unqualified noqa",
"✓ quick-checks: Ensure canonical include",
"✓ quick-checks: Ensure no non-breaking spaces",
"✓ quick-checks: Ensure no tabs",
"✓ flake8",
"✓ quick-checks: Ensure correct trailing newlines",
"✓ quick-checks: Ensure no trailing spaces",
"✓ quick-checks: Run ShellCheck",
]
def test_lint(self):
cmd = ["make", "lint", "-j", str(multiprocessing.cpu_count())]
proc = subprocess.run(cmd, cwd=actions_local_runner.REPO_ROOT, stdout=subprocess.PIPE)
stdout = proc.stdout.decode()
for line in self.expected:
self.assertIn(line, stdout)
self.assertIn("✓ mypy", stdout)
def test_quicklint(self):
cmd = ["make", "quicklint", "-j", str(multiprocessing.cpu_count())]
proc = subprocess.run(cmd, cwd=actions_local_runner.REPO_ROOT, stdout=subprocess.PIPE)
stdout = proc.stdout.decode()
for line in self.expected:
self.assertIn(line, stdout)
self.assertIn("✓ mypy (skipped typestub generation)", stdout)
class TestQuicklint(unittest.IsolatedAsyncioTestCase):
test_files = [
os.path.join("caffe2", "some_cool_file.py"),
os.path.join("torch", "some_cool_file.py"),
os.path.join("aten", "some_cool_file.py"),
os.path.join("torch", "some_stubs.pyi"),
]
maxDiff = None
def setUp(self, *args, **kwargs):
for name in self.test_files:
bad_code = textwrap.dedent("""
some_variable = '2'
some_variable = None
some_variable = 11.2
""").rstrip("\n")
with open(name, "w") as f:
f.write(bad_code)
def tearDown(self, *args, **kwargs):
for name in self.test_files:
os.remove(name)
def test_file_selection(self):
files = actions_local_runner.find_changed_files()
for name in self.test_files:
self.assertIn(name, files)
async def test_flake8(self):
f = io.StringIO()
with contextlib.redirect_stdout(f):
await actions_local_runner.run_flake8(self.test_files, True)
# Should exclude the caffe2/ file
expected = textwrap.dedent("""
x flake8
torch/some_cool_file.py:4:21: W292 no newline at end of file
aten/some_cool_file.py:4:21: W292 no newline at end of file
""").lstrip("\n")
self.assertEqual(expected, f.getvalue())
async def test_mypy(self):
self.maxDiff = None
f = io.StringIO()
with contextlib.redirect_stdout(f):
# Quicklint assumes this has been run already and doesn't work
# without it
_, _, _ = await actions_local_runner.shell_cmd(
[
f"{sys.executable}",
"tools/actions_local_runner.py",
"--job",
"mypy",
"--file",
".github/workflows/lint.yml",
"--step",
"Run autogen",
],
redirect=True,
)
await actions_local_runner.run_mypy(self.test_files, True)
# Should exclude the aten/ file
expected = textwrap.dedent("""
x mypy (skipped typestub generation)
caffe2/some_cool_file.py:3:17: error: Incompatible types in assignment (expression has type "None", variable has type "str") [assignment]
caffe2/some_cool_file.py:4:17: error: Incompatible types in assignment (expression has type "float", variable has type "str") [assignment]
torch/some_cool_file.py:3:17: error: Incompatible types in assignment (expression has type "None", variable has type "str") [assignment]
torch/some_cool_file.py:4:17: error: Incompatible types in assignment (expression has type "float", variable has type "str") [assignment]
torch/some_stubs.pyi:3:17: error: Incompatible types in assignment (expression has type "None", variable has type "str") [assignment]
torch/some_stubs.pyi:4:17: error: Incompatible types in assignment (expression has type "float", variable has type "str") [assignment]
""").lstrip("\n") # noqa: B950
self.assertEqual(expected, f.getvalue())
if __name__ == '__main__':
unittest.main()