Repository: networkx/networkx
Pull request: Simplify DelayedImportErrorModule
Closes #5367

Problem statement:
Error when importing networkx: "module 'importlib' has no attribute 'machinery'"
When importing networkx, the error `AttributeError: module 'importlib' has no attribute 'machinery'` occurs.

It seems like networkx is not importing `importlib.machinery`.

### Steps to Reproduce
```
$ sudo docker run -it --rm fedora:35
# dnf install -y python3 python3-pip
# pip3 install networkx
# python3 --version
Python 3.10.0rc2
# python3 -c "import importlib; print(dir(importlib))"
['_RELOADING', '__all__', '__builtins__', '__cached__', '__doc__', '__file__', '__import__', '__loader__', '__name__', '__package__', '__path__', '__spec__', '_bootstrap', '_bootstrap_external', '_imp', '_pack_uint32', '_unpack_uint32', 'find_loader', 'import_module', 'invalidate_caches', 'reload', 'sys', 'warnings']
# python3 -c "import networkx"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/lib/python3.10/site-packages/networkx/__init__.py", line 59, in <module>
    from networkx import utils
  File "/usr/local/lib/python3.10/site-packages/networkx/utils/__init__.py", line 1, in <module>
    from networkx.utils.misc import *
  File "/usr/local/lib/python3.10/site-packages/networkx/utils/misc.py", line 23, in <module>
    np = nx.lazy_import("numpy")
  File "/usr/local/lib/python3.10/site-packages/networkx/lazy_imports.py", line 138, in lazy_import
    tmp_loader = importlib.machinery.SourceFileLoader(module, path=None)
AttributeError: module 'importlib' has no attribute 'machinery'
# python3 -c "import importlib; import importlib.machinery; import networkx"
```

### Environment
Python version: Python 3.10.0rc2
NetworkX version: networkx-2.7

Hints from issue discussion:
Very strange - maybe something is wrong with the `python3` package in the fedora image? It seems weird that you're getting 3.10rc2 (without specifying that explicitly).

FWIW I can't reproduce locally and CI never caught anything w/ Debian-based linux, macos, or windows.
I think this is due to which modules are loaded by default, which I guess is a platform-specific setting.

https://bugzilla.redhat.com/show_bug.cgi?id=2007664

Fedora 35:
```
$ python3 -c "import sys; print(sorted(sys.modules))"
['__main__', '_abc', '_codecs', '_collections_abc', '_frozen_importlib', '_frozen_importlib_external', '_imp', '_io', '_signal',
'_sitebuiltins', '_stat', '_thread', '_warnings', '_weakref', 'abc', 'builtins', 'codecs', 'encodings', 'encodings.aliases',
'encodings.utf_8', 'genericpath', 'io', 'marshal', 'os', 'os.path', 'posix', 'posixpath', 'site', 'stat', 'sys', 'time', 'zipimport']
```

Ubuntu 20.04:
```
$ python3 -c "import sys; print(sorted(sys.modules))"
['__main__', '_abc', '_bootlocale', '_codecs', '_collections', '_collections_abc', '_frozen_importlib', '_frozen_importlib_external',
'_functools', '_heapq', '_imp', '_io', '_locale', '_operator', '_signal', '_sitebuiltins', '_stat', '_thread', '_warnings', '_weakref', 'abc',
'apport_python_hook', 'builtins', 'codecs', 'collections', 'contextlib', 'encodings', 'encodings.aliases', 'encodings.latin_1',
'encodings.utf_8', 'functools', 'genericpath', 'heapq', 'importlib', 'importlib._bootstrap', 'importlib._bootstrap_external', 'importlib.abc',
'importlib.machinery', 'importlib.util', 'io', 'itertools', 'keyword', 'marshal', 'mpl_toolkits', 'operator', 'os', 'os.path', 'posix', 'posixpath',
'reprlib', 'site', 'sitecustomize', 'stat', 'sys', 'time', 'types', 'warnings', 'zipimport']
```
AFAIK `importlib.machinery` is part of the standard library and should be expected to be available in a Python installation. It seems like its absence might be an explicit decision in how Python is packaged in Fedora? I'm not sure there's a whole lot NetworkX can do about this, since the `SourceFileLoader` is an important component in the implementation of lazy loading in networkx
It is available, just not always imported by default. I think all that's needed is to add the line `import importlib.machinery` to `lazy_imports.py`. Usually it's safest to explicitly import all used modules rather than relying on them to be imported automatically or as a side-effect from another library.

For example, Python 3.9 on the current dev version of Ubuntu 22.04 doesn't include it by default either.

```
$ python3 -c "import importlib; importlib.machinery"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
AttributeError: module 'importlib' has no attribute 'machinery'
$ python3 -c "import importlib; import importlib.machinery; importlib.machinery"
$ echo $?
0
```
I had assumed that, since this never failed in CI nor in any developer's dev env that this was a problem with the Python packages in other distros... it appears that it's the opposite - the availability of `importlib.machinery` by default doesn't seem to be intentional:

https://github.com/python/cpython/blob/df9f7597559b6256924fcd3a1c3dc24cd5c5edaf/Lib/importlib/__init__.py#L2

I think you're right @stevenengler , we need to add an explicit import of the `machinery` module. Thanks for reporting and the follow-up research!
+1 person here to report on this issue causing problems with my CI.
Commenting so that I get auto-notified when the issue closes.

(btw, thanks for your development and continued maintenance of this great library!)
We plan to release a fix for this in 2.7.1 on Monday, March 7th.  Thanks for the bug report.
FWIW I set up docker and I can't reproduce this in any of the official images mentioned above, i.e. `fedora:35`,  `ubuntu:focal`, nor `ubuntu:jammy`. I'm not 100% convinced that this isn't some sort of python packaging problem in the images that are being used - the only evidence I've found to the contrary is the `__all__` I linked above from the `importlib` module, but that doesn't necessarily tell the whole story.

The "fix" is straightforward of course, but I'm just curious if anyone has any insight as to whether (and if so) why it's necessary?
I can provide the following data points:

[GitHub CI failling](https://github.com/tdene/synth_opt_adders/runs/5384343915?check_suite_focus=true) with nearly the same trace as OP, using [this workflow](https://github.com/tdene/synth_opt_adders/blob/main/.github/workflows/config.yml). The workflow `runs-on: ubuntu-latest` and uses CPython 3.10.2

On my personal machine with Python 3.8.10 and Ubuntu 20.04.4, I do not experience this issue, but I can replicate a very similar case:

```~$ python3
Python 3.8.10 (default, Nov 26 2021, 20:14:08) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import importlib
>>> importlib.machinery
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'importlib' has no attribute 'machinery'
>>> importlib.machinery
<module 'importlib.machinery' from '/usr/lib/python3.8/importlib/machinery.py'>
```

The same behavior persists with other built-in packages, such as urllib.request. Posts from [2015](https://web.archive.org/web/20160819073115/http://www.gossamer-threads.com/lists/python/python/1215580) and [2011](https://stackoverflow.com/questions/4871369/python-error-attributeerror-module-object-has-no-attribute) can be found discussing this.

So it seems related to choices made in `__init__.py`. I just learned this for the first time while writing this reply, but it looks like this is how Python behaves.
Now, why this causes errors in GitHub's CI, but not in local CI or manual use, I cannot guess.

Commit message: Simplify DelayedImportErrorModule

Patch to review:
diff --git a/networkx/lazy_imports.py b/networkx/lazy_imports.py
index 248f3b43c4e..9c4d8befe04 100644
--- a/networkx/lazy_imports.py
+++ b/networkx/lazy_imports.py
@@ -81,6 +81,18 @@ def __dir__():
     return __getattr__, __dir__, list(__all__)
 
 
+class DelayedImportErrorModule(types.ModuleType):
+    def __init__(self, import_name, *args, **kwargs):
+        self.__import_name = import_name
+        super().__init__(*args, **kwargs)
+
+    def __getattr__(self, x):
+        if x.startswith("__"):
+            super().__getattr__(x)
+        else:
+            raise ModuleNotFoundError(f"No module named '{self.__import_name}'")
+
+
 def lazy_import(fullname):
     """Return a lazily imported proxy for a module or library.
 
@@ -132,14 +144,7 @@ def myfunc():
     spec = importlib.util.find_spec(fullname)
 
     if spec is None:
-        # module not found - construct a DelayedImportErrorModule
-        spec = importlib.util.spec_from_loader(fullname, loader=None)
-        module = importlib.util.module_from_spec(spec)
-        tmp_loader = importlib.machinery.SourceFileLoader(module, path=None)
-        loader = DelayedImportErrorLoader(tmp_loader)
-        loader.exec_module(module)
-        # dont add to sys.modules. The module wasn't found.
-        return module
+        return DelayedImportErrorModule(fullname, "DelayedImportErrorModule")
 
     module = importlib.util.module_from_spec(spec)
     sys.modules[fullname] = module
@@ -148,24 +153,3 @@ def myfunc():
     loader.exec_module(module)
 
     return module
-
-
-class DelayedImportErrorLoader(importlib.util.LazyLoader):
-    def exec_module(self, module):
-        super().exec_module(module)
-        module.__class__ = DelayedImportErrorModule
-
-
-class DelayedImportErrorModule(types.ModuleType):
-    def __getattribute__(self, attr):
-        """Trigger a ModuleNotFoundError upon attribute access"""
-        spec = super().__getattribute__("__spec__")
-        # allows isinstance and type functions to work without raising error
-        if attr in ["__class__"]:
-            return super().__getattribute__("__class__")
-
-        raise ModuleNotFoundError(
-            f"Delayed Report: module named '{spec.name}' not found.\n"
-            "Reporting was Lazy -- delayed until module attributes accessed.\n"
-            f"Most likely, {spec.name} is not installed"
-        )


Code review comments:
diff_hunk: @@ -81,6 +81,18 @@ def __dir__():
     return __getattr__, __dir__, list(__all__)
 
 
+class DelayedImportErrorModule(types.ModuleType):
+    def __init__(self, import_name, *args, **kwargs):
+        self.__import_name = import_name
+        super().__init__(*args, **kwargs)
+
+    def __getattr__(self, x):
+        if x.startswith("__"):
+            super().__getattr__(x)
+        else:
+            raise ModuleNotFoundError(f"No module named '{self.__import_name}'")
path: networkx/lazy_imports.py
text: @user1:
This makes the raised error text the same as for regular imports.
That may cause confusion because the exception is raised potentially much later.
Is there a reason to have a short exception message?
I'd prefer a message that indicates that this is an exception caused by code that executed potentially long ago.

@author:
Yes, this was a change I made on purpose.

Old behavior:

```
def foo(x):
    import scipy.not_installed
    scipy.not_installed(x)
```

This is what we had before lazy loading: it just raises the moment the function is called.  I'm not sure there is a reason to let the user know that we have a deferred import error now: they'll see, just as before, that scipy.x is missing only once the function tries to use it.

@user1:
Actually, before lazy loading it raised on the import statement itself.  Not on the function call.
The traceback nicely explains that the import is what couldn't find the module.

But with lazy loading the traceback doesn't show any import statement
For example, the old behavior:
```
>>> nx.to_numpy_array(G)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/dschult/NXstefanv/networkx/convert_matrix.py", line 1405, in to_numpy_array
    import numpy as np
ModuleNotFoundError: No module named 'numpy'
```
And the new behavior:
```
>>> nx.utils.misc.dict_to_numpy_array({})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/dschult/NXstefanv/networkx/utils/misc.py", line 230, in dict_to_numpy_array
    return dict_to_numpy_array2(d, mapping)
  File "/Users/dschult/NXstefanv/networkx/utils/misc.py", line 248, in dict_to_numpy_array2
    a = np.zeros((n, n))
  File "/Users/dschult/NXstefanv/networkx/lazy_imports.py", line 93, in __getattr__
    raise ModuleNotFoundError(f"No module named '{self.__import_name}'")
ModuleNotFoundError: No module named 'numpy'
```
There is no import statement in sight.    I guess it's not a big difference. But the tracebacks for lazy imports feel confusing enough. 

I'd love something like:
```
>>> nx.utils.misc.dict_to_numpy_array({})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/dschult/NXstefanv/networkx/utils/misc.py", line 230, in dict_to_numpy_array
    return dict_to_numpy_array2(d, mapping)
  File "/Users/dschult/NXstefanv/networkx/utils/misc.py", line 248, in dict_to_numpy_array2
    a = np.zeros((n, n))
  File "/Users/dschult/NXstefanv/networkx/lazy_imports.py", line 93, in __getattr__
    raise ModuleNotFoundError(f"'import {self.__import_name}' failed")
ModuleNotFoundError: 'import numpy' failed lazily
```

@author:
I will modify this to display a more informative error.

@author:
I changed the error message to look like this:

```
This error is lazily reported, having originally occured in
  File /home/stefan/src/networkx/networkx/tests/test_lazy_imports.py, line 11, in test_lazy_import_basics

----> anything_not_real = lazy.lazy_import("anything_not_real")
```

@user1:
Wow -- so you tracked where the lazy_import was called?!?  That's Cool!

I can't see the code though. Did you push?

@author:
🤦

diff_hunk: @@ -81,6 +81,18 @@ def __dir__():
     return __getattr__, __dir__, list(__all__)
 
 
+class DelayedImportErrorModule(types.ModuleType):
+    def __init__(self, import_name, *args, **kwargs):
+        self.__import_name = import_name
+        super().__init__(*args, **kwargs)
+
+    def __getattr__(self, x):
+        if x.startswith("__"):
path: networkx/lazy_imports.py
text: @user1:
This change allows any dunder attribute to be found, whereas the previous code only found `__spec__` and `__class__`.   Any idea of the impact of that change?

@author:
The impact is that, if a user tries to access `scipy.__some_developer_attribute` it will break.  It may be worth limiting this to specific methods, but I wasn't sure which ones are sufficient to, e.g., print the module and use it in the ways you normally would.  If you say the previous code *only* had access to `__class__` and `__spec__` I can restore similar behavior.

@author:
I've now limited this to `__class__`, `__file__`, and `__import_name` (our variable).  That way, you can at least print the module without trouble.

@user1:
Hmmmm...   not sure this works for printing. What should it print?
```
>>> np = nx.lazy_imports.lazy_import('numpy')
>>> print(np)
<module 'DelayedImportErrorModule'>
>>> print(np.__file__)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/dschult/NXstefanv/networkx/lazy_imports.py", line 91, in __getattr__
    super().__getattr__(x)
AttributeError: 'super' object has no attribute '__getattr__'
```

@author:
Yes, the `__file__` attribute is necessary to make `print(np)` work.