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

allow string imports outside of packages #736

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

carloshorn
Copy link

Problem description

Currently, string imports are not possible outside of packages.

Minimal reproducible example

The following code snippet executed from a script or Jupyter notebook cell raises an AttributeError: 'NoneType' object has no attribute '__package__'

from dependency_injector import providers
deque_factory = providers.Factory('collections.deque')

The trace back reveals that the exception is thrown in src/dependency_injector/providers.pyx in dependency_injector.providers._resolve_calling_package_name().

Proposal for solution

In case the module in dependency_injector.providers._resolve_calling_package_name is None, the package should also be None. The subsequent call to importlib.import_module will handle the situation properly.

@nightblure
Copy link

IMO this will not be a popular use case, using strings instead of objects is a rather exotic and rare practice..

@@ -5034,7 +5034,7 @@ def _resolve_calling_module():

def _resolve_calling_package_name():
module = _resolve_calling_module()
return module.__package__
return getattr(module, "__package__", None)
Copy link

@nightblure nightblure Nov 13, 2024

Choose a reason for hiding this comment

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

It seems like it would be a good idea to write tests or add test cases for clients using this _resolve_calling_package_name function

nightblure

This comment was marked as duplicate.

allow string imports outside of packages
@carloshorn
Copy link
Author

IMO this will not be a popular use case, using strings instead of objects is a rather exotic and rare practice..

At that time, I was writing an image processing application, where the user could provide a list of filters, e.g. something like a Gaussian filter with given sigma and window size, within a config file. However, I got the feature request if I could also allow filters from other libraries, e.g. scipy, skimage and openCV... or a custom filter...
So I thought, let the user select whatever they want by providing the import string and the configuration inside the config file. Then I saw that the factory provider can handle string imports, https://python-dependency-injector.ets-labs.org/providers/factory.html#string-imports. So I thought "No problem!"... However, my first attempt resulted in the above error.

Generally, I agree that this is an exotic and rare practice. Looking at it today reviewing my own PR, I would even argue that providing a string import to another library is unsafe, so it would require either a safe alternative like the safe_load in yaml, or an explicitly unsafe factory provider that allows this...

IMHO, you may abandon the string imports from the factory provider and add an explicit string import provider or delegate the import to the users providing some examples in the documentation. This may shrink the interface to a single way of doing things. However, I can just imagine the difficulties of maintaining such a popular library and dealing with breaking changes. BTW, thanks for providing and maintaining this library!

I could implement the string import provider that allows unsafe imports if you agree.

What do you think?

@nightblure
Copy link

IMO this will not be a popular use case, using strings instead of objects is a rather exotic and rare practice..

At that time, I was writing an image processing application, where the user could provide a list of filters, e.g. something like a Gaussian filter with given sigma and window size, within a config file. However, I got the feature request if I could also allow filters from other libraries, e.g. scipy, skimage and openCV... or a custom filter... So I thought, let the user select whatever they want by providing the import string and the configuration inside the config file. Then I saw that the factory provider can handle string imports, https://python-dependency-injector.ets-labs.org/providers/factory.html#string-imports. So I thought "No problem!"... However, my first attempt resulted in the above error.

Generally, I agree that this is an exotic and rare practice. Looking at it today reviewing my own PR, I would even argue that providing a string import to another library is unsafe, so it would require either a safe alternative like the safe_load in yaml, or an explicitly unsafe factory provider that allows this...

IMHO, you may abandon the string imports from the factory provider and add an explicit string import provider or delegate the import to the users providing some examples in the documentation. This may shrink the interface to a single way of doing things. However, I can just imagine the difficulties of maintaining such a popular library and dealing with breaking changes. BTW, thanks for providing and maintaining this library!

I could implement the string import provider that allows unsafe imports if you agree.

What do you think?

I'm just a user of this package, but I made my own analogue of the injector and understand how much it works inside. User experience is important, and I think that the provider should also be specified explicitly, and not as a string. And explicit is better than implicit!

but if we refer to specific arguments, I have two arguments in favor of not working with strings:

  1. less maintainable code (the best code is the one that is not written, especially in this specific case);
  2. imagine that you want to use more than one DI container. And in this case you will not get by with a string, because you will need to specify a specific container and its provider:
class Container1(DeclarativeContainer): ...
class Container2(DeclarativeContainer): ...

@inject
def do_smth(dep = Provide["provider"]): ...
# string definitely won’t help here, because there is more than one container

@carloshorn
Copy link
Author

[...] I think that the provider should also be specified explicitly, and not as a string. And [...]

but if we refer to specific arguments, I have two arguments in favor of not working with strings:

  1. less maintainable code (the best code is the one that is not written, especially in this specific case);
  2. imagine that you want to use more than one DI container. And in this case you will not get by with a string, because you will need to specify a specific container and its provider:
class Container1(DeclarativeContainer): ...
class Container2(DeclarativeContainer): ...

@inject
def do_smth(dep = Provide["provider"]): ...
# string definitely won’t help here, because there is more than one container

Even though they are related, I think we are talking about different string imports. While I am talking about the first argument of providers.Factory, see https://python-dependency-injector.ets-labs.org/providers/factory.html#string-imports, you are talking about string identifiers during wiring, see https://python-dependency-injector.ets-labs.org/wiring.html#string-identifiers.

Please note that I am not generally against string imports, see my use-case above.
One motivation for the string identifiers feature is given in the documentation: "You could also use string identifiers to avoid a dependency on a container:". However, this is out of scope regarding this PR.
The motivation for the string imports in factory providers is not explicitly given, but I can imagine the case where you want to avoid expensive imports that are not used in every instance of your application, e.g. various supported back-ends which live in different sub-modules where you select only one.

An addition for the feature of this PR would be an unsafe attribute for the providers.Factory which would allow the imports from modules outside of the package. The default would be False and it should be possible to set it per instance. I will think further about it and update this PR.

@carloshorn
Copy link
Author

I just realized, that the above error message is specific to working in an interactive mode, e.g. Jupyter notebooks.

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

Successfully merging this pull request may close these issues.

2 participants