Define __all__ for qiskit

[1ucian0]

Many users tend to copy example code from several places in their first attempts to use Qiskit with the following sequence:

from qiskit import *
....
circuit.draw()

If the quantum circuit to draw is not called circuit, the following counter intuitive error pops:

AttributeError: module 'qiskit.circuit' has no attribute 'draw'

Even if the local variable exists, the namespace seems like polluted somehow.

This happens enough to be quite a common question/confusion (here, here, here)

This PR removes circuit from qiskit/__init__.py namespace sets __all__ in qiskit/__init__.py.

[jakelishman]

I'm actually pretty against this. For one, from qiskit import * is bad style for exactly this reason (polluting the namespace) and we shouldn't be teaching people to do it, but for two, circuit is a very import module if you want to access the circuit library (.circuit.library).

If we were to do this, we should actually define __all__ to include only the names we want to import from a star-import, rather than using del.

[mtreinish]

This doesn't do what you think it will and is not a good idea. qiskit.circuit is a subpackage and trying to delete it like this doesn't actually behave consistently and will potentially cause weird issues down the line as the qiskit module getattr will differ from sys modules list.

The issue here is new python developers and we'll need to teach them and learn to avoid * imports (as this is a common thing with that antipattern and why pylint yells at you about it) and name overloading. Everything is new if you've never used python or qiskit before and I get it can be frsutrating, but this is something we have to fix with documentation and learning material. If people are blindly copying and pasting chunks of code without context and hitting errors that's not really something we should try to guard against in the library.

[1ucian0]

If we were to do this, we should actually define __all__ to include only the names we want to import from a star-import, rather than using del.

I think __all__ would be better indeed. I agree user should not do from qiskit import *. But they do it.

Shall I move in the __all__ direction ?

[jakelishman]

Sure, it doesn't hurt to have qiskit define an __all__ property.

Some of this is an A/B problem - sure, from qiskit import * causes problems, but that's because star-imports are just generally a problem. The correct solution is far more along the lines of "don't do that" than "we should try and make it work in all cases", because we can't decide for a user what top-level names they care about not overriding.

The Python docs have this to say about star-imports and __all__:

Package authors may also decide not to support it [__all__], if they don’t see a use for importing * from their package.

Although certain modules are designed to export only names that follow certain patterns when you use import *, it is still considered bad practice in production code. Remember, there is nothing wrong with using from package import specific_submodule! In fact, this is the recommended notation unless the importing module needs to use submodules with the same name from different packages.

[coveralls]

Pull Request Test Coverage Report for Build 1509200825

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.008%) to 82.851%

---

Totals
Change from base Build 1509155777:	0.008%
Covered Lines:	50118
Relevant Lines:	60492

---

💛 - Coveralls

[1ucian0]

is this imported by mistake?

[jakelishman]

We used to check some environment variables here about suppressing packaging warnings - looks like import os got left after those were removed in #5619. Pylint won't moan about unused imports in __init__ by default I think, because most names in an initialiser won't be used.

[1ucian0]

I defined an __all__ with all the objects from before (and that remove the submodules, which is what I try to do here).

Other option is __all__ = [ ] as way to disincentivize the use of star-imports.

I slightly prefer the first one, but no strong opinion.

[jakelishman]

Defining __all__ properly with some actual entries is fine. We still should discourage people from actually star-importing, but we can still define the interface we expect should happen.

On a minor note: technically this is a public API change and subject to the deprecation policy. However, realistically the only way to detect star-imports is super hacky, and the people who might be bitten by stuff like this (hopefully) aren't writing dependent libraries anyway.

changed to all

[1ucian0]

On a minor note: technically this is a public API change and subject to the deprecation policy. However, realistically the only way to detect star-imports is super hacky, and the people who might be bitten by stuff like this (hopefully) aren't writing dependent libraries anyway.

I added an upgrade reno in 8a5097a

[jakelishman]

We used to check some environment variables here about suppressing packaging warnings - looks like import os got left after those were removed in #5619. Pylint won't moan about unused imports in __init__ by default I think, because most names in an initialiser won't be used.

[jakelishman]

Suggested change

"_config",

We probably don't want to be exporting a private name by default, unless there's a really good reason to.

[1ucian0]

I added because _config = _user_config.get_config() and I assumed that intentional. However, checking in the history it seems that was added by #3362 and ended up being leftovers. So I removed all the related lines in 76370cc

[jakelishman]

It's still usable, but at the very least I'd think something like that ought to be through qualified access, so we're fine not to include it in __all__.

[jakelishman]

I think we can merge this PR for 0.19 so I'm keen to do it sooner rather than later to marginally mitigate the insane CI crunch that's doubtless coming for us on Monday. I'm not keen on removing even the private names from qualified access in an unrelated PR this soon before release, though, so let's leave them in, and we can remove them in a separate PR if they're deemed necessary for removal.

[jakelishman]

This trimmed down version that simply defines __all__ to the public classes and functions we actually want to export is fine by me.

While it's out-of-scope for this PR, I think it may be worth considering promoting Clbit and Qubit to the top-level namespace as well, since they're now more core than ClassicalRegister and QuantumRegister. But that's for another time.