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

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

Open
anuraaga opened this issue Oct 15, 2024 · 4 comments
Open

Comments

@anuraaga
Copy link

anuraaga commented Oct 15, 2024

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

GrahamDumpleton commented Oct 15, 2024

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

anuraaga commented Oct 15, 2024

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

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

anuraaga commented Oct 16, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants