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

Issue with injecting Generic classes - broke in python 3.7 #175

Open
davebirch opened this issue Jan 19, 2021 · 10 comments
Open

Issue with injecting Generic classes - broke in python 3.7 #175

davebirch opened this issue Jan 19, 2021 · 10 comments

Comments

@davebirch
Copy link

davebirch commented Jan 19, 2021

Hi there, currently upgrading a project using injector to python 3.8. We use MyPy generics in our project and I discovered that I can no longer inject my generic class.

On further investigation, this seems to be still present in master, works in python 3.6.8, then is broken in anything 3.7 and above.

The following unit test reproduces the issue:

GenericType = TypeVar("GenericType")


def test_inject_generic_class() -> None:

    class GenericClass(Generic[GenericType]):
        pass

    class InjectsGeneric:
        @inject
        def __init__(self, injected_generic: GenericClass[str]):
            self.injected_generic = injected_generic

    def bindings(binder: Binder) -> None:
        binder.bind(GenericClass)

    injector = Injector(bindings)
    instance = injector.get(InjectsGeneric)

    assert isinstance(instance, InjectsGeneric)
    assert isinstance(instance.injected_generic, GenericClass)

Output on the failing python versions is:

______________________ test_inject_simple_generic_class _______________________
Traceback (most recent call last):
  File "D:\git\injector\injector\__init__.py", line 658, in get_binding
    return self._get_binding(interface, only_this_binder=is_scope)
  File "D:\git\injector\injector\__init__.py", line 653, in _get_binding
    raise KeyError
KeyError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "D:\git\injector\injector_test.py", line 1494, in test_inject_simple_generic_class
    instance = injector.get(InjectsGeneric)
  File "D:\git\injector\injector\__init__.py", line 963, in get
    result = scope_instance.get(interface, binding.provider).get(self)
  File "D:\git\injector\injector\__init__.py", line 291, in get
    return injector.create_object(self._cls)
  File "D:\git\injector\injector\__init__.py", line 990, in create_object
    self.call_with_injection(cls.__init__, self_=instance, kwargs=additional_kwargs)
  File "D:\git\injector\injector\__init__.py", line 1021, in call_with_injection
    dependencies = self.args_to_inject(
  File "D:\git\injector\injector\__init__.py", line 111, in wrapper
    return function(*args, **kwargs)
  File "D:\git\injector\injector\__init__.py", line 1069, in args_to_inject
    instance = self.get(interface)  # type: Any
  File "D:\git\injector\injector\__init__.py", line 952, in get
    binding, binder = self.binder.get_binding(interface)
  File "D:\git\injector\injector\__init__.py", line 667, in get_binding
    binding = self.create_binding(interface)
  File "D:\git\injector\injector\__init__.py", line 582, in create_binding
    provider = self.provider_for(interface, to)
  File "D:\git\injector\injector\__init__.py", line 644, in provider_for
    raise UnknownProvider('couldn\'t determine provider for %r to %r' % (interface, to))
injector.UnknownProvider: couldn't determine provider for injector_test.test_inject_simple_generic_class.<locals>.GenericClass[str] to None

Could do with getting this one fixed soon-ish, and will be taking a look tomorrow to see if I can create a PR that fixes it. If you have any hints or tips that might help, it would be appreciated :)

@davebirch
Copy link
Author

davebirch commented Jan 20, 2021

As an interesting point of note, if I change the binding to bind to the exact generic type specified in the injectable class GenericClass[str], the same exception is thrown, but on instantiation of the injector instead:

Changed code from unit test:

def bindings(binder: Binder) -> None:
        binder.bind(GenericClass[str])

Test failure and stacktrace:

_____________________________ test_inject_generic _____________________________

    def test_inject_generic() -> None:
        class GenericClass(Generic[GenericType]):
            pass
    
        assert issubclass(type(GenericClass), type)
    
        class InjectsGeneric:
            @inject
            def __init__(self, injected_generic: GenericClass[str]):
                self.injected_generic = injected_generic
    
        def bindings(binder: Binder) -> None:
            binder.bind(GenericClass[str])
    
>       injector = Injector(bindings)

test_jenkins_injector.py:80: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
..\..\..\..\.venv.win\py3.8\lib\site-packages\injector\__init__.py:924: in __init__
    self.binder.install(module)
..\..\..\..\.venv.win\py3.8\lib\site-packages\injector\__init__.py:588: in install
    instance(self)
test_jenkins_injector.py:78: in bindings
    binder.bind(GenericClass[str])
..\..\..\..\.venv.win\py3.8\lib\site-packages\injector\__init__.py:490: in bind
    self._bindings[interface] = self.create_binding(interface, to, scope)
..\..\..\..\.venv.win\py3.8\lib\site-packages\injector\__init__.py:593: in create_binding
    provider = self.provider_for(interface, to)

@jstasiak
Copy link
Collaborator

Hey, I don't think this was ever explicitly supported – if it works on some Python versions it's purely by accident. :) I'm not opposed to supporting this in principle, could you provide an example what would you use it for though? (A real example can affect the design decisions to make here)

@davebirch
Copy link
Author

davebirch commented Jan 20, 2021

