Switch from pylint to flake8

[mtreinish]

What is the expected enhancement?

The pylint tool is overly pedantic, it fails over things like variable names and often is more of an annoyance than a helpful tool for enforcing consistent style in a repo. You can just look to how many of our source files have to explicitly exclude rules because they just get in the way. Alternatively we can/should use the flake8 project: http://flake8.pycqa.org/en/latest/ to enforce style (it's worth pointing out that they're both maintained by the same team). It is much more forgiving as it is more about enforcing just pep8 and some other common rules. It also integrates nicely with tox so we can use that to define our base set of rules.

[Bhargavishnu]

How about using Black https://pypi.org/project/black/#:~:text=Black%20is%20the%20uncompromising%20Python,energy%20for%20more%20important%20matters.

[ikkoham]

Hello @Bhargavishnu . Black is a very nice formarter. But it's not a linter.

By the way, Pylint 2.6 is more compatible with Black.
http://pylint.pycqa.org/en/latest/whatsnew/2.6.html

bad-continuation and bad-whitespace have been removed. black or another formatter can help you with this better than Pylint

[enavarro51]

Since we moved to black, is this issue still relevant?

[1ucian0]

closing due lack of activity and given that is probably not relevant any more. Please, reopen if needed it.

[mtreinish]

Since we moved to black, is this issue still relevant?

It is, pylint is still being run and has all the same issues outlined above. Black just made code formatting automatic for us.

[jlapeyre]

Here is one specific problem with pylint. It could be avoided by disabling the particular rule. Or disabling the rule locally (which would be almost every instance that it is flag). Or use something other than pylint. (Or petition pylint to change)

pylint flags false positives (a lot of them) for missing-raises-doc.

1. The Raises section of a docstring should not document exceptions raised when the documented API is violated. But pylint flags failing to document these exceptions. The majority of our entries in the Raises are such cases. Google and numpy style guides do suggest/require documenting unforeseen exceptions. pylint has a single optional check that flags both kinds of exceptions.
   The letter and intent of the requirement to document unforseen exceptions makes a lot of sense. Documenting exceptions due to violating the API does not. This is one way to look at it: because this would paradoxically make behavior under violation of the API part of the API
   The intent is to warn the caller that they may have to handle an exception that might be hard to predict. Eg. some system resource is not available.
   The intent is not to remind the user that the API says to pass an int and if they instead pass a float something bad will happen.

2. The only style guide I find mentioned in Qiskit documentation is the google style guide.

You should strive to document thoroughly all the public interfaces exposed using examples when necessary. For docstrings, Google Python Style Docstrings are used. This is parsed using the napoleon sphinx extension. The napoleon documentation contains a good example of how docstrings should be formatted.

4. pylint says that its param documentation checker can handle the google, numpy, and sphinx styles (i.e. formatting and markup) It doesn't explicitly say that it took this rule from these guides. But, this is as close to an explanation or reference that you'll find in this doc section.

5. The google style guide requires not documenting exceptions raised for violating the API. The numpy rule on this is similar: "This section should be used judiciously, i.e., only for errors that are non-obvious or have a large chance of getting raised." Sphinx doesn't address this (it doesn't have style-guide opinions, I think) None of the PEPs I looked at address this. It appears to come from google and numpy, but has been misunderstood.

6. In fact, there is one example in the pylint documentation for documenting exceptions. It is a very clear case of an exception handling violation of the API. The function takes two int and returns their sum. An error is raised if either param is not an int. And pylint incorrectly requires this to be documented.

In summary, I can't tell where this documentation requirement came from. It is not in any relevant (google and numpy) style guide. EDIT: (there are some who support documenting all exceptions, more in java, php, but also some in python) There is no reference or rationale given. Verifying that an API is adequately documented is very difficult. Determining if a raise in a method should be documented is also very difficult. It would be nice if the linter could do the second, and great if it could do the first. But this would require an expensive, complicated AI model. However it's easy to search for raise and Raises and complain about a mismatch. Pylint requires these Raises entries in the docstring not because it's reasonable, but because it's possible. We don't need to follow this.

