From c88167d2ed687695db56bab231e54c5537f76796 Mon Sep 17 00:00:00 2001 From: "davidriazati@fb.com" Date: Fri, 7 May 2021 15:20:56 -0700 Subject: [PATCH] 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 --- .github/workflows/lint.yml | 18 +- .github/workflows/test_tools.yml | 3 +- Makefile | 32 +-- tools/actions_local_runner.py | 331 +++++++++++++++++------- tools/test/test_actions_local_runner.py | 198 +++++++++++--- 5 files changed, 408 insertions(+), 174 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 1b3caf8fb8e..8d38cdd7e43 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -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 diff --git a/.github/workflows/test_tools.yml b/.github/workflows/test_tools.yml index f3995f7bd29..e4fd65935dc 100644 --- a/.github/workflows/test_tools.yml +++ b/.github/workflows/test_tools.yml @@ -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' diff --git a/Makefile b/Makefile index 6c7bdb2f600..a573c0e1db7 100644 --- a/Makefile +++ b/Makefile @@ -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 \ diff --git a/tools/actions_local_runner.py b/tools/actions_local_runner.py index 09b211df5eb..9a1fda20d1a 100755 --- a/tools/actions_local_runner.py +++ b/tools/actions_local_runner.py @@ -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() diff --git a/tools/test/test_actions_local_runner.py b/tools/test/test_actions_local_runner.py index 2de1410d663..887a4ca5a78 100644 --- a/tools/test/test_actions_local_runner.py +++ b/tools/test/test_actions_local_runner.py @@ -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()