better error messaging when deprecated module can't be imported

cirq-core/cirq/__init__.py

@@ -11,7 +11,6 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-from logging import warning
 
 from cirq import _import
 
@@ -590,55 +589,38 @@
     testing,
 )
 
-try:
-    _compat.deprecated_submodule(
-        new_module_name='cirq_google',
-        old_parent=__name__,
-        old_child='google',
-        deadline="v0.14",
-        create_attribute=True,
-    )
-except ImportError as ex:
-    # coverage: ignore
-    warning("Can't import cirq_google: ", exc_info=ex)
-
-try:
-    _compat.deprecated_submodule(
-        new_module_name='cirq_aqt',
-        old_parent=__name__,
-        old_child='aqt',
-        deadline="v0.14",
-        create_attribute=True,
-    )
-except ImportError as ex:
-    # coverage: ignore
-    warning("Can't import cirq_aqt: ", exc_info=ex)
-
-
-try:
-    _compat.deprecated_submodule(
-        new_module_name='cirq_ionq',
-        old_parent=__name__,
-        old_child='ionq',
-        deadline="v0.14",
-        create_attribute=True,
-    )
-except ImportError as ex:
-    # coverage: ignore
-    warning("Can't import cirq_ionq: ", exc_info=ex)
-
-
-try:
-    _compat.deprecated_submodule(
-        new_module_name='cirq_pasqal',
-        old_parent=__name__,
-        old_child='pasqal',
-        deadline="v0.14",
-        create_attribute=True,
-    )
-except ImportError as ex:
-    # coverage: ignore
-    warning("Can't import cirq_pasqal: ", exc_info=ex)
+_compat.deprecated_submodule(
+    new_module_name='cirq_google',
+    old_parent=__name__,
+    old_child='google',
+    deadline="v0.14",
+    create_attribute=True,
+)
+
+_compat.deprecated_submodule(
+    new_module_name='cirq_aqt',
+    old_parent=__name__,
+    old_child='aqt',
+    deadline="v0.14",
+    create_attribute=True,
+)
+
+
+_compat.deprecated_submodule(
+    new_module_name='cirq_ionq',
+    old_parent=__name__,
+    old_child='ionq',
+    deadline="v0.14",
+    create_attribute=True,
+)
+
+_compat.deprecated_submodule(
+    new_module_name='cirq_pasqal',
+    old_parent=__name__,
+    old_child='pasqal',
+    deadline="v0.14",
+    create_attribute=True,
+)
 
 
 def _register_resolver() -> None:

cirq-core/cirq/_compat.py