Hehe, that makes some sense, yeah, gotta love a happy accident. Still not quite got to the bottom of why it works in 3.6, but need to spend some time figuring out the differences in the typing module between 3.6 and 3.8 as well as remind myself of how the Injector internals work :D

As to our use cases:

One is a simple generic cache class, with generic types for the key and value types of the cached data. (essentially a more complicated dict) - the generic types used here in practice are a mixture of custom objects (URL objects as keys, with JenkinsAPI data objects as the values. Other implementations have str keys instead, but the value types could be pretty much anything.

The other hasn't been written yet, but is likely to at some point soon, which is generic scanner, with generic types for the thing being scanned, and the result class to be returned. TypeVar for the generic result type here will likely be using bounds if that makes any difference?

Is there any more detail you would need to influence design decisions?

I could probably remove the generics usage if entirely necessary, or wrap over the generics with a concrete implementation, but would like to explore the options first that don't involve a load of boilerplate :)

wooyek added a commit to wooyek/injector that referenced this issue Aug 19, 2021
wooyek added a commit to wooyek/injector that referenced this issue Aug 20, 2021
@jstasiak
Copy link
Collaborator

I'd ideally like to see some real world-like pieces of example code that demonstrates the need for this feature, from the description and from the unit tests given so far I'm not entirely convinced this is something that I want to support (but I still don't understand it fully, I admit).

@wooyek
Copy link
Contributor

wooyek commented Aug 31, 2021

We're using parameterized generics to inject dependencies that use generic parameter to configure itself. For eg. here DataclassSerializer will use its generic parametr (Order in the example) to build serialization schema.

@injector.inject
@injector.singleton
class OrderLineProvider:
    _orders_collection: OrdersCollection
    _serializer: DataclassSerializer[Order]

@jstasiak
Copy link
Collaborator

Thank you – what's the DataclassSerializer constructor signature and what's its interface the OrderLineProvider uses? I'm mainly interested in what places does the generic parameter of DataclassSerializer show up and what's being injected into the constructor.

@hf-kklein
Copy link

Hi, I'm late to the party but can give an example on why this could be useful.
We're building software that retrieves data from somewhere and does something with it. And this somewhere, where the data comes from varies. We'd like to use generics for that.

Imagine you have class MyModel, it describes a datastructure you'd like to process.
Now we use injector to decide where the MyModel comes from: Could be a database, could be a file, could be a S3 Bucket, HTTP request... you name it.
The different sources are all ModelProvider[MyModel]s, that have specific methods like:

ModelType = TypeVar("ModelType")
class ModelProvider(Generic[ModelType]):
    def get_data()->list[ModelType]
        """returns a list of models from somewhere"""

There's one implementation of the generic model provider per data source. So there's a DatabaseMyModelProvider, FileMyModelProvider, S3BucketMyModelProvider ...

The signature of the class that processes MyModels looks like this:

def MyModelProcessor:
   """reads data from the model provider and does something with them"""
   
   @inject
   def __init__(my_model_provider: ModelProvider[MyModel]):
        self._my_model_provider = my_model_provider
   
   def do_something():
        models = self._my_model_provider.get_data()
        # do something

Now it'd be super nice to have something like this:

def configure_for_database(binder):
    """used in integration tests and the running application"""
    binder.bind(ModelProvider[MyModel], to=DatabaseMyModelProvider(host, usr, pwd))

def configure_for_database(binder):
    """used in simpler unittests"""
    binder.bind(ModelProvider[MyModel], to=FileMyModelProvider(path_to_a_file_with_testdata))

Being able to easily switch between those different implementations is great and it de-couples the processing logic from the choice of the datasource.

But currently, we cannot use the straight forward code above. But instead, we need a little bit of boilerplate, because we cannot bind ModelProvider[MyModel] directly:

class MyModelProvider(ModelProvider[MyModel], ABC):
    """this class is pure boilerplate"""
    # here's nothing
    pass

...

def configure_for_database(binder):
    """used in integration tests and the running application"""
    binder.bind(MyModelProvider, to=to=DatabaseMyModelProvider(host, usr, pwd))

def configure_for_database(binder):
    """used in simpler unittests"""
    binder.bind(MyModelProvider, to=FileMyModelProvider(path_to_a_file_with_testdata))

...

def MyModelProcessor:
   """reads data from the model provider and does something with them"""
   
   @inject
   def __init__(my_model_provider: MyModelProvider):
        self._my_model_provider = my_model_provider

This artificial class MyModelProvider does nothing. it's only a non-generic class that inherits from the generic class to be used in the binder. And it feels unnecessary.
If we could bind ModelProvider[MyModel] directly, this workaround wouldn't be necessary.

@jstasiak
Copy link
Collaborator

jstasiak commented Sep 5, 2023

Thank you @hf-kklein, I think this is a convincing example.

I'd be open to accept a PR implementing some support for that (there was #188 once upon a time).

@dkmiller
Copy link

Thanks @jstasiak! @wooyek — do you need any help reifying your old PR?

@Nivyaal-zenity
Copy link

why isnt the pr mentioned in #188 was merged? if the issue is reopened?

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

6 participants