Skip to content

Commit

Permalink
Add incremental pylint make target (Qiskit#6597)
Browse files Browse the repository at this point in the history
* Add incremental pylint make target

* add tox.ini and fetch/compare to origin/HEAD

* commands

* opportunistic caching

* Add small tool to avoid shell utils

* import pylint not sub-process. clean examples

* blacken few extra files

* just pylint

* update contributing.md

* tweak error msg, tox.ini

* fixup makefile, tox.ini

* command line tweaks

* add tools to lint-incr target

* Update tox.ini

* Revert "Update tox.ini"

Tox doesn't support globbing so this change wasn't valid that line was
commented on purpose. This reverts the previous tox.ini update and adds
a comment about the commented line to explain the intent.

This reverts commit c33c274.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Matthew Treinish <[email protected]>
  • Loading branch information
3 people authored Jul 23, 2021
1 parent fbf389f commit 105cba3
Show file tree
Hide file tree
Showing 4 changed files with 143 additions and 14 deletions.
28 changes: 17 additions & 11 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -327,38 +327,38 @@ we used in our CI systems more closely.

### Snapshot Testing for Visualizations

If you are working on code that makes changes to any matplotlib visualisations
you will need to check that your changes don't break any snapshot tests, and add
If you are working on code that makes changes to any matplotlib visualisations
you will need to check that your changes don't break any snapshot tests, and add
new tests where necessary. You can do this as follows:

1. Make sure you have pushed your latest changes to your remote branch.
2. Go to link: `https://mybinder.org/v2/gh/<github_user>/<repo>/<branch>?urlpath=apps/test/ipynb/mpl_tester.ipynb`. For example, if your GitHub username is `username`, your forked repo has the same name the original, and your branch is `my_awesome_new_feature`, you should visit https://mybinder.org/v2/gh/username/qiskit-terra/my_awesome_new_feature?urlpath=apps/test/ipynb/mpl_tester.ipynb.
This opens a Jupyter Notebook application running in the cloud that automatically runs
This opens a Jupyter Notebook application running in the cloud that automatically runs
the snapshot tests (note this may take some time to finish loading).
3. Each test result provides a set of 3 images (left: reference image, middle: your test result, right: differences). In the list of tests the passed tests are collapsed and failed tests are expanded. If a test fails, you will see a situation like this:

<img width="995" alt="Screenshot_2021-03-26_at_14 13 54" src="https://user-images.githubusercontent.com/23662430/112663508-d363e800-8e50-11eb-9478-6d665d0ff086.png">
4. Fix any broken tests. Working on code for one aspect of the visualisations
can sometimes result in minor changes elsewhere to spacing etc. In these cases
4. Fix any broken tests. Working on code for one aspect of the visualisations
can sometimes result in minor changes elsewhere to spacing etc. In these cases
you just need to update the reference images as follows:
- download the mismatched images (link at top of Jupyter Notebook output)
- unzip the folder
- copy and paste the new images into `qiskit-terra/test/ipynb/mpl/references`,
- copy and paste the new images into `qiskit-terra/test/ipynb/mpl/references`,
replacing the existing reference images
- add, commit and push your changes, then restart the Jupyter Notebook app in your browser. The
- add, commit and push your changes, then restart the Jupyter Notebook app in your browser. The
tests should now pass.
5. Add new snapshot tests covering your new features, extensions, or bugfixes.
- add your new snapshot tests to `test/ipynb/mpl/test_circuit_matplotlib_drawer.py`
, where you can also find existing tests to use as a guide.
- commit and push your changes, restart the Jupyter Notebook app in your browser.
As this is the first time you run your new tests there won't be any reference
images to compare to. Instead you should see an option in the list of tests
- commit and push your changes, restart the Jupyter Notebook app in your browser.
As this is the first time you run your new tests there won't be any reference
images to compare to. Instead you should see an option in the list of tests
to download the new images, like so:

<img width="1002" alt="Screenshot_2021-03-26_at_15 38 31" src="https://user-images.githubusercontent.com/23662430/112665215-b9c3a000-8e52-11eb-89e7-b18550718522.png">

- download the new images, then copy and paste into `qiskit-terra/test/ipynb/mpl/references`
- add, commit and push your changes, restart the Jupyter Notebook app in your browser. The
- add, commit and push your changes, restart the Jupyter Notebook app in your browser. The
new tests should now pass.

Note: If you have run `test/ipynb/mpl_tester.ipynb` locally it is possible some file metadata has changed, **please do not commit and push changes to this file unless they were intentional**.
Expand All @@ -379,6 +379,12 @@ run `tox -eblack` to automatically update the code formatting to conform to
the style. However, if `pylint` returns any error you will have to fix these
issues by manually updating your code.

Because `pylint` analysis can be slow, there is also a `tox -elint-incr` target, which only applies
`pylint` to files which have changed from the source github. On rare occasions this will miss some
issues that would have been caught by checking the complete source tree, but makes up for this by
being much faster (and those rare oversights will still be caught by the CI after you open a pull
request).

## Development Cycle

The development cycle for qiskit-terra is all handled in the open using
Expand Down
14 changes: 12 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ else
CONCURRENCY := $(shell echo "$(NPROCS) 2" | awk '{printf "%.0f", $$1 / $$2}')
endif

.PHONY: env lint test test_ci
.PHONY: default env lint lint-incr style black test test_randomized pytest pytest_randomized test_ci coverage coverage_erase clean

default: style lint-incr test ;

# Dependencies need to be installed on the Anaconda virtual environment.
env:
Expand All @@ -47,7 +49,15 @@ env:
lint:
pylint -rn qiskit test tools
tools/verify_headers.py qiskit test tools examples
pylint -rn --disable='C0103, C0114, W0621' examples/python/*.py
pylint -rn --disable='invalid-name, missing-module-docstring, redefined-outer-name' examples/python/*.py
tools/find_optional_imports.py

# Only pylint on files that have changed from origin/main. Also parallelize (disables cyclic-import check)
lint-incr:
-git fetch -q https://github.com/Qiskit/qiskit-terra.git :lint_incr_latest
tools/pylint_incr.py -j4 -rn -sn --paths :/qiskit/*.py :/test/*.py :/tools/*.py
tools/pylint_incr.py -j4 -rn -sn --disable='invalid-name, missing-module-docstring, redefined-outer-name' --paths ':(glob,top)examples/python/*.py'
tools/verify_headers.py qiskit test tools examples
tools/find_optional_imports.py

style:
Expand Down
97 changes: 97 additions & 0 deletions tools/pylint_incr.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
#!/usr/bin/env python3

# This code is part of Qiskit.
#
# (C) Copyright IBM 2017, 2018.
#
# This code is licensed under the Apache License, Version 2.0. You may
# obtain a copy of this license in the LICENSE.txt file in the root directory
# of this source tree or at http://www.apache.org/licenses/LICENSE-2.0.
#
# Any modifications or derivative works of this code must retain this
# copyright notice, and modified files need to carry a notice indicating
# that they have been altered from the originals.

"""Run pylint incrementally on only changed files"""

import subprocess
import argparse
import os
import sys

from pylint import lint

ROOT_DIR = os.path.dirname(os.path.abspath(__file__))


def _minimal_ext_cmd(cmd):
# construct minimal environment
env = {}
for k in ["SYSTEMROOT", "PATH"]:
v = os.environ.get(k)
if v is not None:
env[k] = v
# LANGUAGE is used on win32
env["LANGUAGE"] = "C"
env["LANG"] = "C"
env["LC_ALL"] = "C"
with subprocess.Popen(
cmd,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
env=env,
cwd=os.path.join(os.path.dirname(ROOT_DIR)),
) as proc:
stdout, stderr = proc.communicate()
return proc.returncode, stdout, stderr


def _run_pylint(ref, paths, pylint_args):
code, stdout, stderr = _minimal_ext_cmd(
[
"git",
"diff-index",
"--name-only",
"--diff-filter=d",
"--merge-base",
"-z",
ref,
"--",
*paths,
]
)
if code != 0:
print(
f"{__file__}: unable to get list of changed files. Git returncode: {code}\n"
f"Git must be installed, and you need to be in a git tree with a ref `{ref}`\n"
f"{stderr.strip().decode('ascii')}"
)
sys.exit(128)
changed_paths = [path.decode("ascii") for path in stdout.split(b"\x00") if len(path) > 0]
if len(changed_paths) == 0:
print(f"No changed files in {' '.join(paths)}")
sys.exit(0)
changed_paths_pretty = "\n ".join(changed_paths)
print(f"Running pylint on {len(changed_paths)} changed files:\n {changed_paths_pretty}")
lint.Run([*pylint_args, "--", *changed_paths])


def _main():
parser = argparse.ArgumentParser(
description="Incremental pylint.",
epilog="Unknown arguments passed through to pylint",
allow_abbrev=False,
)
parser.add_argument(
"--paths",
required=True,
type=str,
nargs="+",
help="Git <pathspec>s to resolve (and pass any changed files to pylint)",
)
args, pylint_args = parser.parse_known_args()
_run_pylint("lint_incr_latest", args.paths, pylint_args)


if __name__ == "__main__":
_main()
18 changes: 17 additions & 1 deletion tox.ini
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tox]
minversion = 2.1
envlist = py36, py37, py38, lint
envlist = py36, py37, py38, lint-incr
skipsdist = True

[testenv]
Expand All @@ -24,6 +24,22 @@ basepython = python3
commands =
black --check {posargs} qiskit test tools examples setup.py
pylint -rn qiskit test tools
# This line is commented out until #6649 merges. We can't run this currently
# via tox because tox doesn't support globbing
# pylint -rn --disable='invalid-name,missing-module-docstring,redefined-outer-name' examples/python/*.py
{toxinidir}/tools/verify_headers.py qiskit test tools examples
{toxinidir}/tools/find_optional_imports.py
reno lint

[testenv:lint-incr]
envdir = .tox/lint
basepython = python3
allowlist_externals = git
commands =
black --check {posargs} qiskit test tools examples setup.py
-git fetch -q https://github.com/Qiskit/qiskit-terra.git :lint_incr_latest
{toxinidir}/tools/pylint_incr.py -rn -j4 -sn --paths :/qiskit/*.py :/test/*.py :/tools/*.py
{toxinidir}/tools/pylint_incr.py -rn -j4 -sn --disable='invalid-name,missing-module-docstring,redefined-outer-name' --paths :(glob,top)examples/python/*.py
{toxinidir}/tools/verify_headers.py qiskit test tools examples
{toxinidir}/tools/find_optional_imports.py
reno lint
Expand Down

0 comments on commit 105cba3

Please sign in to comment.