@@ -457,6 +457,7 @@ def __init__(
         new_module_name: str,
         old_module_name: str,
         deadline: str,
+        broken_module_exception: Optional[BaseException],
     ):
         """An aliasing module finder that uses an existing module finder to find a python
         module spec and intercept the execution of matching modules.
@@ -465,6 +466,7 @@ def __init__(
         self.new_module_name = new_module_name
         self.old_module_name = old_module_name
         self.deadline = deadline
+        self.broken_module_exception = broken_module_exception
         # to cater for metadata path finders
         # https://docs.python.org/3/library/importlib.metadata.html#extending-the-search-algorithm
         if hasattr(finder, "find_distributions"):
@@ -498,6 +500,8 @@ def find_spec(self, fullname: str, path: Any = None, target: Any = None) -> Any:
             # if we are not interested in it, then just pass through to the wrapped finder
             return self.finder.find_spec(fullname, path, target)
 
+        if self.broken_module_exception is not None:
+            raise self.broken_module_exception
         # warn for deprecation
         _deduped_module_warn_or_error(self.old_module_name, self.new_module_name, self.deadline)
 
@@ -544,6 +548,19 @@ def find_spec(self, fullname: str, path: Any = None, target: Any = None) -> Any:
         return spec
 
 
+class _BrokenModule(ModuleType):
+    def __init__(self, name, exc):
+        self.exc = exc
+        super().__init__(name)
+
+    def __getattr__(self, name):
+        raise self.exc
+
+
+class DeprecatedModuleImportError(ImportError):
+    pass
+
+
 def deprecated_submodule(
     *, new_module_name: str, old_parent: str, old_child: str, deadline: str, create_attribute: bool
 ):
@@ -570,24 +587,49 @@ def deprecated_submodule(
     _validate_deadline(deadline)
 
     old_module_name = f"{old_parent}.{old_child}"
+    broken_module_exception = None
 
     if create_attribute:
-        new_module = importlib.import_module(new_module_name)
-        _setup_deprecated_submodule_attribute(
-            new_module_name, old_parent, old_child, deadline, new_module
-        )
+        try:
+            new_module = importlib.import_module(new_module_name)
+            _setup_deprecated_submodule_attribute(
+                new_module_name, old_parent, old_child, deadline, new_module
+            )
+        except ImportError as ex:
+            msg = (
+                f"{new_module_name} cannot be imported. The typical reasons are"
+                f" that\n 1.) {new_module_name} is not installed, or"
+                f"\n 2.) when developing Cirq, you don't have your PYTHONPATH "
+                f"setup. In this case run `source dev_tools/pypath`.\n\n You can "
+                f"check the detailed exception above for more details or run "
+                f"`import {new_module_name} to reproduce the issue."
+            )
+            broken_module_exception = DeprecatedModuleImportError(msg)
+            broken_module_exception.__cause__ = ex
+            _setup_deprecated_submodule_attribute(
+                new_module_name,
+                old_parent,
+                old_child,
+                deadline,
+                _BrokenModule(new_module_name, broken_module_exception),
+            )
 
     def wrap(finder: Any) -> Any:
         if not hasattr(finder, 'find_spec'):
             return finder
-        # this is just to make mypy not complain about the type of new_module_spec being Optional
-        return DeprecatedModuleFinder(finder, new_module_name, old_module_name, deadline)
+        return DeprecatedModuleFinder(
+            finder, new_module_name, old_module_name, deadline, broken_module_exception
+        )
 
     sys.meta_path = [wrap(finder) for finder in sys.meta_path]
 
 
 def _setup_deprecated_submodule_attribute(
-    new_module_name: str, old_parent: str, old_child: str, deadline: str, new_module: ModuleType
+    new_module_name: str,
+    old_parent: str,
+    old_child: str,
+    deadline: str,
+    new_module: Optional[ModuleType],
 ):
     parent_module = sys.modules[old_parent]
     setattr(parent_module, old_child, new_module)

cirq-core/cirq/_compat_test.py

@@ -41,6 +41,7 @@
     deprecated_submodule,
     DeprecatedModuleLoader,
     DeprecatedModuleFinder,
+    DeprecatedModuleImportError,
 )
 
 
@@ -648,6 +649,49 @@ def test_deprecated_module_deadline_validation():
         )
 
 
+def _test_broken_module_1_inner():
+    with pytest.raises(
+        DeprecatedModuleImportError,
+        match="missing_module cannot be imported. " "The typical reasons",
+    ):
+        # pylint: disable=unused-import
+        import cirq.testing._compat_test_data.broken_ref as br  # type: ignore
+
+
+def _test_broken_module_2_inner():
+    with cirq.testing.assert_deprecated(deadline="v0.20", count=None):
+        with pytest.raises(
+            DeprecatedModuleImportError,
+            match="missing_module cannot be imported. The typical reasons",
+        ):
+            # note that this passes
+            from cirq.testing._compat_test_data import broken_ref  # type: ignore
+
+            # but when you try to use it
+            broken_ref.something()
+
+
+def _test_broken_module_3_inner():
+    with cirq.testing.assert_deprecated(deadline="v0.20", count=None):
+        with pytest.raises(
+            DeprecatedModuleImportError,
+            match="missing_module cannot be imported. The typical reasons",
+        ):
+            cirq.testing._compat_test_data.broken_ref.something()
+
+
+def test_deprecated_module_error_handling_1():
+    subprocess_context(_test_broken_module_1_inner())
+
+
+def test_deprecated_module_error_handling_2():
+    subprocess_context(_test_broken_module_2_inner())
+
+
+def test_deprecated_module_error_handling_3():
+    subprocess_context(_test_broken_module_3_inner())
+
+
 def test_new_module_is_top_level():
     subprocess_context(_test_new_module_is_top_level_inner)()
 
@@ -764,7 +808,7 @@ def invalidate_caches(self) -> None:
             nonlocal called
             called = True
 
-    DeprecatedModuleFinder(FakeFinder(), 'new', 'old', 'v0.1').invalidate_caches()
+    DeprecatedModuleFinder(FakeFinder(), 'new', 'old', 'v0.1', None).invalidate_caches()
     assert called
 
 

cirq-core/cirq/testing/_compat_test_data/__init__.py

@@ -87,3 +87,12 @@
     deadline="v0.20",
     create_attribute=False,
 )
+
+# a missing module that is setup as a broken reference
+_compat.deprecated_submodule(
+    new_module_name='missing_module',
+    old_parent=__name__,
+    old_child='broken_ref',
+    deadline='v0.20',
+    create_attribute=True,
+)

cirq-core/cirq/testing/json.py

@@ -84,7 +84,6 @@ def find_classes_that_should_serialize(self) -> Set[Tuple[str, Type]]:
         result: Set[Tuple[str, Type]] = set()
 
         result.update({(name, obj) for name, obj in self._get_all_public_classes()})
-
         result.update(self.get_resolver_cache_types())
 
         return result