Warn on import failures within optional dependencies (Qiskit#11522)

* Warn on import failures within optional dependencies

This distinguishes the case of "failed to find an optional dependency"
from "found the optional dependency, but it failed to import" within
the lazy testers.  Now, a warning containing the import failures will be
shown to users, rather than silently treating the dependency as missing.

This occurred recently when a PR caused an import failure in Aer, which
the CI suite failed to detect because the optional-dependency checkers
allowed the test suite to pass regardless.  Note that this commit alone
won't reliably cause the test suite to fail on a failed import because:

1. this only emits a warning, not an exception (by design).
2. the `unittest` decorators are evaluated during test discovery, which
   happens before we activate our increased warning filters.

This commit also unifies the now two Qiskit-specific warnings (the other
being `QPYLoadingDeprecatedFeatureWarning`) with a common
`QiskitWarning` subclass, much as we do for `QiskitError`.  This gives
us a convenient way to globally deny these errors during CI runs,
without turning _all_ `UserWarning`s into errors, which we're not ready
to do yet.

* Suppress unnecessary lint warnings

* Fix warning when failed import is parent package

qiskit/exceptions.py

@@ -15,7 +15,10 @@
 Top-level exceptions (:mod:`qiskit.exceptions`)
 ===============================================
 
-All Qiskit-related errors raised by Qiskit are subclasses of the base:
+Exceptions
+==========
+
+All Qiskit-related exceptions raised by Qiskit are subclasses of the base:
 
 .. autoexception:: QiskitError
 
@@ -42,6 +45,22 @@
 
 .. autoexception:: QiskitUserConfigError
 .. autoexception:: InvalidFileError
+
+
+Warnings
+========
+
+Some particular features of Qiskit may raise custom warnings.  In general, Qiskit will use built-in
+Python warnings (such as :exc:`DeprecationWarning`) when appropriate, but warnings related to
+Qiskit-specific functionality will be subtypes of :exc:`QiskitWarning`.
+
+.. autoexception:: QiskitWarning
+
+Related to :exc:`MissingOptionalLibraryError`, in some cases an optional dependency might be found,
+but fail to import for some other reason.  In this case, Qiskit will continue as if the dependency
+is not present, but will raise :exc:`OptionalDependencyImportWarning` to let you know about it.
+
+.. autoexception:: OptionalDependencyImportWarning
 """
 
 from typing import Optional
@@ -95,3 +114,13 @@ def __str__(self) -> str:
 
 class InvalidFileError(QiskitError):
     """Raised when the file provided is not valid for the specific task."""
+
+
+class QiskitWarning(UserWarning):
+    """Common subclass of warnings for Qiskit-specific warnings being raised."""
+
+
+class OptionalDependencyImportWarning(QiskitWarning):
+    """Raised when an optional library raises errors during its import."""
+
+    # Not a subclass of `ImportWarning` because those are hidden by default.

qiskit/qpy/exceptions.py

@@ -12,7 +12,7 @@
 
 """Exception for errors raised by the QPY module."""
 
-from qiskit.exceptions import QiskitError
+from qiskit.exceptions import QiskitError, QiskitWarning
 
 
 class QpyError(QiskitError):
@@ -28,6 +28,6 @@ def __str__(self):
         return repr(self.message)
 
 
-class QPYLoadingDeprecatedFeatureWarning(UserWarning):
+class QPYLoadingDeprecatedFeatureWarning(QiskitWarning):
     """Visible deprecation warning for QPY loading functions without
     a stable point in the call stack."""

qiskit/test/base.py

@@ -28,6 +28,7 @@
 import unittest
 from unittest.util import safe_repr
 
+from qiskit.exceptions import QiskitWarning
 from qiskit.tools.parallel import get_platform_parallel_default
 from qiskit.utils import optionals as _optionals
 from qiskit.circuit import QuantumCircuit
@@ -201,6 +202,8 @@ def setUpClass(cls):
             setup_test_logging(cls.log, os.getenv("LOG_LEVEL"), filename)
 
         warnings.filterwarnings("error", category=DeprecationWarning)
+        warnings.filterwarnings("error", category=QiskitWarning)
+
         allow_DeprecationWarning_modules = [
             "test.python.pulse.test_builder",
             "test.python.pulse.test_block",

qiskit/utils/lazy_tester.py

@@ -13,14 +13,16 @@
 """Lazy testers for optional features."""
 
 import abc
+import collections
 import contextlib
 import functools
 import importlib
 import subprocess
 import typing
+import warnings
 from typing import Union, Iterable, Dict, Optional, Callable, Type
 
-from qiskit.exceptions import MissingOptionalLibraryError
+from qiskit.exceptions import MissingOptionalLibraryError, OptionalDependencyImportWarning
 from .classtools import wrap_method
 
 
@@ -284,12 +286,43 @@ def __init__(
         super().__init__(name=name, callback=callback, install=install, msg=msg)
 
     def _is_available(self):
-        try:
-            for module, names in self._modules.items():
+        failed_modules = {}
+        failed_names = collections.defaultdict(list)
+        for module, names in self._modules.items():
+            try:
                 imported = importlib.import_module(module)
-                for name in names:
-                    getattr(imported, name)
-        except (ImportError, AttributeError):
+            except ModuleNotFoundError as exc:
+                failed_parts = exc.name.split(".")
+                target_parts = module.split(".")
+                if failed_parts == target_parts[: len(failed_parts)]:
+                    # If the module that wasn't found is the one we were explicitly searching for
+                    # (or one of its parents), then it's just not installed.
+                    return False
+                # Otherwise, we _did_ find the module, it just didn't import, which is a problem.
+                failed_modules[module] = exc
+                continue
+            except ImportError as exc:
+                failed_modules[module] = exc
+                continue
+            for name in names:
+                try:
+                    _ = getattr(imported, name)
+                except AttributeError:
+                    failed_names[module].append(name)
+        if failed_modules or failed_names:
+            package_description = f"'{self._name}'" if self._name else "optional packages"
+            message = (
+                f"While trying to import {package_description},"
+                " some components were located but raised other errors during import."
+                " You might have an incompatible version installed."
+                " Qiskit will continue as if the optional is not available."
+            )
+            for module, exc in failed_modules.items():
+                message += "".join(f"\n - module '{module}' failed to import with: {exc!r}")
+            for module, names in failed_names.items():
+                attributes = f"attribute '{names[0]}'" if len(names) == 1 else f"attributes {names}"
+                message += "".join(f"\n - '{module}' imported, but {attributes} couldn't be found")
+            warnings.warn(message, category=OptionalDependencyImportWarning)
             return False
         return True
 

releasenotes/notes/lazy-testers-warn-on-import-errors-95a9bdaacc9c3d2b.yaml

@@ -0,0 +1,14 @@
+---
+features:
+  - |
+    A new warning base class, :exc:`.QiskitWarning`, was added.  While Qiskit will continue to use
+    built-in Python warnings (such as :exc:`DeprecationWarning`) when those are most appropriate,
+    for cases that are more specific to Qiskit, the warnings will be subclasses of :exc:`.QiskitWarning`.
+  - |
+    :exc:`.QPYLoadingDeprecatedFeatureWarning` is now a subclass of :exc:`.QiskitWarning`.
+  - |
+    The optional-functionality testers (:mod:`qiskit.utils.optionals`) will now distinguish an
+    optional dependency that was completely not found (a normal situation) with one that was found,
+    but triggered errors during its import.  In the latter case, they will now issue an
+    :exc:`.OptionalDependencyImportWarning` telling you what happened, since it might indicate a
+    failed installation or an incompatible version.

test/python/utils/test_lazy_loaders.py

@@ -12,12 +12,17 @@
 
 """Tests for the lazy loaders."""
 
+from __future__ import annotations
+
+import importlib.abc
+import importlib.util
 import sys
+import warnings
 from unittest import mock
 
 import ddt
 
-from qiskit.exceptions import MissingOptionalLibraryError
+from qiskit.exceptions import MissingOptionalLibraryError, OptionalDependencyImportWarning
 from qiskit.test import QiskitTestCase
 from qiskit.utils import LazyImportTester, LazySubprocessTester
 
@@ -49,6 +54,29 @@ def mock_availability_test(feature):
     return mock.patch.object(type(feature), "_is_available", wraps=feature._is_available)
 
 
+def patch_imports(mapping: dict[str, importlib.abc.Loader]):
+    """Patch the import system so that the given named modules will skip the regular search system
+    and instead be loaded by the given loaders.
+
+    Already imported modules will not be affected; this should use uniquely named modules."""
+
+    class OverrideLoaders(importlib.abc.MetaPathFinder):
+        """A metapath finder that will simply return an explicit loader for specific modules."""
+
+        def __init__(self, mapping: dict[str, importlib.abc.Loader]):
+            self.mapping = mapping
+
+        def find_spec(self, fullname, path, target=None):
+            """Implementation of the abstract (but undefined) method."""
+            del path, target  # ABC parameters we don't need.
+            if (loader := self.mapping.get(fullname)) is not None:
+                return importlib.util.spec_from_loader(fullname, loader)
+            return None
+
+    new_path = [OverrideLoaders(mapping)] + sys.meta_path
+    return mock.patch.object(sys, "meta_path", new_path)
+
+
 @ddt.ddt
 class TestLazyDependencyTester(QiskitTestCase):
     """Tests for the lazy loaders.  Within this class, we parameterise the test cases with
@@ -71,6 +99,23 @@ def test_evaluates_correctly_false(self, test_generator):
         if test_generator():
             self.fail("did not evaluate false")
 
+    def test_submodule_import_detects_false_correctly(self):
+        """Test that a lazy import of a submodule where the parent is not available still generates
+        a silent failure."""
+
+        # The idea here is that the base package is what will fail the import, and the corresponding
+        # `ImportError.name` won't be the same as the full path we were trying to import.  We want
+        # to make sure that the "was it found and failed to import?" handling is correct in this
+        # case.
+        def checker():
+            return LazyImportTester("_qiskit_module_does_not_exist_.submodule")
+
+        # Just in case something else is allowing the warnings, but they should be forbidden by
+        # default.
+        with warnings.catch_warnings(record=True) as log:
+            self.assertFalse(checker())
+        self.assertEqual(log, [])
+
     @ddt.data(available_importer, available_process, unavailable_importer, unavailable_process)
     def test_check_occurs_once(self, test_generator):
         """Check that the test of availability is only performed once."""
@@ -324,6 +369,59 @@ class Module:
                 vars(type(mock_module))[attribute].assert_called()
             vars(type(mock_module))["unaccessed_attribute"].assert_not_called()
 
+    def test_warns_on_import_error(self):
+        """Check that the module raising an `ImportError` other than being not found is warned
+        against."""
+
+        # pylint: disable=missing-class-docstring,missing-function-docstring,abstract-method
+
+        class RaisesImportErrorOnLoad(importlib.abc.Loader):
+            def __init__(self, name):
+                self.name = name
+
+            def create_module(self, spec):
+                raise ImportError("sentinel import failure", name=self.name)
+
+            def exec_module(self, module):
+                pass
+
+        dummy = f"{__name__}_{type(self).__name__}_test_warns_on_import_error".replace(".", "_")
+        tester = LazyImportTester(dummy)
+        with patch_imports({dummy: RaisesImportErrorOnLoad(dummy)}):
+            with self.assertWarnsRegex(
+                OptionalDependencyImportWarning,
+                rf"module '{dummy}' failed to import with: .*sentinel import failure.*",
+            ):
+                self.assertFalse(tester)
+
+    def test_warns_on_internal_not_found_error(self):
+        """Check that the module raising an `ModuleNotFoundError` for some module other than itself
+        (such as a module trying to import parts of Terra that don't exist any more) is caught and
+        warned against, rather than silently caught as an expected `ModuleNotFoundError`."""
+
+        # pylint: disable=missing-class-docstring,missing-function-docstring,abstract-method
+
+        class ImportsBadModule(importlib.abc.Loader):
+            def create_module(self, spec):
+                # Doesn't matter what, we just want to return any module object; we're going to
+                # raise an error during "execution" of the module.
+                return sys
+
+            def exec_module(self, module):
+                del module  # ABC parameter we don't care about.
+                import __qiskit__some_module_that_does_not_exist
+
+        dummy = f"{__name__}_{type(self).__name__}_test_warns_on_internal_not_found_error".replace(
+            ".", "_"
+        )
+        tester = LazyImportTester(dummy)
+        with patch_imports({dummy: ImportsBadModule()}):
+            with self.assertWarnsRegex(
+                OptionalDependencyImportWarning,
+                rf"module '{dummy}' failed to import with: ModuleNotFoundError.*__qiskit__",
+            ):
+                self.assertFalse(tester)
+
     def test_import_allows_attributes_failure(self):
         """Check that the import tester can accept a dictionary mapping module names to attributes,
         and that these are recognised when they are missing."""
@@ -334,7 +432,8 @@ def test_import_allows_attributes_failure(self):
         }
 
         feature = LazyImportTester(name_map)
-        self.assertFalse(feature)
+        with self.assertWarnsRegex(UserWarning, r"'builtins' imported, but attribute"):
+            self.assertFalse(feature)
 
     def test_import_fails_with_no_modules(self):
         """Catch programmer errors with no modules to test."""