Add namespace redirect hook for qiskit-aer #5089

mtreinish · 2020-09-18T21:48:22Z

Summary

Namespace packages are constant source of problems for users. The python
packaging ecosystem around splitting packages across namespaces is
fragile at the best of times and can often leave a you with an
environment that isn't recoverable (especially when mixing install
methods). There is also a performance hit whenever there is a piece of
the namespace we allow external packages to extend since it requires
doing a full python path search which can be slow depending on the
backing I/O and the number of paths in sys.path for an environment.

For most of the former namespace packages we've migrated away from using
them as part of renaming or deprecation. For example, aqua was retired and
split up into 4 different packages, ignis was replaced by qiskit-experiments, etc.
The only former element that this doesn't apply to is qiskit-aer which is keeping
it's name. But in an effort to clean up the namespace we'd like to move qiskit-aer
to a dedicated package namespace qiskit_aer to decouple it from qiskit-terra.
This commit adds a custom meta path finder that hooks into the import path in
Python to detect when someone tries to import qiskit.providers.aer (the former/current
namespace of qiskit-aer) and tries to redirect that as an import to qiskit_aer instead.
If that works then qiskit_aer will silently be aliased for the root of the import, if it doesn't
succeed then the normal import process will continue.

To support this migration we will have to keep the arbitrary namespace hooks using
pkgutil to ensure that different methods of installing the qiskit packages enable the
namespace package discovery. We'll have to keep these around until at some point
if we decide we want to drop support for using older qiskit-aer installations.

Details and comments

Namespace packages are constant source of problems for users. The python packaging ecosystem around splitting packages across namespaces is fragile at the best of times and can often leave a you with an environment that isn't recoverable (especially when mixing install methods). There is also a performance hit whenever there is a piece of the namespace we allow external packages to extend since it requires doing a full python path search which can be slow depending on the backing I/O and the number of paths in sys.path for an environment. This commit starts the process of addressing this by removing the arbitrary namespace hook points and hard coding the element namespace maps via a custom import loader at the root of the namespace. This has 2 advantages it removes the use of namespace packages so the fragility and performance impact are fixed since every element will be renamed to use 'qiskit_' instead of 'qiskit.', but it also makes it explicit where we extend the namespace. The previous method allowed any package to extend qiskit.* and qiskit.providers.* with whatever they wanted. We'll need to coordinate updating the elements with this merging, because it is a breaking change for each element (although not for end users). A potential follow on is to add a plugin interface for 3rd party providers like what was proposed in Qiskit#1465 so that we can make external providers externally discoverable without needing to add manual hook points moving forward (this was done for backwards compat with the aqt and honeywell provider). This is a second attempt at removing namespace packaging. The first attempt in PR Qiskit#4767 was merged and had to be reverted because there were some circular import error issues that needed to be resolved. Since having this in terra blocks CI for all the qiskit elements a revert was necessary to unblock developement for the entire project while those were resolved.

This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to be lazy loading instances of lazy loading wrapper classes. The intent here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from qiskit-terra, while this is safe while the packages share a shared namespace we've been actively working to remove the use of namespace packaging (see Qiskit#5089, Qiskit#4767, and Qiskit#559) and the circular dependency caused by re-exporting these attributes is blocking progress on this. By using a lazy loading wrapper class we avoid an import type circular dependency and opportunistically use qiskit-aer and/or qiskit-ibmq-provider at runtime only if they're present after everything is imported. This also may have some benefit for the overall import performance of qiskit (being tracked in Qiskit#5100) as it removes qiskit-aer and qiskit-ibmq-provider from the import path unless they're being used. Although the presence of qiskit.__qiskit_version__ might prevent any performance improvements as it still imports all the elements to get version information (and will be tackled in a separate PR). Fixes Qiskit#5532

mtreinish · 2021-01-13T00:18:20Z

This is blocked on #5619 and #5620 which removes the import dependency on other elements

* Wrap qiskit.Aer and qiskit.IBMQ with lazy loading object This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to be lazy loading instances of lazy loading wrapper classes. The intent here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from qiskit-terra, while this is safe while the packages share a shared namespace we've been actively working to remove the use of namespace packaging (see #5089, #4767, and #559) and the circular dependency caused by re-exporting these attributes is blocking progress on this. By using a lazy loading wrapper class we avoid an import type circular dependency and opportunistically use qiskit-aer and/or qiskit-ibmq-provider at runtime only if they're present after everything is imported. This also may have some benefit for the overall import performance of qiskit (being tracked in #5100) as it removes qiskit-aer and qiskit-ibmq-provider from the import path unless they're being used. Although the presence of qiskit.__qiskit_version__ might prevent any performance improvements as it still imports all the elements to get version information (and will be tackled in a separate PR). Fixes #5532 * Fix lint * Fix test lint * DNM: test with ignis patch * Revert "DNM: test with ignis patch" This reverts commit ac9611c. * Use ignis from source for tutorial job * Update release note to be more clear

coveralls · 2022-05-05T14:36:08Z

Pull Request Test Coverage Report for Build 2538971404

34 of 45 (75.56%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.007%) to 84.28%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/init.py	8	9	88.89%
qiskit/namespace.py	26	36	72.22%

Totals
Change from base Build 2538969791:	-0.007%
Covered Lines:	55000
Relevant Lines:	65259

💛 - Coveralls

mtreinish · 2022-05-09T21:43:46Z

So this is pointing to an interesting migration/compatibility issue here. To do this migration we need to add the custom metapath loader in terra as it owns the qiskit package and is where the redirect from qiskit.providers.aer -> qiskit_aer needs to happen. But, if we add the redirect it breaks versions of aer still using qiskit.providers.aer. I can add a fallback path to the custom loader class so it tries the old name first and if that fails it tries the name path. In other words something roughly sketched as:

try:
   sys.modules["qiskit,providers.aer"] = import_module("qiskit.providers.aer")
except ImportError:
   sys.modules["qiskit,providers.aer"] = import_module("qiskit_aer")

but the problem with this is that python won't uninstall the old namespace package in all situations (pip should do it by default in normal cases but there are a lot of possible combinations and ways to install) which would cause this to use the wrong version. But I think this is about the best we can do.

jakelishman · 2022-05-11T12:17:44Z

Since qiskit_aer will be the new form, and we want to override old versions with that, perhaps we can fix that awkward loop by doing the test the other way around? For example

try:
    sys.modules["qiskit.providers.aer"] = import_module("qiskit_aer")
except ImportError:
    sys.modules["qiskit.providers.aer"] = import_module("qiskit.providers.aer")

(plus all the other nonsense around dealing with namespace packages).

To ensure that someone with an old version of Aer installed can still access it via the old namespace this changes the meta finder logic to first try qiskit_aer, if it's importable then we build the redirect path first and use that for the legacy qiskit.providers.aer path. If it's not then we just return None and fall back to the other finders in sys.meta_path. To support this the pkgutil hook is added back to qiskit.providers to add the namespace hook for old version of aer using namespace packagingm although not strictly necessary because the implicit support for namespace packages will still likely work we can remove it at a later date.

mtreinish · 2022-05-31T12:57:42Z

I think we can add the deprecation warning in 0.22.0 so there is a release between the new namespace for aer and emitting deprecation warnings (so we comply with the deprecation policy).

Namespace packages are constant source of problems for users. The python packaging ecosystem around splitting packages across namespaces is fragile at the best of times and can often leave a you with an environment that isn't recoverable (especially when mixing install methods). There is also a performance hit whenever there is a piece of the namespace we allow external packages to extend since it requires doing a full python path search which can be slow depending on the backing I/O and the number of paths in sys.path for an environment. This commit addresses the piece from the aer perspective by moving qiskit.providers.aer to it's own package and namespace 'qiskit_aer'. This is coupled with a custom namespace module that implements a custom import loader at the root of the namespace. This has 2 advantages it removes the use of namespace packages so the fragility and performance impact are fixed since every element will be renamed to use 'qiskit_' instead of 'qiskit.' (aer being the only one remaining). This is the second attempt at doing this namespace rename, the first failed attempt in Qiskit#840 was trying to migrate all the former qiskit elements at the same time and was too difficult to migrate everything at once without causing breaking api changes. This commit is mechanically the same (renaming qiskit.providers.aer -> qiskit_aer), but how it integrates with qiskit-terra is changed. With Qiskit/qiskit#5089 qiskit-terra, the package which owns the 'qiskit.*' namespace, adds a metaimporter that will redirect imports from qiskit.providers.aer to qiskit_aer. This means before we can merge this commit Qiskit/qiskit#5089 will need to be released first to ensure that for backwards compatibility existing users will be able to access aer from the old namespace. Fixes Qiskit/qiskit#559

jakelishman

This looks both very clever and very neat! My questions are pretty much only about the public interface we're presenting here.

qiskit/__init__.py

jakelishman · 2022-06-21T14:23:55Z

qiskit/namespace.py

+            except Exception:  # pylint: disable=broad-except
+                return None


Do we need to catch Exception here? Can we reduce it to ImportError? I don't really know - I'm thinking of cases where there's import code error in a development version of Aer, which don't get caught because the non-import error gets masked into "package not available".

I made this a broad exception since I didn't want a development error to break the machinery. Like I think if import qiskit_aer raises an AttributeError because of a bug I'm pretty sure this will just raise an exception and the import will fail with a weird attribute error coming from a random place. But thinking about this I think changing this to an import error is reasonable we can always change it later if needed.

I think that's the reason I was advocating for those exceptions to propagate - I think if we catch all exceptions, a development environment might accidentally pull an old version of Aer from who knows where (namespace packaging problems...) which would mask the error. Either way, we shouldn't ever be publishing a version of Aer that errors out on import, so it's probably just about which way we think is better for development.

To be honest, I think I'd even go as far as limiting it to ModuleNotFoundError - if Aer raises an import error due to one of its compiled extensions failing to load, we'd probably want to propagate that.

Done in 6ba893a

jakelishman · 2022-06-21T14:31:32Z

qiskit/namespace.py

+
+    def find_spec(self, fullname, path=None, target=None):
+        """Return the ModuleSpec for Qiskit element."""
+        if fullname.startswith(self.old_namespace):


I suppose technically this might break some compatibility if some random package is extending the namespace with qiskit.providers.aer_with_my_modifications (still startswith("qiskit.providers.aer"), but we probably don't need to care about that.

I'm not sure namespace packaging would work for someone trying to extend qiskit.providers.aer since aer itself doesn't add the same pkgutil hooks. In some cases someone probably could extend the namespace with their custom subpackage but not in all permutations of packaging as namespace packaging isn't reliable (this is why I couldn't remove the pkgutil calls otherwise I'd break compat with old/current aer releases).

I mean if somebody extends qiskit.providers with a name that starts with aer, like aer_but_actually_not. I think it's probably not worth worrying about, though.

ah ok now I follow what you're saying. Yeah I should have done a proper regex match something like ^qiskit\.provider\.aer\.* to match on exactly the aer namespace. But yeah I'm not too worried a potential name squatter like that. The bigger concern for me is how we deprecate and remove the pkgutil namespace hooks longer term. I know there are external users of it and trying to migrate them off will be a pain.

jakelishman

just nitpicking

qiskit/__init__.py

qiskit/namespace.py

Co-authored-by: Jake Lishman <jake@binhbar.com>

Namespace packages are constant source of problems for users. The python packaging ecosystem around splitting packages across namespaces is fragile at the best of times and can often leave a you with an environment that isn't recoverable (especially when mixing install methods). There is also a performance hit whenever there is a piece of the namespace we allow external packages to extend since it requires doing a full python path search which can be slow depending on the backing I/O and the number of paths in sys.path for an environment. This commit addresses the piece from the aer perspective by moving qiskit.providers.aer to it's own package and namespace 'qiskit_aer'. This is coupled with a custom namespace module that implements a custom import loader at the root of the namespace. This has 2 advantages it removes the use of namespace packages so the fragility and performance impact are fixed since every element will be renamed to use 'qiskit_' instead of 'qiskit.' (aer being the only one remaining). This is the second attempt at doing this namespace rename, the first failed attempt in Qiskit#840 was trying to migrate all the former qiskit elements at the same time and was too difficult to migrate everything at once without causing breaking api changes. This commit is mechanically the same (renaming qiskit.providers.aer -> qiskit_aer), but how it integrates with qiskit-terra is changed. With Qiskit/qiskit#5089 qiskit-terra, the package which owns the 'qiskit.*' namespace, adds a metaimporter that will redirect imports from qiskit.providers.aer to qiskit_aer. This means before we can merge this commit Qiskit/qiskit#5089 will need to be released first to ensure that for backwards compatibility existing users will be able to access aer from the old namespace. Fixes Qiskit/qiskit#559

* Wrap qiskit.Aer and qiskit.IBMQ with lazy loading object This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to be lazy loading instances of lazy loading wrapper classes. The intent here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from qiskit-terra, while this is safe while the packages share a shared namespace we've been actively working to remove the use of namespace packaging (see Qiskit#5089, Qiskit#4767, and Qiskit#559) and the circular dependency caused by re-exporting these attributes is blocking progress on this. By using a lazy loading wrapper class we avoid an import type circular dependency and opportunistically use qiskit-aer and/or qiskit-ibmq-provider at runtime only if they're present after everything is imported. This also may have some benefit for the overall import performance of qiskit (being tracked in Qiskit#5100) as it removes qiskit-aer and qiskit-ibmq-provider from the import path unless they're being used. Although the presence of qiskit.__qiskit_version__ might prevent any performance improvements as it still imports all the elements to get version information (and will be tackled in a separate PR). Fixes Qiskit#5532 * Fix lint * Fix test lint * DNM: test with ignis patch * Revert "DNM: test with ignis patch" This reverts commit ac9611c. * Use ignis from source for tutorial job * Update release note to be more clear

…it#5619) * Wrap qiskit.Aer and qiskit.IBMQ with lazy loading object This commit migrates the qiskit.Aer and qiskit.IBMQ module attributes to be lazy loading instances of lazy loading wrapper classes. The intent here is to avoid importing from qiskit-aer or qiskit-ibmq-provider from qiskit-terra, while this is safe while the packages share a shared namespace we've been actively working to remove the use of namespace packaging (see Qiskit/qiskit#5089, Qiskit/qiskit#4767, and Qiskit/qiskit#559) and the circular dependency caused by re-exporting these attributes is blocking progress on this. By using a lazy loading wrapper class we avoid an import type circular dependency and opportunistically use qiskit-aer and/or qiskit-ibmq-provider at runtime only if they're present after everything is imported. This also may have some benefit for the overall import performance of qiskit (being tracked in Qiskit/qiskit#5100) as it removes qiskit-aer and qiskit-ibmq-provider from the import path unless they're being used. Although the presence of qiskit.__qiskit_version__ might prevent any performance improvements as it still imports all the elements to get version information (and will be tackled in a separate PR). Fixes Qiskit/qiskit#5532 * Fix lint * Fix test lint * DNM: test with ignis patch * Revert "DNM: test with ignis patch" This reverts commit ac9611c3ace30d101a70bda06e1987df14662182. * Use ignis from source for tutorial job * Update release note to be more clear

mtreinish requested review from jyu00 and a team as code owners September 18, 2020 21:48

mtreinish added the on hold Can not fix yet label Sep 18, 2020

mtreinish added 3 commits October 8, 2020 10:59

Merge branch 'master' into namespace-no-more

3174149

Try using new aer path for qiskit.Aer alias

990864b

Try moving Aer and IBMQ alias to the end

6e896c5

mtreinish mentioned this pull request Dec 15, 2020

Avoid potential import cycles from Aer and IBMQ #5532

Closed

mtreinish mentioned this pull request Jan 12, 2021

Wrap qiskit.Aer and qiskit.IBMQ with lazy loading object #5619

Merged

Merge branch 'master' into namespace-no-more

0f985de

mtreinish requested a review from chriseclectic as a code owner February 5, 2021 20:33

mtreinish added 6 commits February 9, 2021 10:21

Fix typo

d2a93b9

Merge branch 'master' into namespace-no-more

32b5499

Merge remote-tracking branch 'origin/master' into namespace-no-more

c17e0c1

Fix circular import issue

fa1781e

Fix typos

9483708

Merge branch 'main' into namespace-no-more

208131e

mtreinish added 2 commits May 9, 2022 14:18

Run black

5e493fc

Remove file encoding

e3866ea

mtreinish added 3 commits May 11, 2022 09:29

Adjust init with import re-direct

f759582

Merge remote-tracking branch 'origin/main' into namespace-no-more

4836642

mtreinish mentioned this pull request May 31, 2022

Move Aer to its own package Qiskit/qiskit-aer#1526

Merged

mtreinish added this to the 0.21 milestone Jun 3, 2022

mtreinish added the priority: high label Jun 3, 2022

jakelishman reviewed Jun 21, 2022

View reviewed changes

jakelishman self-assigned this Jun 21, 2022

mtreinish added 3 commits June 21, 2022 13:11

Merge remote-tracking branch 'origin/main' into namespace-no-more

8eb7ce7

Make qiskit.Aer a pending deprecation instead of a deprecation

5e43989

Restrict namespace redirect error to ModuleNotFoundError

6ba893a

mtreinish requested a review from jakelishman June 21, 2022 19:07

jakelishman reviewed Jun 21, 2022

View reviewed changes

qiskit/__init__.py Outdated Show resolved Hide resolved

qiskit/__init__.py Outdated Show resolved Hide resolved

qiskit/namespace.py Outdated Show resolved Hide resolved

mtreinish and others added 2 commits June 21, 2022 15:22

Apply suggestions from code review

20c63c8

Co-authored-by: Jake Lishman <jake@binhbar.com>

Merge branch 'main' into namespace-no-more

132c894

jakelishman approved these changes Jun 21, 2022

View reviewed changes

jakelishman added the automerge label Jun 21, 2022

Merge branch 'main' into namespace-no-more

9e11213

mergify bot merged commit 8dc0cfc into Qiskit:main Jun 22, 2022

mtreinish deleted the namespace-no-more branch June 23, 2022 19:25

This was referenced Jun 24, 2022

Fix FakeToronto import and Fix Aer Deprecate Messages qiskit-community/qiskit-machine-learning#424

Merged

Fix Aer Deprecate Messages qiskit-community/qiskit-optimization#385

Merged

Fix Aer Deprecate Messages qiskit-community/qiskit-finance#181

Merged

mtreinish mentioned this pull request Jul 13, 2022

Proposal Plan To Rename Library Qiskit/rustworkx#641

Closed

wshanks mentioned this pull request Sep 16, 2022

Update test imports for qiskit-aer 0.11 Qiskit-Extensions/qiskit-experiments#913

Merged

IvanIsCoding mentioned this pull request Nov 4, 2022

🐛 Fix deprecated Loader.load_module() Qiskit/rustworkx#728

Merged

burgholzer mentioned this pull request Nov 5, 2022

🐛 fix deprecated Loader.load_module() #9078

Merged

jakelishman mentioned this pull request Dec 20, 2023

Deprecate qiskit.Aer and qiskit.providers.aer #11439

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add namespace redirect hook for qiskit-aer #5089

Add namespace redirect hook for qiskit-aer #5089

mtreinish commented Sep 18, 2020 •

edited

Loading

mtreinish commented Jan 13, 2021

coveralls commented May 5, 2022 •

edited

Loading

mtreinish commented May 9, 2022

jakelishman commented May 11, 2022

mtreinish commented May 31, 2022

jakelishman left a comment

jakelishman Jun 21, 2022

mtreinish Jun 21, 2022

jakelishman Jun 21, 2022

mtreinish Jun 21, 2022

jakelishman Jun 21, 2022

mtreinish Jun 21, 2022

jakelishman Jun 21, 2022

mtreinish Jun 21, 2022

jakelishman left a comment

Add namespace redirect hook for qiskit-aer #5089

Add namespace redirect hook for qiskit-aer #5089

Conversation

mtreinish commented Sep 18, 2020 • edited Loading

Summary

Details and comments

mtreinish commented Jan 13, 2021

coveralls commented May 5, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2538971404

💛 - Coveralls

mtreinish commented May 9, 2022

jakelishman commented May 11, 2022

mtreinish commented May 31, 2022

jakelishman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

mtreinish commented Sep 18, 2020 •

edited

Loading

coveralls commented May 5, 2022 •

edited

Loading