Add ruff to local tests and CI #10116

jlapeyre · 2023-05-16T03:17:15Z

This adds linting using ruff to the relevant configuration files. Only a few rules are enabled and none of them trigger an error in the current state of qiskit-terra.

The immediate goal of this PR is to test integrating ruff into local workflows as well as CI. This is the reason why only very few rules are enabled, and none necessitated code changes.

If this PR is successful, more ruff rules will be added in subsequent PRs.

This adds linting using ruff to the relevant configuration files. Only a few rules are enabled and none of them trigger an error in the current state of the repo.

qiskit-bot · 2023-05-16T03:17:19Z

One or more of the the following people are requested to review this:

@Qiskit/terra-core

coveralls · 2023-05-16T03:43:30Z

Pull Request Test Coverage Report for Build 5126433937

0 of 0 changed or added relevant lines in 0 files are covered.
315 unchanged lines in 32 files lost coverage.
Overall coverage increased (+0.04%) to 85.944%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/routing/commuting_2q_gate_routing/commuting_2q_block.py	1	95.65%
qiskit/transpiler/passes/routing/commuting_2q_gate_routing/commuting_2q_gate_router.py	1	99.07%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	90.27%
qiskit/transpiler/passmanager_config.py	1	97.26%
qiskit/transpiler/synthesis/aqc/cnot_unit_objective.py	1	99.26%
qiskit/circuit/add_control.py	2	96.3%
qiskit/dagcircuit/collect_blocks.py	2	98.6%
qiskit/pulse/library/symbolic_pulses.py	2	95.01%
crates/qasm2/src/lex.rs	3	91.9%
qiskit/transpiler/passes/layout/disjoint_utils.py	3	97.48%

Totals
Change from base Build 4982201939:	0.04%
Covered Lines:	71234
Relevant Lines:	82884

💛 - Coveralls

mtreinish

This LGTM, I think there are probably some other low hanging fruit rules we can add that are unlikely to be controversial or cause conflicts. But that's easy enough to do in a follow up. This starts us getting used to using the tool and we can incrementally ramp things up from here until we're ready to drop pylint. Just a couple questions inline but nothing blocking

mtreinish · 2023-05-18T17:34:33Z

CONTRIBUTING.md

+running `tox -elint` which will run `black`, `ruff`, and `pylint` to check the
+local code formatting and lint. If black returns a code formatting error you can
+run `tox -eblack` to automatically update the code formatting to conform to the
+style. However, if `ruff` or `pylint` return any error you will have to fix


Ruff does have some auto fix rules which can automatically be fixed (to some degree at least) with --fix. But, probably not worth getting into that here.

No need to block this PR on this. But, I find it useful to have three top-level commands:

tox -e fmt, run all formatters

tox -e fix, run all formatters & fixers

tox -e lint, run all formatters and fixers in check-only mode, along with pure linters

Why split fix and fmt? You sometimes only want to format your code without making any semantic changes. For example, some "fixers" will remove unused import. But that import might only be unused because you are still writing the code.

We've used fmt vs fix vs lint in Pantsbuild (a build system) for the past year and it's worked great.

And we have unused imports just for the side effects. I made a comment about fixing below. I agree you have to be a bit more careful with running fix. I think something like your three commands looks good. But maybe not for this PR.

mtreinish · 2023-05-18T17:35:44Z

CONTRIBUTING.md

+You can also run just `ruff`. If you have installed the development packages in
+your python environment via `pip install -r requirements.dev`, then `ruff` will
+be available. Otherwise install it with `pip install ruff`. Running the command
+`ruff check qiskit test tools examples setup.py` will run the same `ruff` tests
+that are run via `tox` as well as in CI.


I assume you put this here because ruff is fast and people might want to run it standalone. If so black fits into this category too.

Exactly. Some people (myself included) like to avoid tox when possible because it's a bit slow and complicated. Even though I run it at least once when everything is ready. And yes, adding a note about black makes sense and I don't think complicates the PR.

mtreinish · 2023-05-18T17:37:53Z

requirements-dev.txt

