Use pylint to look for issues on every pull request

[cclauss]

Confirms the need for #146

flake8 testing of https://github.com/QISKit/qiskit-sdk-py on Python 3.6.3

$ flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics

./qiskit/dagcircuit/_dagcircuit.py:570:49: F821 undefined name 'n'
            nd = input_circuit.multi_graph.node[n]
                                                ^

./qiskit/tools/visualization.py:463:70: E999 SyntaxError: EOL while scanning string literal
    """Plot the equal angle slice spin Wigner function of an arbitrary
                                                                     ^

./qiskit/tools/qcvv/tomography.py:805:17: E999 IndentationError: expected an indented block
                W[wpt] = W[wpt]+(x[i]/shots)*parity[i]
                ^

2     E999 IndentationError: expected an indented block
1     F821 undefined name 'n'
3

Description

Add flake8 to the testing to find syntax errors and undefined names. Revisits #8

Motivation and Context

We want highly reliable software that avoids runtime issues. E901,E999,F821,F822,F823 are flake8 issues than can negatively impact runtime.

How Has This Been Tested?

By running Travis CI on this and all future PRs

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed. These changes find bugs which cause the build to fail!!

[markjeveritt]

@diego-plan9 Not sure if we are expected to take any action but the flake8 output:

./qiskit/tools/qcvv/tomography.py:805:17: E999 IndentationError: expected an indented block
W[wpt] = W[wpt]+(x[i]/shots)*parity[I]

is in the same place as the indentation error that happened last time - could it be being introduced by this process?

[diego-plan9]

Thanks @cclauss - we fully agree that we should try to minimize the amount of runtime problems and make use of automated aids to catch those as soon as possible, and it seems indeed that a couple of those errors crept into the codebase!

However, as mentioned in the documentation and in #80, we are aiming to use pylint for our linting needs - calling the errors you caught "linting" is a bit debatable as they are indeed more sever, but nevertheless introducing flake8 only for those specific error codes seems to be a bit of overlap. We also need to consider that at the moment we cannot enforce the full pylint configuration file, as there are still many warnings to be fixed.

With that in mind, could you:

• replace flake8 with pylint on the modified line on the travis config file, trying to find a sensible subset of errors? In particular, something along the lines of:

  pylint  qiskit test --errors-only
  

  seems reasonable - could you also provide some insight on your rationale for selecting the E901,E999,F821,F822,F823 errors if that is not the case? (Also, you might need to use --extension-pkg-whitelist if the numpy/scipy/etc cause some false positives).
• fix the errors raised by the command? It seems the last two are fixed by Wigner function functionality: Fixed an error #146 , but we would need to have them fixed before merging this PR in order to avoid the rest of the PRs failing. If it is not a trivial fix, please let us know so we can take a deeper look.

[diego-plan9]

@diego-plan9 Not sure if we are expected to take any action but the flake8 output:
...

No need to take action on this PR, @markjeveritt, as those errors should be fixed by your #146 PR!

[cclauss]

introducing flake8 only for those specific error codes seems to be a bit of overlap

This would be the case if and only if pylint could catch these errors.

Flake8 is a wrapper around PyFlakes, pep8, and McCabe complexity analysis so it often catches issues that are not seen by a single tool.

• E999 was selected because these are halt-the-runtime issues like syntax errors, indentation errors, un-terminated strings, etc. If Python was a compiled language, then these would be compile-time errors.
• F821 was selected because undefined names can raise NameError at runtime. These are often missing imports, misspelled variable names, missing self., or copy-and-paste issues

If pylint is unable to find such issues, then adding a tool that can in a targeted way seems useful.

[cclauss]

I put flake8 into --exit-zero mode so we can see both flake8 output and pylint output side-by-side.

The good news that that pylint also catches the F821 error. The bad news is that I now understand what you mean by false positives. Pylint seems to be baffled about important methods like numpy.tan() and numpy.random.choice().

[atilag]

Hi @cclauss, I didn't know about flake8 and seems like it could be a good alternative to pylint in fact. Having said that, I do agree with @diego-plan9 on having just one tool for lintering, the one we choose is pretty "standard" I'd say, but we would love to hear from other options that could fit better. Therefore I was going to suggest opening a new issue and move the discussion there.

[diego-plan9]

The good news that that pylint also catches the F821 error. The bad news is that I now understand what you mean by false positives. Pylint seems to be baffled about important methods like numpy.tan() and numpy.random.choice().

Thanks for the confirmation! Can you try using:

pylint qiskit --errors-only --extension-pkg-whitelist=numpy

for working around the numpy magic by allowing pylint to load the module and hopefully see the imports and valid, and dropping the test dir (as it seems reasonable to assume that errors on the test files will actually be caught during the test execution)?

It seems to produce 3 errors on my setup (one of them related to numpy that looks like a real error and might meant to be scipty.linalg.expm instead), that should be taken care of before applying the travis changes - if you are up for fixing them, it would be great, but please let us know if it is not the case.

Also, agreeing with @atilag , could you please remove the flake related code for this iteration, and retake the conversion in the mentioned issue? I would personally like to explore it a bit more eventually!

[cclauss]

Pylint now finds three issues. I am unable to suggest fixes for the first two. I have suggested a fix for the last issue at f49975e#commitcomment-25627794

On the other two issues:

• ************* Module qiskit.dagcircuit._dagcircuit
• E:570,48: Undefined variable 'n' (undefined-variable)

I think the n is supposed to be node but that is just a hunch. It would be best if someone more knowledgeable of this code found the correct solution.

• ************* Module qiskit.mapper._compiling
• E:241,11: Module 'numpy.linalg' has no 'expm' member (no-member)

I believe that this should be scipy.linalg.expm instead of numpy.linalg.expm. This is because numpy.linalg does not have an expm() routine and because scipy.linalg is a superset of numpy.linalg.

[diego-plan9]

Thanks for looking into the errors, @cclauss! Specially for taking the time for digging into the expm one, which seems indeed a bit problematic and hard to track. I will follow up with some commits on your branch shortly and hopefully get the PR ready to be merged soon.