qiskit
qiskit copied to clipboard
Qiskit
For discussion: use isort for formatting import
Summary
Terra introduced black formatter in #6361. However, there is currently no formatting for imports.
Developer's style differences regarding line breaks and ordering of imports can cause merge conflicts
as well as make code difficult to read. isort is a formatter compatible with black. The development
flow will be the same as when black was introduced, and there will be no major changes for developers.
Details and comments
if this direction looks good, I will modify contibution guidelines, etc.
References
- isort: https://github.com/PyCQA/isort
Thank you for opening a new pull request.
Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.
While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.
One or more of the the following people are requested to review this:
- @Cryoris
- @Qiskit/terra-core
- @ajavadia
- @enavarro51
- @ikkoham
- @levbishop
- @manoelmarques
- @mtreinish
- @nkanazawa1989
- @t-imamichi
- @woodsp-ibm
Pull Request Test Coverage Report for Build 2305222944
- 1076 of 1122 (95.9%) changed or added relevant lines in 481 files are covered.
- 7 unchanged lines in 7 files lost coverage.
- Overall coverage decreased (-0.05%) to 84.329%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| qiskit/dagcircuit/dagcircuit.py | 3 | 4 | 75.0% |
| qiskit/opflow/list_ops/composed_op.py | 1 | 2 | 50.0% |
| qiskit/opflow/state_fns/sparse_vector_state_fn.py | 0 | 1 | 0.0% |
| qiskit/tools/jupyter/copyright.py | 0 | 1 | 0.0% |
| qiskit/tools/jupyter/job_widgets.py | 0 | 1 | 0.0% |
| qiskit/tools/jupyter/monospace.py | 0 | 1 | 0.0% |
| qiskit/tools/jupyter/progressbar.py | 0 | 1 | 0.0% |
| qiskit/tools/jupyter/version_table.py | 0 | 1 | 0.0% |
| qiskit/tools/jupyter/watcher_monitor.py | 0 | 1 | 0.0% |
| qiskit/transpiler/passes/optimization/_gate_extension.py | 0 | 1 | 0.0% |
| <!-- | Total: | 1076 | 1122 |
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| qiskit/tools/jupyter/jupyter_magics.py | 1 | 0% |
| qiskit/transpiler/passes/basis/ms_basis_decomposer.py | 1 | 0% |
| qiskit/transpiler/passes/optimization/_gate_extension.py | 1 | 0% |
| qiskit/transpiler/passes/scheduling/calibration_creators.py | 1 | 0% |
| qiskit/transpiler/passes/scheduling/rzx_templates.py | 1 | 0% |
| qiskit/util.py | 1 | 0% |
| qiskit/visualization/pulse_v2/plotters/matplotlib.py | 1 | 0% |
| <!-- | Total: | 7 |
| Totals | |
|---|---|
| Change from base Build 2304379250: | -0.05% |
| Covered Lines: | 54292 |
| Relevant Lines: | 64381 |
💛 - Coveralls
I'll preface this by saying that I don't feel very strongly. Personally, I don't remember ever having had any serious issue caused by import order, either as a merge conflict or by having difficulty reading them. If I have had a merge conflict with them, it was so trivial to solve that I don't remember it. On the other hand, I'm concerned about the on-boarding cost to developers by adding another tool that they have to learn and configure if it's not providing us with much benefit.
My vote would be against this, as the supposed benefits don't seem sufficient to justify the cost to new or external developers, but I'm aware that's somewhat subjective and others might feel more strongly. I think I used to be more in favour of this sort of tool, but as I work with others, and particularly getting new contributors started, it's dropped in my estimations a bit. I like code formatters in general, I just don't like a fragmented ecosystem of several of them.
I know Lev's thought about doing this before as well (@levbishop).
Thanks @jakelishman. I understand what you are saying. I think there is a trade-off between quality and on-boarding cost.
I also don't remember when, but there were different developers with different isort settings, and every time they updated, they alternated the changes just like ping-pong... It might be a good idea for us not to enforce, i.e. not to introduce checking format, but only to put in a setting (pyproject.toml).
I did explore some of these ideas a few months ago on my branch https://github.com/levbishop/qiskit-terra/tree/isort If you look at the individual commits on that branch you can see different levels of how invasive changes to make:
- The first commit adds options to only group the imports into sections:
[tool.isort]
profile = "black"
line_length = 100
treat_all_comments_as_code = "True"
only_sections = "True"
and adds some comments with the treat_all_comments_as_code in order to avoid circular import errors. I think I like this approach a bit better than just excluding __init__.py as in your approach. And the second commit commit runs isort with those options
-
The next few commits add
from __future__ import annotationsto all files, and runs pyupgrade and black to enable the associated type annotation improvements with some manual cleanup. Overall I like the readability improvements from this, but I'm not sure if it's a good idea to commit to it before python 3.11 is released with official mandatory requirement of PEP 563 (it was already bumped from python 3.10). -
The next few commits use absolufy to replace absolute imports, which I don't think we've discussed much, but if we're going to have all this churn from isort already, then now would be the time to bite the bullet and do it (I much prefer absolute import).
-
The next couple commits turn off
only_sectionsand activate sorting within sections. I feel this is not much extra value on top of the previous changes, but I don't feel strongly. -
The next couple commits add in some options from the google style guide, which I think would be a mistake for our codebase. Those options make sense if you are following the full google style guide:
from sound.effects import echo ... echo.EchoFilter(input, output, delay=0.7, atten=4)
vs
from sound.effects.echo import EchoFilter ... EchoFilter(input, output, delay=0.7, atten=4)
I actually rather like the google recommendation here, but we don't generally follow it in qiskit, and without it the google isort settings make the import sections way too verbose. I don't think the effort to switch over to the google style of importing modules only would come close to being worthwhile (unless maybe there were a tool that could do it in the manner of black, but I haven't seen one, and even then it's probably too disruptive).
My current thoughts would be that if we make a change like this, then to do all of steps 1-3, probably don't do 4, definitely don't do 5.
I think with 1-3 only then its low-impact on contributors to add CI checking of the isort usage, since its easy to get the sectioning correct by hand.
If we add 4 then I tend to agree with @jakelishman that it's adding the barrier for contributors and for not-much benefit. I'm skeptical about the claimed merge-conflict resolution.
As a compromise we could turn off only_sections for the purposes of generating the isort-ed code to check in for this PR, since there will be a ton of churn anyway with this PR. But then leave only_sections=True in the config-file for the future. That way it will presumably take a while for the codebase to drift away from that sorting and if contributors happen to have tooling setup that sorts within sections they won't be adding additional churn.