@@ -11,6 +11,7 @@ black[jupyter]~=22.0
 pydot
 astroid==2.14.2
 pylint==2.16.2
+ruff==0.0.267


I think pinning is the right call, but I'm curious if ruff has any documentation on their stability policy. Like black makes it very clear there will be no default rule changes for a year/major version which is why we pin on ~=22.0.0 which should be stable but still get bugfixes. I'm assuming it's probably too early for ruff to have a policy like that.

Hmm dunno. I can check. I did see that 0.0.266 was withdrawn. But we can always deal with that kind of thing fairly easily.

Eric-Arellano

Thanks!

Eric-Arellano · 2023-05-18T17:49:30Z

CONTRIBUTING.md

+running `tox -elint` which will run `black`, `ruff`, and `pylint` to check the
+local code formatting and lint. If black returns a code formatting error you can
+run `tox -eblack` to automatically update the code formatting to conform to the
+style. However, if `ruff` or `pylint` return any error you will have to fix


No need to block this PR on this. But, I find it useful to have three top-level commands:

tox -e fmt, run all formatters

tox -e fix, run all formatters & fixers

tox -e lint, run all formatters and fixers in check-only mode, along with pure linters

Why split fix and fmt? You sometimes only want to format your code without making any semantic changes. For example, some "fixers" will remove unused import. But that import might only be unused because you are still writing the code.

We've used fmt vs fix vs lint in Pantsbuild (a build system) for the past year and it's worked great.

jlapeyre · 2023-05-18T18:18:27Z

Ruff does have some auto fix rules

I found they are not quite as fool proof as, say black. I don't recall the details. Maybe that we might want to make some exceptions. In any case, I tried to avoid complicating this first PR.

mtreinish · 2023-05-18T20:54:31Z

CONTRIBUTING.md

+Because they are so fast, it is sometimes convenient to run the tools `black` and `ruff` separately
+rather than via `tox`. If you have installed the development packages in your python environment via
+`pip install -r requirements.dev`, then `ruff` and `black` will be available. Otherwise install them
+with `pip install ruff black`. Running the commands `ruff check qiskit test tools examples setup.py`


