scientific-python / lazy-loader

Populate library namespace without incurring immediate import costs
BSD 3-Clause "New" or "Revised" License
118 stars 19 forks source link

ENH: Add require argument to load() to accept version specifiers #48

Closed effigies closed 5 months ago

effigies commented 1 year ago

This PR adds an optional require keyword argument to load() that accepts requirements.txt-style version specifiers.

np = lazy.load("numpy", require="numpy >=1.24")

The effect is to make the import fail if the dependency exists but the version is not satisfied.

Closes #13.


Original text Found a little time tonight to take a first stab at implementing `lazy.load_requirement()`, a variant of `lazy.load()` that accepts a `requirements.txt`-style requirement specifier. This would allow one to write: ```Python nibabel, have_nibabel = lazy.load_requirement('nibabel >= 4.2') yaml, have_yaml = lazy.load_requirement('pyyaml >= 6.0', 'yaml') stats, have_scipy = lazy.load_requirements('scipy >= 1.9', 'scipy.stats') ``` The two-argument form is for when module names do not match distribution names or you want to lazily import a submodule while constraining the base module version. I'll get to tests when I can, but might as well see if this much complexity is within-scope. I doubt it can be done much simpler.
codecov[bot] commented 1 year ago

Codecov Report

Merging #48 (10dffc6) into main (2334bd2) will increase coverage by 1.99%. The diff coverage is 90.76%.

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   90.53%   92.52%   +1.99%     
==========================================
  Files           4        4              
  Lines         169      214      +45     
==========================================
+ Hits          153      198      +45     
  Misses         16       16              
Impacted Files Coverage Δ
lazy_loader/__init__.py 89.42% <88.09%> (+4.05%) :arrow_up:
lazy_loader/tests/test_lazy_loader.py 96.22% <95.65%> (-0.16%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

stefanv commented 1 year ago

I think this could work!

Would we be able to re-use some existing code by making the interface lazy.load(spec, requires=...), instead of defining an entirely new function?

Adding the new function is also fine, although I would swap the order of the arguments scipy.stats, scipy >= 1.9.

effigies commented 1 year ago

Sure, see the latest patch. I don't want to change your return type, so I also added a helper function to test for whether a module will be usable. Not a critical feature as long as the dummy module type can be imported as well.

effigies commented 1 year ago

I was curious about how heavy these imports are, so I moved importing inspect, packaging.requirements and importlib.metadata inside the functions that need them.

Click for diff ```diff diff --git a/lazy_loader/__init__.py b/lazy_loader/__init__.py index 016c26d..f3427c1 100644 --- a/lazy_loader/__init__.py +++ b/lazy_loader/__init__.py @@ -7,16 +7,10 @@ Makes it easy to load subpackages and functions on demand. import ast import importlib import importlib.util -import inspect import os import sys import types -try: - import importlib_metadata -except ImportError: - import importlib.metadata as importlib_metadata - __all__ = ["attach", "load", "attach_stub"] @@ -181,9 +175,14 @@ def load(fullname, *, require=None, error_on_import=False): if not have_module: not_found_message = f"No module named '{fullname}'" elif require is not None: # Old style lazy loading to avoid polluting sys.modules import packaging.requirements + try: + import importlib_metadata + except ImportError: + import importlib.metadata as importlib_metadata + req = packaging.requirements.Requirement(require) try: have_module = req.specifier.contains( @@ -202,6 +201,8 @@ def load(fullname, *, require=None, error_on_import=False): if not have_module: if error_on_import: raise ModuleNotFoundError(not_found_message) + import inspect + try: parent = inspect.stack()[1] frame_data = { ```

Then I profiled loading lazy_loader and each of these requirements in turn.

$ python -X importtime -c "import lazy_loader; import inspect; import packaging.requirements; import importlib.metadata" 2>&1 | awk '/[0-9e] \| [a-z]/' 
import time: self [us] | cumulative | imported package
[...]
import time:       205 |       4984 | lazy_loader
import time:      1067 |       3710 | inspect
import time:       156 |      19275 | packaging.requirements
import time:       860 |      12247 | importlib.metadata

packaging.requirements and importlib.metadata are each 2-4x slower to load than lazy_loader itself, and only needed if this feature is called for. That said, we're talking about a one-time import cost of ~30-35ms. I'll leave it to you to decide if we should import everything at the module level or defer some until needed.

stefanv commented 1 year ago

I think this is the kind of library where that type of optimization makes perfect sense.

effigies commented 11 months ago

Rebased, smoothed down ruff's feathers about how complex load() was getting, and removed the have_module() function as out-of-scope.

This is ready for review, whenever it's convenient.

codecov-commenter commented 11 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (bf82b68) 93.95% compared to head (15a1d1a) 94.00%.

Files Patch % Lines
lazy_loader/__init__.py 92.68% 3 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #48 +/- ## ========================================== + Coverage 93.95% 94.00% +0.05% ========================================== Files 4 4 Lines 182 217 +35 ========================================== + Hits 171 204 +33 - Misses 11 13 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

stefanv commented 5 months ago

Sorry for the long turnaround; I think the implementation looks good. I may want to tweak the docs a bit, but can do that in a follow-up PR. I've also pinged @dschult to see if he has a moment to review.

stefanv commented 5 months ago

Thank you very much @effigies, for your contribution and your patience while we decided how to handle this.