Poison import if qiskit-terra detected
#11618
Conversation
|
One or more of the the following people are requested to review this:
|
|
Given that this 1.0-branch poisoning is a bit more likely to trip up developers (the conditions needed for a false positive are much simpler), perhaps it might be worth putting in an existence check for a file that's known to only exist in |
Pull Request Test Coverage Report for Build 7731909408Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
|
The docs failure here is actually the poisoning working correctly; the problem is that |
|
I've made #11621 that is one way of fixing the tox environments, and for now I've cherry-picked it into this branch to see if there's any further problems lurking. edit: this PR now also contains #11625, which is fixes for more bugs in Qiskit 1.0 found by the fix of #11621. I'm just cherry-picking in those commits to check this PR ahead of time; it should merge last in the chain. |
This uses `importlib.metadata` to search for installations of `qiskit-terra` at runtime, and poison the import if one is detected. The two packages are incompatible in a way that we cannot express to `pip` without having poisoned the `qiskit-terra` package as a whole, which is something we're less willing to do in order to keep it easier to install old versions of Qiskit-dependent software, and to keep the Qiskit 1.0 packaging story clean for the coming years. `qiskit>=1.0` and `qiskit-terra<1.0` both attempt to install files in the same directories, so we can't do the detection by looking at the Qiskit version; we'll likely get whichever was installed second if they've both been installed to `site-packages`, or otherwise, whichever comes first on the search path. This uses the distribution component of `importlib.metadata` to detect installed _distributions_ rather than imported files, allowing us to distinguish `qiskit` from `qiskit-terra`.
jakelishman
force-pushed
the
import-error/1.0
branch
from
368b9ee to
cd85aa4
Compare
jakelishman
added
on hold
|
Qiskit/documentation#710 should be merged and live now before any release would actually land. |
| # 'qiskit.tools' is present in all 0.x series of Qiskit and not in Qiskit 1.0+. If a dev has an | ||
| # editable install and switches from 0.x branches to 1.0+ branches, they might have an empty | ||
| # `qiskit/tools` folder in their path, which will appear as a "namespace package" with no valid | ||
| # location. We catch that case as "not actually having Qiskit 0.x" as a convenience to devs. | ||
| _has_tools = getattr(importlib.util.find_spec("qiskit.tools"), "has_location", False) |
There was a problem hiding this comment.
This is a clever trick relying on the 1.0 removals to determine if the terra import is breaking anything or not.
There was a problem hiding this comment.
Thanks!
Will (@wshanks) alluded to a different case we might want to catch / improve the documentation to handle over in Qiskit/documentation#710 (comment) in addition to this. If he's got time to find the repro steps he mentions I can see if there's anything we might want to do in the import mechanism.
I don't want it to get too complex, though - I'd rather have a couple of (minor) false negatives for devs than slow imports or false positives for all users
There was a problem hiding this comment.
I wasn't able to reproduce it by installing 0.25->0.45->main, so I don't think we need to worry about it. Either it takes an intricate set of steps to reproduce or I was just confused about something when I saw it.
There was a problem hiding this comment.
Ok cool, thanks Will. If you do come across another case, let us know - it's possible for us to update this check without much pain (though I am really nervous about how I've been deliberately breaking import!)
There was a problem hiding this comment.
This LGTM, if there is another edge case we need to detect here we can always expand change the check condition down the road. The one I'm thinking about is with editable installs and qiskit<0.43 leaving bits of qiskit.providers in an odd state, but I haven't though throught how we would detect that short of import qiskit not even working.
This will catch the vast majority of the common invalid install cases so lets move forward with this for now.
Summary
This uses
importlib.metadatato search for installations ofqiskit-terraat runtime, and poison the import if one is detected. The two packages are incompatible in a way that we cannot express topipwithout having poisoned theqiskit-terrapackage as a whole, which is something we're less willing to do in order to keep it easier to install old versions of Qiskit-dependent software, and to keep the Qiskit 1.0 packaging story clean for the coming years.qiskit>=1.0andqiskit-terra<1.0both attempt to install files in the same directories, so we can't do the detection by looking at the Qiskit version; we'll likely get whichever was installed second if they've both been installed tosite-packages, or otherwise, whichever comes first on the search path. This uses the distribution component ofimportlib.metadatato detect installed distributions rather than imported files, allowing us to distinguishqiskitfromqiskit-terra.Details and comments
See #11617 for a similar PR made to the 0.45 series (which I propose we release as 0.45.3) and to be ported to the 0.46 branch. The reason I think we also need the 0.45.x and 0.46.x series to have this error is in case they get accidentally installed after Qiskit 1.0 (such as during a subsequent
pip installcommand), and the import system resolvesqiskit.__file__to theqiskit-terra__init__.pyrather than the Qiskit 1.0 init.A similar false-positive to the one I mentioned on #11617 applies here too, but is probably a fair amount more likely if a developer still has a
qiskit_terra.egg-infodirectory in their path, such as in the repo root of Qiskit. The remedy is to delete that directory; it's not part of the actual installation (and tbh, I'm not sure why pip/setuptools drops it there given thatpip uninstalldoesn't clear it up).