[WIP] Add ruff configuration to pyproject.toml #9586

jlapeyre · 2023-02-15T01:24:32Z

EDIT I will close this PR as we can still refer to it when closed. But it will continue to cause confusion if it remains open.

EDIT This PR is obsolete, replaced by #10116 . The latter PR is a much smaller, more incremental way forward. I am leaving this open as a draft because the discussion will be useful for possible further ruff PRs.

This PR is an experiment in using ruff, a linting tool, as suggested by @mtreinish (#1179 (comment)). The intent is to allow people to experiment with ruff on qiskit-terra without committing yet to using it in CI.

This PR replaces pylint with ruff for linting qiskit-terra. I have added several more lint tests to the ruff configuration that are not included in this PR. They will be included in a following PR, as this one is already fairly large.

This PR should not be merged before #9689 and #9690 are. I'll also remove rules that reviewers object to or find questionable. Some of these can be reviewed in a subsequent PR.

The main ~~, or perhaps only,~~ advantage that we expect at present is an increase in performance over pylint. The gain is indeed large; two or three orders of magnitude. Using ruff also makes it easy to add pyflake plugins (reimplemented in Rust) that execute very quickly. On my thinkpad ruff check qiskit test finishes in about 350ms with an empty cache ~~28ms.~~

The set of lint checks included in this PR is not the same as the previous (pylint) set. And neither is a subset of the other.

Configuration

I have configured ruff in pyproject.toml. Alternatively, we could use a ruff-only config file. tox is configured to use ruff rather than pylint. CI has also been switched from pylint to ruff.

It includes a bit of configuration in pyproject.toml. It also disables some checks on a line-by-line basis with # noqa: XXX.

TODO

Make single overrides inline. ~~I disabled some tests in particular files via configuration in pyproject.toml.~~ These ought to done instead via # noqa: comments in the files.
Make all python files ruff clean ~~You can run > ruff check test. But you'll get many errors, as I did not yet correct or disable these.~~
~~

Notes

I did not see a way to use text representation of the error code when disabling, as you can do with pylint. Rather I have to use the error code, for example E401. I don't prefer this because it may not be obvious what you are disabling. But maybe there is a way to use a semantic tag. [EDIT: It appears there is no way.]
I corrected many of the lint errors. Most of them should be non-controversial. (But not all, I suppose) Anything controversial or more a matter of convention, I left for later discussion.

modified: qiskit/algorithms/amplitude_estimators/ae_utils.py

Cryoris · 2023-02-15T08:31:09Z

qiskit/utils/mitigation/circuits.py

@@ -114,10 +114,10 @@ def complete_meas_cal(

 def tensored_meas_cal(
    mit_pattern: List[List[int]] = None,
-    qr: Union[int, List["QuantumRegister"]] = None,
-    cr: Union[int, List["ClassicalRegister"]] = None,
+    qr: Union[int, List["QuantumRegister"]] = None, # noqa: F821


The docs say that F821 is an "undefined name error" -- do you think this is that because the type is in quotes?

Yes. That's it. I did not track down the reasoning. Of course, the name is in quotes precisely because the symbol is not defined. Maybe some people don't think this should be done, so there is a rule ?

@Cryoris Just because of this issue, I have reverted the application of this rule in 80b078b . There may be a way to sort this out, make a global exception or something. But for now, it's just in the way here.

mtreinish · 2023-02-15T12:53:18Z

So the interesting thing is black isn't happy with this PR:

https://dev.azure.com/qiskit-ci/qiskit-terra/_build/results?buildId=43701&view=logs&j=86d13450-b8b4-5d9a-22f1-9f7e71062a71&t=be4bcae7-a989-5a68-9de7-524b8fbabf91&l=12

Can you also update the tox.ini, Makefile, and .azure/lint-linux.yml to use ruff instead of pylint so we can see what using ruff looks like in ci?

Also we probably should add .ruff_cache/ to the .gitignore too.

mtreinish · 2023-02-15T12:54:54Z

pyproject.toml

+"qiskit/transpiler/passes/scheduling/calibration_creators.py" = ["F401"]
+"qiskit/transpiler/passes/scheduling/rzx_templates.py" = ["F401"]
+
+


Per the ruff docs and some of your comments on docstrings in #1179 I'm wondering if we want to add

[tool.ruff.pydocstyle] convention = "google"

here too

https://github.com/charliermarsh/ruff#does-ruff-support-numpy--or-google-style-docstrings

That's certainly worth trying.

I added some doc string checks. But in order to make this more manageable, I plan to instead defer them to a later PR (provided we actually make a switch to ruff) However, several doc string errors/suggestions made by ruff have been merged in recent PRs.

jakelishman · 2023-02-15T18:41:12Z

I think Black's problems are real, and somewhat unrelated to Ruff - in the couple of places I spot-checked, it's because there's been a suppression comment added that doesn't follow the Black rules. Same-line comments need to be separated from the content by two spaces. Black should be able to autofix them.

jlapeyre · 2023-02-15T18:48:25Z

Same-line comments need to be separated from the content by two spaces

That should be compatible with ruff.

jakelishman · 2023-02-15T19:22:36Z

Yeah sorry, I'm just saying that I don't think you've run black is all - no big deal.

jlapeyre · 2023-02-16T20:07:40Z

I'm just saying that I don't think you've run black is all

That's right. I was not clear on that point. I did not yet run black. But I need to do that.

levbishop · 2023-02-17T08:18:43Z

Also we probably should add .ruff_cache/ to the .gitignore too.

Ruff generates a .gitignore in its cache directory so it takes care of this automatically, I think.

…: E741 Ambiguous variable name: `I`" This reverts commit 0ecbe91.

This reverts commit a6fd291.

This reverts commit 812cda8.

This reverts commit 57ef618.

…ssign a `lambda` expression, use a `def`" This reverts commit 93a2bd0.

This reverts commit d577c4c.

…:39:9: E731 [*] Do not assign a `lambda` expression, use a `def`" This reverts commit de596c1.

…format" This reverts commit 2863fd4. The reason for making these changes is not clear. A desired policy for formatting should be decided if we want to enforce a rule.

The output object is different with this change. So undo it.

jlapeyre · 2023-05-18T17:28:34Z

The comments here remain useful, but I am closing this PR to avoid confusion. The current good PR is #10116

jlapeyre added 13 commits February 14, 2023 18:09

Add ruff configuration to pyproject.toml

2da0e50

Replace star import with explicit imports in fake_provider.py

535a583

Add missing imports not included in previous commit

adc0fba

Use left rather than l for identifier

57ef618

modified: qiskit/algorithms/amplitude_estimators/ae_utils.py

use "j not in" rather than "not j in"

462b3ba

Change variable names so that ruff likes them

812cda8

Use def rather than lambda

d577c4c

Comments for ruff config in pyproject.toml

e652cb3

Symbol imported twice

e9735d3

Rename variable for ruff

a6fd291

Use "is not None"

ae83178

Use "x not in" rather than "not x in"

30fed92

Squash or disablbe several ruff errors

a30f15d

This comment was marked as off-topic.

Sign in to view

Cryoris reviewed Feb 15, 2023

View reviewed changes

mtreinish linked an issue Feb 15, 2023 that may be closed by this pull request

Switch from pylint to flake8 #1179

Closed

mtreinish added type: qa Issues and PRs that relate to testing and code quality Changelog: None Do not include in changelog labels Feb 15, 2023

mtreinish reviewed Feb 15, 2023

View reviewed changes

Qiskit deleted a comment from qiskit-bot Feb 15, 2023

jlapeyre added 4 commits February 19, 2023 20:23

Add ruff to CI tools, run black on python files

7f3c267

Fix conflict

4a75693

Remove spurious trailing blank line

56c224f

Merge branch 'main' into use-ruff

49350eb

jlapeyre added 8 commits February 27, 2023 18:35

Revert "test/python/algorithms/minimum_eigensolvers/test_qaoa.py:36:1…

2c0f65d

…: E741 Ambiguous variable name: `I`" This reverts commit 0ecbe91.

Ignore ruff rule E741, Do not use variables named 'I', 'O', or 'l'

9afc502

Revert "Rename variable for ruff"

7d1f185

This reverts commit a6fd291.

Revert "Change variable names so that ruff likes them"

0783aff

This reverts commit 812cda8.

Revert "Use left rather than l for identifier"

de395df

This reverts commit 57ef618.

Use sys.float_info rather than np.finfo

d79662d

Use dictname.values() rather than .items() when key is not used

3f35fdd

Remove unused testenv:ruff environment

7a2889e

This was referenced Mar 1, 2023

Fix docstring surrounding whitespace #9689

Merged

Ruff fix C4?? "unnecessary X" #9690

Merged

jlapeyre changed the title ~~Add ruff configuration to pyproject.toml~~ [WIP] Add ruff configuration to pyproject.toml Mar 1, 2023

jlapeyre added 15 commits March 2, 2023 00:17

Merge branch 'main' into use-ruff

d016769

Remove temp file from revision control

55f408f

Merge branch 'main' into use-ruff

20c854d

Use sys.float_info.epsilon instead of numpy

ffaf874

Disable do not assign lambda expression, ruff E731

d590b72

Revert "test/python/opflow/test_gradients.py:722:9: E731 [*] Do not a…

f02371b

…ssign a `lambda` expression, use a `def`" This reverts commit 93a2bd0.

Revert "Use def rather than lambda"

adcd907

This reverts commit d577c4c.

Revert "test/python/algorithms/optimizers/utils/test_learning_rate.py…

97ebcb5

…:39:9: E731 [*] Do not assign a `lambda` expression, use a `def`" This reverts commit de596c1.

Simplify iteration over dict keys

2d659c5

Disable ruff rule F821 undefined name

80b078b

Use slightly more readable version check

03f4b6b

Merge branch 'main' into use-ruff

b60fc61

Merge branch 'main' into use-ruff

cb725b0

Revert "Add ruff rule UP031 Use format specifiers instead of percent …

83b8ddf

…format" This reverts commit 2863fd4. The reason for making these changes is not clear. A desired policy for formatting should be decided if we want to enforce a rule.

Undo wrong improvement in sorting dict

7b2ca00

The output object is different with this change. So undo it.

jlapeyre force-pushed the use-ruff branch from 316dc91 to 7b2ca00 Compare March 7, 2023 05:10

jlapeyre marked this pull request as draft May 16, 2023 17:42

jlapeyre closed this May 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add ruff configuration to pyproject.toml #9586

[WIP] Add ruff configuration to pyproject.toml #9586

jlapeyre commented Feb 15, 2023 •

edited

Loading

This comment was marked as off-topic.

Cryoris Feb 15, 2023

jlapeyre Feb 15, 2023

jlapeyre Mar 3, 2023

mtreinish commented Feb 15, 2023 •

edited

Loading

mtreinish Feb 15, 2023

jlapeyre Feb 15, 2023

jlapeyre Mar 3, 2023

jakelishman commented Feb 15, 2023

jlapeyre commented Feb 15, 2023

jakelishman commented Feb 15, 2023

jlapeyre commented Feb 16, 2023

levbishop commented Feb 17, 2023

jlapeyre commented May 18, 2023

		"qiskit/transpiler/passes/scheduling/calibration_creators.py" = ["F401"]
		"qiskit/transpiler/passes/scheduling/rzx_templates.py" = ["F401"]

[WIP] Add ruff configuration to pyproject.toml #9586

[WIP] Add ruff configuration to pyproject.toml #9586

Conversation

jlapeyre commented Feb 15, 2023 • edited Loading

Configuration

TODO

Notes

This comment was marked as off-topic.

Cryoris Feb 15, 2023

Choose a reason for hiding this comment

jlapeyre Feb 15, 2023

Choose a reason for hiding this comment

jlapeyre Mar 3, 2023

Choose a reason for hiding this comment

mtreinish commented Feb 15, 2023 • edited Loading

mtreinish Feb 15, 2023

Choose a reason for hiding this comment

jlapeyre Feb 15, 2023

Choose a reason for hiding this comment

jlapeyre Mar 3, 2023

Choose a reason for hiding this comment

jakelishman commented Feb 15, 2023

jlapeyre commented Feb 15, 2023

jakelishman commented Feb 15, 2023

jlapeyre commented Feb 16, 2023

levbishop commented Feb 17, 2023

jlapeyre commented May 18, 2023

jlapeyre commented Feb 15, 2023 •

edited

Loading

mtreinish commented Feb 15, 2023 •

edited

Loading