Here is a discussion of documenting thrown exceptions

I think the topic is open invites polemics. For example in a comment from the post above:

Huh? Are you being dense on purpose

But, it ought to be able to resolve this without violating a COC.

Here is an opinion in the context of PHP. They say to document all exceptions. But all the examples given would be allowed by google and numpy styles.

Here is an argument for documenting all exceptions. I may know the API of the function I'm calling. And I can expect an exception to be thrown if I violate it. But, I don't know which exception is thrown. Eg If I pass an int instead of the required float, many different exception types may be thrown. As a caller, I have to know the type of error in order to catch it specifically.

[jlapeyre]

Here is a partial audit. "wrong" means an entry in Raises is incorrect according to google style, numpy style, and common sense (depends on whose sense), but is required by pylint.

If I find any that are correct, I'll mark them with "correct".
So far, I have not found a single correct use of Raises.
It seems to me that the reasonable thing to do, short of ditching pylint, is to drop the check for missing-raises-doc in our pylintrc.

The vast majority of these involve validation of input data and raising an exception if characteristics of the input data violate the API. Usually, the API is not well documented (because the linter isn't clever enough to detect this and prevent merging the PR) Often the required Raises doc serves as a poor, cryptic substitute for documentation of the API.

• result/counts.py 3 wrong, 1 mariginal (wrong in numpy style for sure)
• result/models.py 1 wrong
• result/postprocess.py 3 wrong
• result/result.py 6 wrong
• result/utils.py 3 wrong
• result/distributions/probability.py 2 wrong
• result/distributions/quasi.py 2 wrong
• result/mitigation/correlated_readout_mitigator.py 1 wrong
• result/mitigation/local_readout_mitigator.py 2 wrong
• result/mitigation/utils.py 1 wrong

[wshanks]

At this point, it might be better to skip flake8 and jump straight to ruff 🙂

I ended up here because I hit a bug in the currently pinned version of pylint with a code change I made. I noticed that the pinned version was quite old (almost two years). I tried to update it and found that many new issues were flagged. I was curious how the pylint rules were chosen for qiskit in order to know whether the new issues should be fixed or ignored and ended up here. For my personal case, I will just pylint ignore the line that was flagged.

In my experience, what distinguishes pylint from flake8 is that pylint makes an effort to inspect attribute access and function calls for correctness while flake8 only flags issues with the syntax of the code. So flake8 flags things like undefined variables and f-strings without formatted variables, while pylint will flag a function called with too many arguments. One tradeoff in these approaches is that pylint must be run in the environment that the package is installed in while flake8 can run on any Python code. Also, flake8 only evaluates files individually, so it does well for incremental checks and parallelizing while pylint needs to parse more files to process a single file.

In the past couple years, pylint has tried to shift to disabling a lot of the more annoying checks by default. Maybe the qiskit pylintrc is out of date with that direction. Still it can be annoying to keep up with new rules added in new versions of pylint. One option is to run with --errors-only and not worry about the style suggestions.

[mtreinish]

I was not familiar with ruff, I agree that looks like a very promising alternative. I agree if we're going to move away from pylint I would probably prefer to use ruff instead of flake8. I was playing a bit with it now it and it seems quite fast, but I think we'll need a bit of effort to get the configuration dialed in, but considering with a default configuration it returned in 0.08 seconds for me running on the full qiskit repository (and reporting 15284 errors, mostly line length because it defaults to 88 characters instead of our 100).

As for the differences between pylint and flake8, at least when I opened this issue almost 5 years ago I felt (and still feel this way today) that the extra level of inspection that pylint provides wasn't worth the slow execution or the other issues I outlined in the OP. At the time I failed to get a consensus around this, and it fell of my radar in the intervening years. I still feel moving to either flake8 or now ruff would be a big quality of life improvement for development in qiskit. I'll raise this up again more broadly to see if we can get a consensus around moving to ruff now since I expect a lot of people will be quite happy with execution time less than a second to full run lint checks.

[jlapeyre]

• Switch from pylint to ruff #9631 has been merged

This issue can be closed now, right?