GrahamDumpleton / wrapt

A Python module for decorators, wrappers and monkey patching.
BSD 2-Clause "Simplified" License
2.07k stars 231 forks source link

Register import hook instead of directly wrapping when module is string #275

Open anuraaga opened 1 month ago

anuraaga commented 1 month ago

Currently, wrapping via e.g. wrap_function_wrapper import wrapped modules right away

https://github.com/GrahamDumpleton/wrapt/blob/develop/src/wrapt/patches.py#L17

I was wondering if it could make sense to instead register a import hook using register_import_hook. Conceptually,

Before

wrap_function_wrapper(module, ...):
  if isinstance(module, str):
    module = import(module)
  do_stuff_with_module(module, ...)

After

wrap_function_wrapper(module, ...):
  if isinstance(module, str):
    register_import_hook(module, do_stuff_with_module)
    return
  do_stuff_with_module(module, ...)

I understand this would be a big difference in behavior but I wonder if there is any conceivable case where it could affect downstream code. Currently, calls to wrap_ eagerly import the libraries, which means loading libraries to monkey patch which may not actually be imported by the application. Instead using an import hook would allow monkey patching only when a library is actually used which could have some memory benefits, etc. If a user passed a module in directly it would still behave as currently - a string for module seems like a good user intent to want "lazy behavior".

It's not difficult to expect callers to use the import hook themselves so it's not a big deal, just wanted to bring up the idea in case it could make it easier for wrappers without causing regressions in behavior. Just for reference, this would make the behavior match closer e.g. nodejs/import-in-themiddle or Java classloader instrumentation in agents - I doubt that should be a priority for this library but just for context.

GrahamDumpleton commented 1 month ago

Do you mean:

Thus:

@wrapt.when_imported('this')
def hook_this(module):
    # do stuff with module 'this' if is already imported, or only later when it gets imported

Otherwise not really sure what you are asking for.

anuraaga commented 1 month ago

Yeah it's possible to call wrap_ methods within that hook. But I was wondering if it makes sense for the wrap_ functions like wrap_function_wrapper to implicitly register a hook when the passed module is a string, rather than importing and wrapping it inline just to make it more convenient and reactive.

GrahamDumpleton commented 1 month ago

I would have to think about it, it would be mixing what are currently separate bits of functionality which themselves are not dependent on each other. The current behaviour also possibly could not be changed to avoid impacts so perhaps would have to saying something like "this?", ie., use trailing "?" (convention sort of like optional chaining used in languages like Swift) to flag that should not do it immediately if not yet imported and instead setup lazy import hook instead.

anuraaga commented 1 month ago

Thanks for the consideration @GrahamDumpleton! Agree that while I wouldn't expect it commonly, it is hard to be confident if making the change transparently. FWIW, syntax like a question mark to opt-in would make sense to me and add the convenience of going through the import hook.