This is potentially problematic to suggest because of our pinning. If you run pip install black for example it will pull in a 23.x.y version which will behave differently than what CI will check (I think we can look at bumping it in a separate PR as it's separate), but it might be good to include the pins here although I guess it's one more place to keep in sync.

I'd recommend we remove this paragraph for the exact problem you're describing.

One option is to have multiple environments:

tox -e fmt

tox -e fix

tox -e lint

tox -e ruff

tox -e black

tox -e pylint

The downside is that tox.ini will be more awkward, but that doesn't seem like a big deal to me. Optimize for developer experience, not how elegant a config file is that very few people touch.

Fine to remove the line about pip install...

Won't tox -e ruff be much slower than just running ruff?

Won't tox -e ruff be much slower than just running ruff?

Why? It will be annoying the first time you have to install the virtual environment. But otherwise it should be roughly the same speed.

I'm looking for a good place in the contributors doc to warn users that they should install dev tools only via requirements-dev.txt. Or at least get the correct version if they do it piecemeal (btw, Having a robust packaging system instead of talking about this would be convenient.). I notice that reno is pinned, but there is an instruction to just pip install it.

I tried a ruff target in tox.ini. It runs in 900ms. Running ruff directly is 23ms. And less if you run it only on the directory you are editing. The story with black is similar... But I don't think the comments I added there are optimal. I mean giving explicit instructions for one invocation of ruff at the cli doesn't really belong there.

Eric-Arellano · 2023-05-30T16:20:02Z

CONTRIBUTING.md

+`pip install -r requirements-dev.txt`, then `ruff` and `black` will be available and can be run from
+the command line. See [`tox.ini`](tox.ini) for how `tox` invokes them.


Nice change. I like this.

jlapeyre · 2023-05-30T16:28:49Z

I simplified the discussion 963958e (#10116) of running black and ruff in CONTRIBUTING. I think a discussion of requirements-dev.txt would be a great idea, but that it would be a distraction in this PR.
External comment on reliability of ruff autofix.
This is the only discussion of organizing addition of rules and default rules that I find. The bottom line seems the be that they are just adding stuff at 100 mph and releasing a lot.
At present running ruff remains hardcoded with no options into the existing tox test target. I also added a Makefile target. But I did not document it, again because whether and how to do that belongs in other issues and PRs. In general, I'm trying to dodge the question of how much API to offer to developers and TMTOWTDI.

Eric-Arellano

Thanks!

pyproject.toml

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

mtreinish

LGTM, thanks for doing this

## Summary Just updating the README to reflect that IBM has been using ruff for a year already: Qiskit/qiskit#10116.

Add ruff to local tests and CI

f5b3924

This adds linting using ruff to the relevant configuration files. Only a few rules are enabled and none of them trigger an error in the current state of the repo.

jlapeyre requested a review from a team as a code owner May 16, 2023 03:17

jlapeyre mentioned this pull request May 16, 2023

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

Closed

2 tasks

mtreinish previously approved these changes May 18, 2023

View reviewed changes

mtreinish added type: qa Changelog: None labels May 18, 2023

Eric-Arellano previously approved these changes May 18, 2023

View reviewed changes

Add comments on running black separately from tox

60024eb

jlapeyre dismissed stale reviews from Eric-Arellano and mtreinish via 60024eb May 18, 2023 18:40

mtreinish reviewed May 18, 2023

View reviewed changes

Simplify and remove potentially bug causing instructions in CONTRIBUTING

963958e

Eric-Arellano reviewed May 30, 2023

View reviewed changes

Eric-Arellano previously approved these changes May 30, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

jlapeyre dismissed Eric-Arellano’s stale review via 67d0038 May 30, 2023 21:29

Eric-Arellano approved these changes May 30, 2023

View reviewed changes

mtreinish approved these changes May 30, 2023

View reviewed changes

mtreinish added this pull request to the merge queue May 30, 2023

Merged via the queue into Qiskit:main with commit 901e3b8 May 30, 2023

jlapeyre mentioned this pull request Aug 15, 2023

Switch from pylint to ruff #9631

Closed

IvanIsCoding mentioned this pull request Jul 21, 2024

Add IBM to Who is Using ruff astral-sh/ruff#12433

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ruff to local tests and CI #10116

Add ruff to local tests and CI #10116

jlapeyre commented May 16, 2023

qiskit-bot commented May 16, 2023

coveralls commented May 16, 2023 •

edited

Loading

mtreinish left a comment

mtreinish May 18, 2023

Eric-Arellano May 18, 2023

jlapeyre May 18, 2023

mtreinish May 18, 2023

jlapeyre May 18, 2023

mtreinish May 18, 2023

jlapeyre May 18, 2023

Eric-Arellano left a comment

Eric-Arellano May 18, 2023

jlapeyre commented May 18, 2023

mtreinish May 18, 2023

Eric-Arellano May 18, 2023

jlapeyre May 19, 2023

Eric-Arellano May 19, 2023

jlapeyre May 19, 2023

jlapeyre May 19, 2023 •

edited

Loading

Eric-Arellano May 30, 2023

jlapeyre commented May 30, 2023 •

edited

Loading

Eric-Arellano left a comment

mtreinish left a comment

		`pip install -r requirements-dev.txt`, then `ruff` and `black` will be available and can be run from
		the command line. See [`tox.ini`](tox.ini) for how `tox` invokes them.

Add ruff to local tests and CI #10116

Add ruff to local tests and CI #10116

Conversation

jlapeyre commented May 16, 2023

qiskit-bot commented May 16, 2023

coveralls commented May 16, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5126433937

💛 - Coveralls

mtreinish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlapeyre commented May 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlapeyre May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlapeyre commented May 30, 2023 • edited Loading

Eric-Arellano left a comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

coveralls commented May 16, 2023 •

edited

Loading

jlapeyre May 19, 2023 •

edited

Loading

jlapeyre commented May 30, 2023 •

edited

Loading