Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Jan 30, 2024

Conversation

effigies
Copy link
Collaborator

@effigies effigies commented Mar 14, 2023

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:

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
Copy link

codecov bot commented Mar 14, 2023

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%) ⬆️
lazy_loader/tests/test_lazy_loader.py 96.22% <95.65%> (-0.16%) ⬇️

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

@stefanv
Copy link
Member

stefanv commented Mar 14, 2023

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
Copy link
Collaborator Author

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.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this comment mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Initially I'd used load() to lazily load packaging.requirements at the module root, but it seems better not to add it to sys.modules unless calling code actually makes use of packaging.requirements. So "old style lazy loading" just means import at function runtime.

Copy link
Member

@stefanv stefanv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good already!

lazy_loader/__init__.py Outdated Show resolved Hide resolved
@effigies effigies marked this pull request as ready for review March 15, 2023 17:20
@effigies effigies changed the title ENH: Implement load_requirement ENH: Implement require argument to load() to accept version specifiers Mar 22, 2023
@effigies
Copy link
Collaborator Author

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 --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
Copy link
Member

stefanv commented Mar 24, 2023

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

@effigies effigies changed the title ENH: Implement require argument to load() to accept version specifiers ENH: Add require argument to load() to accept version specifiers Mar 25, 2023
@jarrodmillman jarrodmillman added the type: Enhancement New feature or request label Jul 2, 2023
@effigies
Copy link
Collaborator Author

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
Copy link

codecov-commenter commented Aug 17, 2023

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 ⚠️

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stefanv
Copy link
Member

stefanv commented Jan 25, 2024

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.

Copy link
Contributor

@dschult dschult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a reasonable feature and it seems set up well. I don't know the mock unittest construct details but the intent seems good. I put a few comments below as suggestions.

self.__frame_data = frame_data
self.__message = message
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want a default message? Like the old value: f"No module named '{fd['spec']}'\n\n""?
Also, why is message after *args in the function sig instead of before? For backward compat?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want a default message?

I found it was easier to be more explicit about setting the message in the branching logic. I can rework with a default, if preferred.

Also, why is message after *args in the function sig instead of before? For backward compat?

I figured keyword-only would be clearer, but I'm okay with any signature. Let me know if you'd like me to change this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I see what you mean and agree that dealing with the message is better handled in the branching logic.

README.md Outdated Show resolved Hide resolved
effigies and others added 9 commits January 26, 2024 14:35
Using python -X importtime -c "import lazy_loader":

Before
------
import time: self [us] | cumulative | imported package
[...]
import time:       131 |      22995 | lazy_loader

After
-----
import time: self [us] | cumulative | imported package
[...]
import time:       115 |       4248 | lazy_loader
Co-authored-by: Dan Schult <dschult@colgate.edu>
Copy link
Contributor

@dschult dschult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve this PR (subject to the latest changes passing tests once the testing workflows are approved).
Thanks!

@stefanv stefanv merged commit 0d4fb38 into scientific-python:main Jan 30, 2024
26 checks passed
@jarrodmillman jarrodmillman added this to the 0.4 milestone Jan 30, 2024
@stefanv
Copy link
Member

stefanv commented Jan 30, 2024

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

@effigies effigies deleted the enh/load_requirement branch January 30, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optional package features
5 participants