scientific-python / lazy-loader

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

Add context manager functionality using hooks #121

Open vnmabus opened 3 months ago

vnmabus commented 3 months ago

This is an alternative (simpler and safer) implementation of #70 using a meta hook. By using this approach, typing should work without the need of repeating code or using typing stubs (it would allow to remove most of the code in this package, unless there is some problem that I did not notice).

The syntax used to define the lazy imports would be the same as #70:

import lazy_loader as lazy

with lazy.lazy_imports():
    from .some_func import some_func
    from . import some_mod, nested_pkg

This is currently a draft PR because I wanted to receive feedback and do more tests. It should work with any kind of import.

The code should be quite easy to understand and extend if necessary.

One think to take into account: I did NOT implement error_on_import=False with this syntax. I think that it would complicate the implementation and I see no point in delaying to report the ImportError to the user.

@tlambert03, @stefanv what are your thoughts on this?

stefanv commented 3 months ago

Wow, this almost looks too good to be true :eyes: What's the catch!

Is https://github.com/scientific-python/lazy-loader/pull/121/files#diff-0f92aed2087ad50adf43e527efcd07237adacc5031b7296ae1ece2693462986bR320 a recommended practice for taking over imports?

tlambert03 commented 3 months ago

certainly does look elegant! I don't have much experience with the practical details of using importlib.util.LazyLoader, so I can't provide much experience here. The official docs do make it sound like A) it might indeed be a good choice for very large libraries that are trying to avoid up-front greedy import times... but also that B) it's not without caveats. From the docs:

This class only works with loaders that define exec_module() as control over what module type is used for the module is required. For those same reasons, the loader’s create_module() method must return None or a type for which its __class__ attribute can be mutated along with not using slots. Finally, modules which substitute the object placed into sys.modules will not work as there is no way to properly replace the module references throughout the interpreter safely; ValueError is raised if such a substitution is detected.

[!NOTE] Note For projects where startup time is critical, this class allows for potentially minimizing the cost of loading a module if it is never used. For projects where startup time is not essential then use of this class is heavily discouraged due to error messages created during loading being postponed and thus occurring out of context.

Certainly, none of that is a deal-breaker (and indeed, we're already deep down in the magic here simply by trying to support lazy imports in a type safe way), but perhaps those warnings should be passed along in the readme here along with docs on how to use this feature

vnmabus commented 3 months ago

Is https://github.com/scientific-python/lazy-loader/pull/121/files#diff-0f92aed2087ad50adf43e527efcd07237adacc5031b7296ae1ece2693462986bR320 a recommended practice for taking over imports?

So, adding the finder to sys.meta_path seems to be the way. AFAIK, the finders are invoked in order, so it make sense to put ours at the beginning, in order to be called before the others. Of course, nothing prevents other finder to do the same afterwards and run before ours, but then it should have a good reason to do so.

certainly does look elegant! I don't have much experience with the practical details of using importlib.util.LazyLoader, so I can't provide much experience here. The official docs do make it sound like A) it might indeed be a good choice for very large libraries that are trying to avoid up-front greedy import times... but also that B) it's not without caveats.

I didn't either. I just saw it inside the load function in this same package, and my solution was inspired by that. However, I am not proposing here the one and only way to implement this functionality, but just a way in which lazy imports can be implemented in a safe way and with a nice syntax (and one that tricks type-checkers into thinking that they are normal imports). What I want is some discussion around this functionality to see the best way to proceed forward. Should we use LazyLoader or a different approach? For example, it would be possible, albeit a bit more cumbersome, to record each import in the finder, remove the loaded modules after exiting the context manager, and use a mechanism similar to what attach does, if we feel that approach is better.

vyasr commented 2 months ago

@stefanv asked me to take a look at this. Planning to review soon, but messaging in advance so you can ping me if I drop the ball.