Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Poison import if Qiskit 1.0+ is detected #11617

Merged
merged 5 commits into from
Jan 31, 2024

Conversation

jakelishman
Copy link
Member

@jakelishman jakelishman commented Jan 22, 2024

Summary

This uses importlib.metadata to search for installations of Qiskit 1.0 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.

It is not generally expected that a user specifically attempting to install Qiskit 0.45/0.46 will accidentally install Qiskit 1.0 as well. The intention of this poisoning in the "stable" branch is so that the case that a user installs Qiskit 1.0, but either also includes an old package with a dependency purely on qiskit-terra, or runs a subsequent pip install command whose version resolution causes qiskit-terra to become installed. In both these cases, it could be the case that the import qiskit command finds the qiskit-terra __init__.py file, and so any error we also place in Qiskit 1.0 would have been skipped.

Details and comments

I will say that I don't love the idea of poisoning a package by any stretch, but given all the problems we've seen where people have inadvertently been installing broken environments while trying to test Qiskit 1.0, I think this is the pragmatic solution.

The only case of false-positive poisoning I've been able to find is very developer-centric:

  1. a main-branch version of Qiskit 1.0 has been installed in editable mode
  2. Qiskit 1.0 is uninstalled
  3. qiskit-terra 0.45.x (this commit) is installed by any means, and the qiskit metapackage is either not installed, or installed but not editable in a way that overwrites the installations of step 1
  4. the Python session can see the qiskit.egg-info directory that gets dropped by pip install -e . from step 1 before any dist-info or egg-info directory of the old qiskit metapackage in the meta path (such as if the session is launched from the repo root).

If all those criteria are met, it's possible for an incompatible qiskit.egg-info to be in the meta path without the actual Qiskit 1.0 code being in the import path, in which case the package will emit a false positive import error. edit: the only way I can think to get a false negative is if somebody manually deletes stuff out of site-packages without using pip uninstall, and that's not something we need to guard against.

That possibility was enough to scare me, so I threw in a QISKIT_SUPPRESS_1_0_IMPORT_ERROR environment variable, that can be set to 1 to override the error.

See #11618 for the companion PR on main.

This uses `importlib.metadata` to search for installations of Qiskit 1.0
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.

It is not generally expected that a user specifically attempting to
install Qiskit 0.45/0.46 will accidentally install Qiskit 1.0 as well.
The intention of this poisoning in the "stable" branch is so that the
case that a user installs Qiskit 1.0, but either also includes an old
package with a dependency purely on `qiskit-terra`, or runs a subsequent
`pip install` command whose version resolution causes `qiskit-terra` to
become installed.  In both these cases, it could be the case that the
`import qiskit` command finds the `qiskit-terra` `__init__.py` file,
and so any error we also place in Qiskit 1.0 would have been skipped.
@jakelishman jakelishman requested a review from a team as a code owner January 22, 2024 18:28
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@mtreinish
Copy link
Member

How does this relate to: #11230 the plan we were going with before was to warn on import

@jakelishman
Copy link
Member Author

It's largely orthogonal to #11230 (which I'd forgotten about) - this PR fails the import if a qiskit distribution is installed with its major version non-zero, that PR raises a warning if the corresponding qiskit distribution isn't installed with 0.46. Both are valid messages, and I think both should likely exist.

@jakelishman jakelishman added on hold Can not fix yet Changelog: API Change Include in the "Changed" section of the changelog labels Jan 23, 2024
@jakelishman
Copy link
Member Author

I've marked this as "on hold", because we're planning to move ahead with it, but to include a link to a migration guide with more detail in the error message first.

@1ucian0 1ucian0 added this to the 0.45.3 milestone Jan 25, 2024
@jakelishman
Copy link
Member Author

I've added a shortlink to the message which will work soon, but is blocked on Qiskit/documentation#710 being deployed.

@jakelishman
Copy link
Member Author

Qiskit/documentation#710 should be merged and live now before any release would actually land.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me, I agree it's not a great situation but raising a clear error message that the install is corrupted is the lesser evil in this case. I just had a few small inline suggestions on wording then I think this is good to merge

pass
else:
_major, _ = _qiskit_version.split(".", 1)
_suppress_error = os.environ.get("QISKIT_SUPPRESS_1_0_IMPORT_ERROR", False) == "1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this should be documented anywhere? We do have: https://docs.quantum.ibm.com/start/configure-qiskit-local#environment-variables for env variables, but this seems esoteric and dangerous enough that it's probably not worth putting there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's at the very very bottom of https://qisk.it/packaging-1-0, and in some ways I was deliberately trying to obfuscate it because I don't want people to use it. It's only intended as a relief valve if I've made a mistake in this PR.

I thought people who know enough to use it safely are likely to check the source code to see what checks it's doing before raising the error, in which case they'll see the envvar.

qiskit/__init__.py Outdated Show resolved Hide resolved
qiskit/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now, thanks for the fast update. We'll have to bump the version numbers to prepare a 0.45.3 release but at least the release notes are handled with this.

@mtreinish mtreinish enabled auto-merge January 31, 2024 19:09
@mtreinish mtreinish added this pull request to the merge queue Jan 31, 2024
Merged via the queue into Qiskit:stable/0.45 with commit 94ea49d Jan 31, 2024
11 checks passed
@jakelishman jakelishman deleted the import-error/0.45 branch February 1, 2024 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants