Pivot to using ruff for all linting#15603
Conversation
This commit switches us to fully using ruff for all the linting in CI and locally. The primary motivation for this change is to improve productivity because ruff is signficantly faster. Pylint is incredibly slow in general, but compared to ruff especially so. For example, on my laptop ruff takes 0.04 seconds to run on the qiskit/ subdirectory (after clearing the cache, with the cache populated it takes 0.025 sec) of the source tree while running pylint on the same path took 70 sec. This leads to people skipping lint locally and causes churn in CI becaus. We had started to experimenting with ruff in the past and used it for a some small set of rules but were still using pylint for the bulk of the linting in the repo. The concern at the time was a loss of lint coverage or a lot of code churn caused by migrating to a new tool. Specifically pylint does more type inference and checking that ruff doesn't. However since we started the experiment one major change in qiskit is how much work is happening in rust now vs Python. At this point any loss in lint coverage is unlikely to cause a significant problem in practice and we'll make real productivity gains by making this change.
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 21357799966Details
💛 - Coveralls |
jakelishman
left a comment
There was a problem hiding this comment.
At this point in time I'm personally ok with moving to Ruff instead of pylint.
I left several comments asking to change from Flake8 style magic codes to named rules, but I'm not 100% sure you actually can in Ruff yet (imo a rather out-of-character poor design choice from Astral).
Do we have the missing-docstring rules turned on? I'm suspicious because Ruff doesn't appear to interpret pylint: disable rules, which I know we have a lot of in the test suite to disable them. Can we make sure that the public class and function docstring rules are running on qiskit and not on the other files? I'm completely fine with disabling the module docstring rules.
Can we also clean up all the remnant # pylint: comments elsewhere in the codebase as we make this change?
| from .standard_gates import * # noqa: F403 | ||
| from .templates import * # noqa: F403 |
There was a problem hiding this comment.
Does Ruff let you do # noqa: undefined-local-with-star these days? If so, can we use named rules instead of the magic numbers?
There was a problem hiding this comment.
I think this is the shorthand PyFlakes notation here, it essentially means the same. See this doc.
There was a problem hiding this comment.
It is - my point is that only robots can know what the flake8 magic numbers actually mean without looking it up; it'd be more descriptive about what we're disabling inline if we use the human-friendly names.
There was a problem hiding this comment.
It only works with the code in ruff. I tried using undefined-local-with-import-star (the name of F403) instead of F403 and ruff didn't recognize the ignore comment.
There was a problem hiding this comment.
Can we include both the code and what it stands for?
| if TYPE_CHECKING: | ||
| from qiskit.circuit.quantumcircuitdata import CircuitInstruction |
There was a problem hiding this comment.
You can just import from qiskit.circuit within a TYPE_CHECKING block - there's no need to go into non-public interface within external modules.
|
|
||
|
|
||
| if TYPE_CHECKING: | ||
| from qiskit.circuit.annotated_operation import AnnotatedOperation |
There was a problem hiding this comment.
Similar to the other comment - this can just import from qiskit.circuit and make it easier to change in the future. No risk of cyclical imports because it's a static-check only import.
| # pylint: disable=wildcard-import,unused-wildcard-import | ||
| from qiskit.circuit.library.standard_gates import * | ||
| from qiskit.circuit.library.standard_gates import ( |
There was a problem hiding this comment.
If we're touch this:
- can we clear out the
pylintcomment too - can we swap the import to at least be from the public place
qiskit.circuit.libraryand not the undocumentedstandard_gates?
(Ideally on point 2 I'd like to do from qiskit.circuit import library as lib and just use dotted access, but that's personally style and I don't care much.)
| # These versions are pinned precisely because ruff can includes new | ||
| # on-by-default lint failures in new versions, which breaks our CI. |
There was a problem hiding this comment.
I'm not sure the rest of the sentence is true any more, but I do still think we should lock ruff.
jakelishman
left a comment
There was a problem hiding this comment.
Also there's a couple of mentions of pylint in the Makefile that could do with changing.
|
The thing I just realized is that the default ruleset is a bit more conservative than I was expecting and we should probably enable some additional check classes. But I'm thinking for this PR to just leave it as is and then do a separate PR for ramping up the ruleset we use to isolate the changes and bikeshed the changes made in isolation. |
|
I'd rather ensure that we've got a good baseline of lints turned on before we merge this, or alternatively that we merge with the small set but don't remove |
|
Well on re-reviewing the documentation they enable all the flake8 rules and a subset of the pycodestyle rules by default. Things that I thought would be, like the pyupgrade rules and the pylint subset are not enabled. I'll enable the rules and update things as necessary. |
This commit adds the docstring rules on the repo. This involves a few more changes than previous commits because there are a lot of formatting consistency rules that needed to be auto-applied. The checking also found several instances where there was missing documentation that should have been included.
This raises a good point I disabled the unused import rule |
|
Actually after taking a look at this, I think we should save adding the |
|
Oh, also if it wasn't clear I reverted the addition of the pydocstyle docstring rules: eebef5e enabling all these rules broke the docs builds and while I probably could have worked through it I'm not sure the churn was really worth it. A lot of these rules were very opinionated about formatting in non-functional ways or oddly pedantic stylistic choices. There are probably a more restrictive subset of rules we'll want to enable like |
Since this PR was first opened there have been several new ruff releases. This updates to the latest release as of this commit. The 0.15.0 release included a new default rule in the RUF ruleset which triggered on the code base which this also fixes.
jakelishman
left a comment
There was a problem hiding this comment.
I think at this point we're resolved to make this change in linter from pylint to ruff, and there are no further major complaints.
I'm going to enqueue this now so we can move on from the humongous PR, though there's a couple of comments we might want to look at to fix forwards (tidying up the pyproject.toml tool.pylint table in particular.)
| # Modulo: (19, 20) | ||
| ast.Binary.Op.MUL: _BindingPower(19, 20), | ||
| ast.Binary.Op.DIV: _BindingPower(19, 20), | ||
| # | ||
| ast.Binary.Op.ADD: _BindingPower(17, 18), | ||
| ast.Binary.Op.SUB: _BindingPower(17, 18), | ||
| # | ||
| ast.Binary.Op.SHIFT_LEFT: _BindingPower(15, 16), | ||
| ast.Binary.Op.SHIFT_RIGHT: _BindingPower(15, 16), | ||
| # | ||
| ast.Binary.Op.LESS: _BindingPower(13, 14), | ||
| ast.Binary.Op.LESS_EQUAL: _BindingPower(13, 14), | ||
| ast.Binary.Op.GREATER: _BindingPower(13, 14), | ||
| ast.Binary.Op.GREATER_EQUAL: _BindingPower(13, 14), | ||
| # | ||
| ast.Binary.Op.EQUAL: _BindingPower(11, 12), | ||
| ast.Binary.Op.NOT_EQUAL: _BindingPower(11, 12), | ||
| # | ||
| ast.Binary.Op.BIT_AND: _BindingPower(9, 10), | ||
| ast.Binary.Op.BIT_XOR: _BindingPower(7, 8), | ||
| ast.Binary.Op.BIT_OR: _BindingPower(5, 6), |
There was a problem hiding this comment.
These gaps were deliberate and it's a shame ruff deletes them, but not a big deal.
| out = self.draw() | ||
| if isinstance(out, str): | ||
| print(out) # pylint: disable=bad-builtin | ||
| print(out) |
There was a problem hiding this comment.
Technically I think this is a lint loss - we turned on the "don't use print" lint deliberately. Not worth holding the PR up over though.
There was a problem hiding this comment.
There is a lint for that in ruff IIRC. Lets add it to the list in a follow-up.
| with open(filename, "r") as dimacs_file: | ||
| with open(filename) as dimacs_file: |
There was a problem hiding this comment.
Imo ruff is wrong to change this, but not a big deal. Explicit shouldn't be penalised.
There was a problem hiding this comment.
We can disable this lint, it was one of the later rules I enabled IIRC as part of a new group I added.
|
|
||
| def __init__(self): # pylint: disable=super-init-not-called | ||
| def __init__(self): | ||
| self._inner = None |
There was a problem hiding this comment.
This seems like a potentially useful lint we're losing. Again, not a big deal.
There was a problem hiding this comment.
I didn't see any equivalent of this in https://docs.astral.sh/ruff/rules/ I might have missed it, but this is something we'll have to keep an eye out for I guess. We can also potentially open an issue on ruff about it I guess.
| # pylint: disable=super-init-not-called | ||
|
|
There was a problem hiding this comment.
This seems like another lint loss, but not worth keeping pylint for.
| @@ -162,17 +162,15 @@ def test_circuit_qasm_with_multiple_composite_circuits_with_same_name(self): | |||
| circuit.append(my_gate_inst3, [qr[0]]) | |||
| my_gate_inst3_id = id(circuit.data[-1].operation) | |||
| # pylint: disable-next=consider-using-f-string | |||
There was a problem hiding this comment.
Seems like your automation missed the pylint: disable-next statements. Let's follow up, though.
There was a problem hiding this comment.
Yep I used a regex matching pylint: disable= I didn't even know this was a thing.
| [tool.pylint.main] | ||
| extension-pkg-allow-list = [ |
There was a problem hiding this comment.
I think the entire tool.pylint table can go from pyproject.toml now, right?
In the recently merged Qiskit#15603 we migrated to using ruff as are sole Python linting tool in Qiskit. However that PR neglected to remove the stray pylint configuration in the pyproject.toml. This commit fixes that oversight and removes this configuration entries from the file.
One of the lints we previously had enabled in pylint was disallowing prints. This was added in Qiskit#13796 to prevent us from accidently committing print debug statements which was happening fairly frequently. However, in the recently merged Qiskit#15603 we migrated from pylint to use ruff and during that migration this lint was dropped. This commit corrects this oversight by enabling the ruff rule group T20 which is used to disable using print or pprint in code. One small change that this enabled was the discovery there was an unused `verbose` flag in the internal matplotlib circuit drawer APIs that was used to print every op node as it drew them. This has no real value and wasn't exposed via any public API so this commit just opts to remove the option along with the print statement.
In the recently merged #15603 we migrated to using ruff as are sole Python linting tool in Qiskit. However that PR neglected to remove the stray pylint configuration in the pyproject.toml. This commit fixes that oversight and removes this configuration entries from the file.
One of the lints we previously had enabled in pylint was disallowing prints. This was added in #13796 to prevent us from accidently committing print debug statements which was happening fairly frequently. However, in the recently merged #15603 we migrated from pylint to use ruff and during that migration this lint was dropped. This commit corrects this oversight by enabling the ruff rule group T20 which is used to disable using print or pprint in code. One small change that this enabled was the discovery there was an unused `verbose` flag in the internal matplotlib circuit drawer APIs that was used to print every op node as it drew them. This has no real value and wasn't exposed via any public API so this commit just opts to remove the option along with the print statement.
In the recently merged Qiskit#15603 we migrated from pylint to use ruff. During the development of that PR the rule group "D" for rules derived from pydocstyle was investigated but the full rule group was overly pedantic and even with disabling the most egregious rules also changed the docs for the worse (at least by autofixing many rules) so that sphinx would not succesfully build after applying the fixes. So during Qiskit#15603 the changes for this rule group were reverted. However, one of the useful rules from that group D417 which checks that all arguments are documented. This rule found real issues in the documentation and it is worthwhile to enable it. This commit enables the rule in our ruff checks and fixes the issues associated with. During this process ruff flagged one place in the `__call__` method docstring for the `TwoQubitControlledUDecomposer` class has not documented several arguments on the method. However, these arguments are unused currently. It is apparently trying to match the `__call__` signature of the other two qubit decomposers but is not using most of the arguments. Most could be conceivably supported by `TwoQubitControlledUDecomposer` and it's not clear why they were added. But, for this PR I didn't want to look into adding the support as it was out of scope. For the time being this PR suppresses the ruff rule and adds a TODO comment to start using the unused arguments.
* Enable missing argument in docstring lint In the recently merged #15603 we migrated from pylint to use ruff. During the development of that PR the rule group "D" for rules derived from pydocstyle was investigated but the full rule group was overly pedantic and even with disabling the most egregious rules also changed the docs for the worse (at least by autofixing many rules) so that sphinx would not succesfully build after applying the fixes. So during #15603 the changes for this rule group were reverted. However, one of the useful rules from that group D417 which checks that all arguments are documented. This rule found real issues in the documentation and it is worthwhile to enable it. This commit enables the rule in our ruff checks and fixes the issues associated with. During this process ruff flagged one place in the `__call__` method docstring for the `TwoQubitControlledUDecomposer` class has not documented several arguments on the method. However, these arguments are unused currently. It is apparently trying to match the `__call__` signature of the other two qubit decomposers but is not using most of the arguments. Most could be conceivably supported by `TwoQubitControlledUDecomposer` and it's not clear why they were added. But, for this PR I didn't want to look into adding the support as it was out of scope. For the time being this PR suppresses the ruff rule and adds a TODO comment to start using the unused arguments. * Update filename docstring * Revise wording on state visualization filename arg
* Enable missing argument in docstring lint In the recently merged Qiskit#15603 we migrated from pylint to use ruff. During the development of that PR the rule group "D" for rules derived from pydocstyle was investigated but the full rule group was overly pedantic and even with disabling the most egregious rules also changed the docs for the worse (at least by autofixing many rules) so that sphinx would not succesfully build after applying the fixes. So during Qiskit#15603 the changes for this rule group were reverted. However, one of the useful rules from that group D417 which checks that all arguments are documented. This rule found real issues in the documentation and it is worthwhile to enable it. This commit enables the rule in our ruff checks and fixes the issues associated with. During this process ruff flagged one place in the `__call__` method docstring for the `TwoQubitControlledUDecomposer` class has not documented several arguments on the method. However, these arguments are unused currently. It is apparently trying to match the `__call__` signature of the other two qubit decomposers but is not using most of the arguments. Most could be conceivably supported by `TwoQubitControlledUDecomposer` and it's not clear why they were added. But, for this PR I didn't want to look into adding the support as it was out of scope. For the time being this PR suppresses the ruff rule and adds a TODO comment to start using the unused arguments. * Update filename docstring * Revise wording on state visualization filename arg
Summary
This commit switches us to fully using ruff for all the linting in CI and locally. The primary motivation for this change is to improve productivity because ruff is signficantly faster. Pylint is incredibly slow in general, but compared to ruff especially so. For example, on my laptop ruff takes 0.04 seconds to run on the qiskit/ subdirectory (after clearing the cache, with the cache populated it takes 0.025 sec) of the source tree while running pylint on the same path took 70 sec. This leads to people skipping lint locally and causes churn in CI becaus.
We had started to experimenting with ruff in the past and used it for a some small set of rules but were still using pylint for the bulk of the linting in the repo. The concern at the time was a loss of lint coverage or a lot of code churn caused by migrating to a new tool. Specifically pylint does more type inference and checking that ruff doesn't. However since we started the experiment one major change in qiskit is how much work is happening in rust now vs Python. At this point any loss in lint coverage is unlikely to cause a significant problem in practice and we'll make real productivity gains by making this change.
Details and comments