Initial setup of lazy_import functions. by dschult · Pull Request #4909 · networkx/networkx

dschult · 2021-06-17T15:49:27Z

Lazy load of potentially uninstalled libraries.
Also, a function to set up the internal sub-packages in a lazy-load fashion.

This first version will fail tests in environments without numpy because pytest.importorskip is not lazy yet.

Still needs:

way to handle pytest.importorskip
Loader class instead of monkey patch on SourceFileLoader
Way to identify a lazily-loaded-not-yet-used module

Still needs: - way to handle pytest.importorskip - Loader class instead of monkey patch on SourceFileLoader - Way to identify a lazily-loaded-not-yet-used module

dschult · 2021-06-18T12:32:20Z

I believe the test failures here are due to not replacing pytest.importorskip uniformly throughout the library. So some modules are using nx.lazy_importorskip and others aren't. I've tested it locally on utils/tests/test_misc.py and on linalg/tests/test_algebraic_connectivity.py and it works for those.

I've settled on mimicking the importlib.LazyLoader and _LazyModule code as much as possible. Two new subclasses of these enable introspection of the "spec" without triggering the ModuleNotFoundError. That allows a function lazy_importorskip in addition to the two functions from scikit-image (morphed accordingly) lazy_import() and I renamed the other to lazy_package_import().

They should be fairly easy to read now. I'll start workign on figuring out how much of the code has to be changed to enable lazy_importorskip and I'll think a little about whether we can suggest this change to the original pytest.importorskip. Probably no new pushed from me until Sunday. Feedback welcome. :}

stefanv

Nice work, Dan! This looks great already. I just had a few questions (mainly for my own enlightenment).

networkx/lazy_imports.py

stefanv · 2021-06-20T00:11:11Z

networkx/tests/test_lazy_imports.py

+        pass
+
+
+def test_lazy_package_import():


This is nice; would be good to get something like that over to the skimage PR too!

This is nice; would be good to get something like that over to the skimage PR too!

I'm going to try to get something like these tests of attach over in the skimage PR as suggested.

Should I also update the lazy.load function there to allow "delayed failed modules"? It currently raises a ModuleNotFoundError immediately upon lazy.load when a library is not found. That works well for many cases, but not if you want to import optional libraries. Thoughts @stefanv ?

I think that would be a good feature to have, thanks Dan!

networkx/tests/test_lazy_imports.py

networkx/lazy_imports.py

…odules

dschult · 2021-06-22T22:25:20Z

Some of these ideas might be helpful in the PR over at: scikit-image/scikit-image#5101

dschult · 2021-12-26T20:40:36Z

Looks like the differences between the NX lazy-load and scikit lazy-load are:

attach first argument name is module_name rather than package_name
load raises ModuleNotFoundError for uninstalled libraries in both, but this version delays the ImportError until it is used. The skimage version raises upon the calling of load.

What are the next steps?

replace all non-builtin imports with lazy imports. Should they be moved out of the functions to the top of the module? We lose easy search for where they are used, but gain that all import related commands appear at top of module.
load the NetworkX subpackage code in a lazy way. It may be worth going through our prior choices of what gets put in the top level namespace as part of this.

Note: number 1) is done to utils/misc.py in this PR to show what it might look like.

Finally, shall I make these "next steps" in this PR? I'm thinking maybe making another PR that relies on this one. It will touch a lot of code to change the imports.

stefanv

This looks good to me.

As @dschult mentioned, the biggest change from the skimage approach here is that import exceptions are delayed. This means that a user may be able to import a subpackage successfully, but that code will break once it is used. In practice (a) it will allow more code to execute successfully [if your code doesn't use a specific import, it won't fail] (b) raise errors at a different time to what users are currently accustomed to (on function execution rather than module import). I think (b) is not an issue for two reasons: (1) these imports used to reside inside functions before load anyway and (2) the difference will only really be noticeable in interactive work, where raising the exception later makes more sense.

OK, I may have convinced myself to port that change over to skimage as well ;)

networkx/tests/test_lazy_imports.py

dschult · 2022-01-21T22:55:56Z

Thanks @Stefan,
There is another trait of this way to lazy-load. It means that new contributors who are used to using import numpy as np at the top of the module will need to prompted to change that syntax to np = nx.lazy_imports.load("numpy").

