Enable unused-import in ruff

[Cryoris]

Summary

Enables unused import checks (F401) in ruff.

Details and comments

This was triggered since we have a bunch of unused imports in the code right now. We still allow unused imports in all __init__.py files (the alternative would be listing all imports explicitly in __all__, but that was decided against).

This has 3 increasing levels of invasiveness:

1. 57ae9ac only removes some unused imports
2. 52f59ea enables F401
3. 0c89b19 replaces using nested internal import paths in favor of using the API-documented entry point (e.g. use qiskit.quantum_info.random_unitary over qiskit.quantum_info.operators.random.random_unitary)

I think it would be nice to have at least steps 1 and 2, but we can also drop step 3 which might make reviewing this PR a bit tough since it touches lots of lines (albeit rather trivially).

[qiskit-bot]

One or more of the following people are relevant to this code:

• @enavarro51
• @Cryoris
• @Qiskit/terra-core
• @ajavadia
• @levbishop
• @mtreinish

[Cryoris]

qiskit_experiments uses one of the undocumented paths I removed, I guess it goes back in...

  File "/home/runner/.local/lib/python3.12/site-packages/qiskit_experiments/library/randomized_benchmarking/standard_rb.py", line 31, in <module>
    from qiskit.quantum_info.random import random_clifford
ModuleNotFoundError: No module named 'qiskit.quantum_info.random'

[jakelishman]

I'd probably just leave qiskit.quantum_info.random as it was - it's not documented, but it's also ancient and given qiskit.circuit.random is documented and necessary, it's not super unreasonable that others were accidentally using it.

We've not needed to touch that file in forever, so it's not like it's hurting us all that much.

[jakelishman]

Can you undo these formatting changes to the TOML file - if we're going to apply a formatter to our TOML files, we should be consistent about it and enforce it in CI.

[Cryoris]

ah crap -- I usually remember to save w/o formatting but here it snuck in 🙂 thanks!

[jakelishman]

I haven't really looked at this in depth, but I will say that in its current form, I'm not wild about the amount of busy-work it's implying to write simple code in non-public modules, or to work with the code organisation that Qiskit has for historical reasons.

Can we look into whether there's better ways to handle __all__ in the presence of nested imports, and see if we can collapse some of it to * imports where appropriate (since having __all__ makes star-imports in the import system much safer).

[jakelishman]

Will Ruff accept a form that's like this:

from .boolean_logic import *
from .n_local import *
# ...

__all__ = [
  *boolean_logic.__all__,
  *n_local.__all__,
  # ...
]

so we can actually semantically specify what the import structure is, rather than making it so a simple addition requires modifications to the import system in dozens of places?

[Cryoris]

Ruff is unhappy in two places about the above:

1. It doesn't accept star-imports generally (F403)
2. It says __all__ must only contain strings (PLE0604)

It's true it would be nice to avoid this redundancy, but in practice this redundancy already exists right now since we have to re-import the objects explicitly from the submodule. One way to avoid this would be to only import them in the __init__.py we actually want users to import the object from and stop having multiple valid locations

[jakelishman]

If PLE0604 is triggering, then Ruff is incorrect - my __all__ does contain only strings.

For F403: the defence Ruff gives of that is partly "it's against PEP8", but an actual reading of PEP8 says (emphasis mine):

Wildcard imports (from <module> import *) should be avoided, as they make it unclear which names are present in the namespace, confusing both readers and many automated tools. There is one defensible use case for a wildcard import, which is to republish an internal interface as part of a public API.

which is consistent with what we're doing here.

My preference would be to find a way to do this - the (imo unnecessarily, but that ship sailed) nested internal module structure of Qiskit means we're going to have to duplicate so many lists that literally are just intended to be re-exports. Can we file-level disable Ruff's F403 on __init__.py files, and work out how we get Ruff to learn that __all__ = [*x.__all__] should be fine too?

[jakelishman]

One way to avoid this would be to only import them in the __init__.py we actually want users to import the object from and stop having multiple valid locations.

I'm ok with this as a second preference, but given how deeply nested some of our modules are, it might turn pretty ugly pretty fast, and it might cause some gross circular dependencies with the current Qiskit structure.

[Cryoris]

The situation is actually better: if we star-import, ruff doesn't complain about the exports not being listed in __all__, so something like

from .boolean_logic import *
from .n_local import *
from .somewhere import SpecificThingWithoutStarImport

__all__ = ["SpecificThingWithoutStarImport"]

works (with an F403 ignore on __init__.py files)

[jakelishman]

Ah haha, but that's a logical problem that Ruff is simply failing to detect now. Those star imports are part of the public API, and they should be listed in the __all__.

Will Ruff complain if you tack

__all__ += boolean_logic.__all__
__all__ += n_local.__all__

onto your example?