But I guess they are currently being prompted to move the import inside their functions. So that's likely to be about the same maintenance burden. The CI will check it either way too.
:}

stefanv · 2022-01-21T22:59:50Z

But I guess they are currently being prompted to move the import inside their functions. So that's likely to be about the same maintenance burden. The CI will check it either way too. :}

True :) I commented on the other PR that slightly nicer syntax may be something like nx.lazy_import(x) or nx.lazy_load(x).

dschult · 2022-01-22T14:30:35Z

I was using the function name "load" because of the version in skimage. But maybe that's not the best name...

Why is the term "load" used? That seems to normally refer to bringing data from a file into a variable. The standard term in Python seems to be "import". Is there a reason to avoid the term "import" here, e.g. nx.lazy_import("numpy")? What was the thought behind using the name load?

stefanv · 2022-01-22T15:21:17Z

We went through various discussions on naming and that is what we settled on, but I don't think there's any particularly good reason for it other to distinguish it from a traditional import. lazy_import should be fine.

* Initial setup of lazy_import functions. Still needs: - way to handle pytest.importorskip --> soln is not to load delayed modules into sys.modules - Loader class instead of monkey patch on SourceFileLoader - Way to identify a lazily-loaded-not-yet-used module --> now create an instance of the Delayed Module class. * fix importorskip with new module class * Remove lazy_importorskip. Don't add Delayed reporting module to sys.modules * make tests work for pypy * refactor to include changes from skimage.lazy * fix test handling of types.ModuleType * Change name from nx.lazy_imports.load to nx.lazy_import * fix tests to use new name. keep `attach` name as is.

dschult added the type: Maintenance label Jun 17, 2021

jarrodmillman mentioned this pull request Jun 17, 2021

Update lazy loading SPEC history scientific-python/specs#106

Merged

dschult added 2 commits June 18, 2021 08:14

Initial setup of lazy_import functions.

d4db9ff

Still needs: - way to handle pytest.importorskip - Loader class instead of monkey patch on SourceFileLoader - Way to identify a lazily-loaded-not-yet-used module

fix importorskip with new module class

7c0bc0c

dschult force-pushed the lazy branch from c1dcfa4 to 7c0bc0c Compare June 18, 2021 12:18

stefanv reviewed Jun 20, 2021

View reviewed changes

dschult added 2 commits June 21, 2021 00:46

Remove lazy_importorskip. Don't add Delayed reporting module to sys.m…

0f58500

…odules

make tests work for pypy

d1a440a

refactor to include changes from skimage.lazy

269780a

rossbar mentioned this pull request Aug 27, 2021

Just importing networkx makes python process commit 4GB of memory #4857

Closed

dschult mentioned this pull request Dec 27, 2021

Update imports to use lazy loading #5242

Closed

fix test handling of type.ModuleType

97972da

stefanv approved these changes Jan 21, 2022

View reviewed changes

networkx/tests/test_lazy_imports.py Show resolved Hide resolved

dschult added 2 commits January 25, 2022 14:17

Change name from nx.lazy_imports.load to nx.lazy_import

2f6052a

fix tests to use new name. keep attah name as is.

83e911e

dschult merged commit 1491775 into networkx:main Jan 25, 2022

dschult added this to the networkx-2.7 milestone Jan 25, 2022

dschult mentioned this pull request Jan 25, 2022

Update imports to use lazy imports #5280

Closed

nstarman mentioned this pull request Oct 10, 2022

Adopt Lazy Loading astropy/astropy#13821

Open

dschult deleted the lazy branch February 21, 2024 20:05

Uh oh!

Conversation

dschult commented Jun 17, 2021

Uh oh!

dschult commented Jun 18, 2021

Uh oh!

stefanv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stefanv Jun 20, 2021

Choose a reason for hiding this comment

Uh oh!

dschult Aug 7, 2021

Choose a reason for hiding this comment

Uh oh!

stefanv Aug 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dschult commented Jun 22, 2021

Uh oh!

dschult commented Dec 26, 2021

Uh oh!

stefanv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dschult commented Jan 21, 2022

Uh oh!

stefanv commented Jan 21, 2022

Uh oh!

dschult commented Jan 22, 2022

Uh oh!

stefanv commented Jan 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants