python-injector / injector

Python dependency injection framework, inspired by Guice
BSD 3-Clause "New" or "Revised" License
1.27k stars 81 forks source link

My experience switching from haps and a multitude of questions #164

Open JosXa opened 3 years ago

JosXa commented 3 years ago

Hi @jstasiak and the team,

Last week I went on a research spree to get myself updated on what's the latest DI/IoC hotness nowadays, after having made an initial decision to adopt haps as my go-to library two years ago. This is how I stumbled over injector and I might aswell have snorted that documentation through my nose that's how excited I was about it. The way it looks, I'm gonna make the switch and go on using this well-maintained and stable alternative going forward.

So during the past days I started porting some code over and it went fairly smoothly and without many surprises, but still there were a bunch of uncertainties and lack of guidance with regard to what design patterns should be used when it comes to injector.

I would like to bundle those concerns in this issue to make writing this essay easier (πŸ˜„), but would also be happy to split them up into separate issues if you pointed out candidates worth doing so. The goal with this issue is mainly to get some usage questions answered, but also to start a conversation around the state of dependency injection in Python and how injector fits in that ecosystem.

All of the following is in the context of "modern Python" with full typing - I have absolutely no interest in coding in or supporting anything below 3.7.


1. [Usage Question] Is there are way to force validity checks on modules and configure-functions at startup?

During my experiments I was missing something like a binder.bind(..., eager=True) and a way to enforce this on module level also. I thought that one of the advantages of dependency injection should be increased safety from runtime problems, and if I could just invoke all bindings in a module with a single test, I figure that would help.


2. [Oddness] Bindings get silently overridden. Are polymorphic dependencies an intended use case?

Haps does not allow you to configure more than one implementation for a given base type, but for injector that doesn't seem to be a problem. At least on the surface, the following code passes without warning:

class IFoo(ABC): ...
class MyCoolFoo(IFoo): ...
class MyAwesomeFoo(IFoo): ...

def setup_dem_foos(binder: Binder):
    binder.bind(IFoo, to=MyCoolFoo)
    binder.bind(IFoo, to=MyAwesomeFoo)

If I now attempt to get an IFoo out of an injector, from what I can tell the library just silently overrides whatever is already configured with the most recent declaration. Is this intended? I haven't seen the expected behavior documented anywhere, so maybe you can shed some light on whether and how to use this kind of polymorphism.


3. Strict Scopes

From C# .NET Core and Microsoft's builtin dependency injection framework, I am used to scopes having a special meaning (in .NET) and the requirements/scope hierarchy being more tightly controlled when it comes to mixing different scopes with one another.

For example, they will throw an exception at you for using an instance-scoped type as part of a singleton-scoped class, as this would cause the former to lose its integrity of "being instantiated anew on every invocation". This essentially forces you to understand and think about proper scoping and your dependency graph, so it results in cleaner architecture.

From what I can tell, scopes in injector don't bother with any of these shenanigans, but I was curious what your thoughts are about this.


4. [Best Practices] How to work with injector to expose functionality as part of a library?

Most of my Python coding goes towards a variety of libraries that make building and working with bots on Telegram Messenger easier. This question concerns a rather complex framework called Botkit and the ability to modularize both the internals and the public-facing interface, as well as to expose a convenient API for users to split their code into logically-sound modules/components/"reusables".

Using haps, I ran into many problems here due to its global nature and the fact that it's not really meant for hierarchical organization of modules, at least not nearly as nicely as the explicit container declarations that injector offers.

Now, using injector, I ran into a problem related to 2. above. Essentially, I would like to offer different implementations of an abstract IFoo (some kind of service), but already expose one default implementation from a prebuilt module.

What would be a concise way to let users decide they want to use a different implementation, and in which way would you recommend they should pass required dependencies for their particular choice into the framework's setup code?

Here is the concrete example:

Library offers IDataStore and a reference implementation MemoryDictDataStore, but also a RedisDataStore that would require an extra pip-installed redis module; but also a SQLAlchemyDataStoreImpl with its own optional installation. Assuming the dependencies for all three implementations are passed to the library (somehow), how could you afford to the user to make a decision with a nice API?

Using haps, I was able to solve this in a very elegant way using named dependencies, and my approach was to expose something like my_lib.use_data_store_impl: Literal["memory", "redis", "sqlalchemy"] = 'memory' and it would change the implementation at runtime, but neither does injector seem to offer named dependencies, nor would this way of going about it fit with its general style, I think...

Haps implementation:
class MemoryCallbackStore(ICallbackStore): ...
class RedisCallbackStore(ICallbackStore): ...  # with redis client dependency and a try/catch ImportError
class SqlAlchemyCallbackStore(ICallbackStore): ...  # with sqlalchemy dependency and a try/catch ImportError

@egg("redis")  # `egg` in haps is `to=...` in injector
@scope(SINGLETON_SCOPE)
def create_redis_callback_store() -> ICallbackStore:  # a factory function
    try:
        redis = Container().get_object(Redis)
    except Exception as e:
        raise RedisClientUnavailableException(
            "If `redis` is chosen as the qualifier for the botkit callback manager, "
            "you must provide an instantiated `Redis` client to the dependency "
            "injection. Refer to the `callback_store_qualifier` setting documentation."
        ) from e
    redis_cbs = RedisCallbackStore(redis, "callbacks", maxsize=10)
    redis_cbs.remove_outdated(botkit_settings.callbacks_ttl_days)
    return redis_cbm

I will end here for tonight, but there are still 3 more things on my list that I might address depending responsivity and time :)

Thanks in advance!

jstasiak commented 3 years ago

Hey @JosXa, thanks for a detailed writeup!

Thank you for the kind words (I appreciate the documentation can use several improvements, even though you don't necessarily say so yourself :)).

Let me respond point by point:

  1. [Usage Question] Is there are way to force validity checks on modules and configure-functions at startup?

It depends on what specifically do you mean here. Can you show the desired API and how you'd like it to behave?

  1. [Oddness] Bindings get silently overridden. Are polymorphic dependencies an intended use case?

Yes, the last binding is the binding one (pardon the pun). This is kind of by design (to allow having modules that provide some defaults and then modules that overwrite some of those defaults), kind of a result of the implementation (no check has been implemented to prevent this). I appreciate this may create silent and difficult to debug issues and I'm open to accepting a patch that improves things in this regard (after some general design discussion just so everyone's time is used well).

Not sure what do you mean by "polymorphic dependencies", but this behavior has nothing to do with polymorphism in my mind.

  1. Strict Scopes

There's no verification of interactions between various objects' scopes, no formal scope hierarchy exists. This leads to a burdon on the programmer to understand those interactions and carefully craft the object graph (including the scopes) to work correctly. This can (and has) lead to sometimes difficult to debug issues and is definitely an inconvenience. Like in the point 2 above, I don't have specific plans or needs to improve this at the moment but I'm open to accepting a contribution.

  1. [Best Practices] How to work with injector to expose functionality as part of a library?

I'd recommend parameterizing the library module with the implementation type to use (+ a default) like this:

from typing import Type
import injector

class ICallbackStore:
    def callback(self) -> str:
        raise NotImplementedError("Implement me")

class MemoryCallbackStore(ICallbackStore):
    def callback(self) -> str:
        return "memory"

class RedisCallbackStore(ICallbackStore):
    def callback(self) -> str:
        return "redis"

class LibraryModule(injector.Module):
    def __init__(self, store_class: Type[ICallbackStore] = MemoryCallbackStore) -> None:
        self._store_class = store_class

    def configure(self, binder: injector.Binder) -> None:
        binder.bind(ICallbackStore, to=self._store_class)

if __name__ == '__main__':
    injector1 = injector.Injector([LibraryModule])
    injector2 = injector.Injector([LibraryModule(RedisCallbackStore)])
    store1 = injector1.get(ICallbackStore)
    store2 = injector2.get(ICallbackStore)
    print(store1.callback())
    print(store2.callback())

This works and typechecks with default mypy settings:

% python example.py
memory
redis
% mypy example.py 
Success: no issues found in 1 source file
JosXa commented 3 years ago
  1. [Usage Question] Is there are way to force validity checks on modules and configure-functions at startup?

It depends on what specifically do you mean here. Can you show the desired API and how you'd like it to behave?

Sure, so let's just say I have a Module and some bindings get made, but those bindings would not survive an actual injector.get invocation. I suppose this should be doable: If I had a testing function that attempted to instantiate every binding made by that module and raised on or bundled exceptions when errors occur, I'd be able to verify that all bindings actually work, not only the ones that get used (think again in the context of a library). It could also report unused bindings. One could argue that all those items in the container should explicitly covered by tests, especially if this is part of a library, but then again you typically try to circumvent dependency injection and unit test classes in isolation.

  1. [Oddness] Bindings get silently overridden. Are polymorphic dependencies an intended use case?

...

Not sure what do you mean by "polymorphic dependencies", but this behavior has nothing to do with polymorphism in my mind.

Ah, my writing was not very cohesive late at night, sorry. The polymorphism here referred to named dependencies - you bind Foo to Bar and call it "mybar", but also Foo to Baz with the name "mybaz". In my eyes Foo would then be a polymorphic dependency..? Maybe not.

In any case, I am having a huge issue with named dependencies and am actively looking for a nice syntax to introduce them. There doesn't seem a way to do it with string names in type declarations, as they'd get deferred evaluated to concrete types I think (my_named: NamedInject[Foo, "the_name"] ). Only thing that would technically work for type annotations only is what injectable does with its my_named: Autowired(Foo, name="the_name") syntax (see here), but I don't know if there's any precedent to using functions as markers like that. I like injectable's features quite a bit, but that puts me off.

So with injector I went with service locator pattern instead (injecting all_named: Dict[str, Foo] and looking the item up in the constructor), which I'm not a fan of at all.


I'd recommend parameterizing the library module with the implementation type to use

Yes, awesome! That works

JosXa commented 3 years ago

Another thing that seems to be missing from the docs is how injector is going to behave in the context of class hierarchies. If there's a base class that has the @inject decorator, the child class will also have it?

The field descriptor Inject() from haps was really cool in such cases, as the implementing class didn't need to have a gigantic parameter list anymore to satisfy the base class. Anything that you think is unlikely to be overriden from a base class but it still a strong dependency goes into fields like below, but "more dynamic" properties have their place in the constructor. Very versatile this way, and WAY less boilerplate I noticed now that I rewrote a couple of classes from below syntax to only having an initializer...

class Foo:
    my_dep: Bar = Inject()
    my_named_dep: Baz = Inject("a_baz")
JosXa commented 3 years ago

With regard to all these syntax suggestions that are possible with modern Python, I've been dying to pick your brain about that revolutionary new DI system that FastAPI has brought forward, I'm sure you've seen it: https://fastapi.tiangolo.com/tutorial/dependencies/#create-a-dependency-or-dependable

At first glance it seems odd to "abuse" the default parameters for the purpose of function markers, but then you realize that it doesn't hurt because FastAPI/Pydantic are built around this concept that the default value is always the first argument to whatever you're replacing it with:

class Model(BaseModel):
    my_field: List[str] = Field(["a", "b"], min=2)
    second_field: List[int] = Field(..., some=other, args=None)

or

@router.get("/query/list_str/default")
def get_query_type_list_str_default(query: List[str] = Query(['a', 'b'])):

    return f"foo bar {query}"

They can basically tick all the boxes with just these simple concepts and build arbitrarily complex hierarchies of dependencies... For execute-only kind of handlers, the library also offers to declare "dependables" in decorators: https://fastapi.tiangolo.com/tutorial/dependencies/dependencies-in-path-operation-decorators/#add-dependencies-to-the-path-operation-decorator

Do you have any opinion on this as a de-facto expert? :)

jstasiak commented 3 years ago

Sure, so let's just say I have a Module and some bindings get made, but those bindings would not survive an actual injector.get invocation. I suppose this should be doable: If I had a testing function that attempted to instantiate every binding made by that module and raised on or bundled exceptions when errors occur, I'd be able to verify that all bindings actually work, not only the ones that get used (think again in the context of a library). It could also report unused bindings.

It's been requested before (https://github.com/alecthomas/injector/issues/111 and maybe in other places too), there's no way to do this right now. Your Module will likely need other Modules to be able to instantiate things, so testing one module this way, in separation, may be troublesome at best.

Also unclear what "unused bindings" are in this context – they presumably can be used by the users of your library and they stop being unused?

One could argue that all those items in the container should explicitly covered by tests, especially if this is part of a library, but then again you typically try to circumvent dependency injection and unit test classes in isolation.

It depends. You can test your Modules for example – create an Injector and some stub modules + your actual module, see if you can instantiate things you want in a way you want etc.

There's no support for injecting into base classes' constructors, the only constructor Injector will inject into is the one constructor of the class it constructs. See commit dcbd8c42bfab262a83c5c3b73afcf968c0b865d5.

In any case, I am having a huge issue with named dependencies and am actively looking for a nice syntax to introduce them.

The only way to introduce them here is using Annotated as it generates an actual type that both static type checkers recognize and it works at runtime (I'm not saying it's feasible! It's just that other solutions are no-go for me because of lack of static type checkers support). This has been requested in https://github.com/alecthomas/injector/issues/133 and https://github.com/alecthomas/injector/issues/69 (some workarounds are mentioned there).

With regard to all these syntax suggestions that are possible with modern Python, I've been dying to pick your brain about that revolutionary new DI system that FastAPI has brought forward, I'm sure you've seen it: https://fastapi.tiangolo.com/tutorial/dependencies/#create-a-dependency-or-dependable

This seems like an interesting use of default parameters to solve the problem, but this is my only comment after reading this, I haven't actually used it.

JosXa commented 3 years ago

It depends. You can test your Modules for example – create an Injector and some stub modules + your actual module, see if you can instantiate things you want in a way you want etc.

Yes, exactly. Let me back up a bit and provide an example:

# pytest

from ticktick import *  # <-- a lib I'm making

def test_module_can_be_instantiated():
    inj = Injector([TickTickModule(TickTickCredentials("login", "pw"))])
    for t in [  # ⬇️ manually list all the injectables from this module
        TickTickClient,
        Executor,
        CacheSettings,
        TickTickCredentials,
        TickTickDataCache,
        TickTick,
        Queue,
    ]:
        assert inj.get(t)  # πŸ‘ˆ ensures in the future that no regressions have been introduced

The idea would be to have a convenient way of testing that all injectables from a module can indeed be retrieved.

Also unclear what "unused bindings" are in this context – they presumably can be used by the users of your library and they stop being unused?

We're speaking more about design time than actual usage of the library with this I'd say. Unlike @pristupa I have yet to encounter a use case where I need all bindings from a(ll the) module(s) at actual runtime. Usually that should be solvable with multibindings I suppose.

Your Module will likely need other Modules to be able to instantiate things, so testing one module this way, in separation, may be troublesome at best.

Hmm, my intuition strongly disagrees with this. In my eyes a module must be a standalone unit, otherwise it has no reason to exist. As a result, I think if you're introducing cross-dependencies between modules you're breaking the unenforced rule (that exists in my head πŸ˜„) which says that from composition root down to the leaves your object graph must be a strictly directed. Obviously I'm not gonna expect others to also follow this religiously, but at least for me I'd have no troubles if that was a strict constraint. Do you see it differently? In my week using injector now I've always approached modules this way, basically similar to how python modules themselves behave, "do not import from upper or outside scope, only in modules underneath"...

JosXa commented 3 years ago

By the way, please feel in no way obligated to address all of my comment spam @jstasiak - I'm just having a good time with the library's design and wanted to have a nice chat about it, so if you feel I'm stealing your time and it's not fun or interesting to discuss for you then that's alright πŸ˜‰

JosXa commented 3 years ago

Figured out a way to have named dependencies:

T = TypeVar("T")
Named = Annotated

class TestModule(Module):
    def configure(self, binder: Binder) -> None:
        binder.bind(Named[str, "first"], to=InstanceProvider("first_value"))
        binder.bind(Named[str, "second"], to=InstanceProvider("second_value"))
        binder.bind(str, to=InstanceProvider("raw_string"))

inj = Injector(TestModule)
assert inj.get(Named[str, "first"]) == "first_value"
assert inj.get(Named[str, "second"]) == "second_value"
assert inj.get(str) == "raw_string"

Very cool that it worked out of the box πŸ‘

I think that would deserve a PR with at least the Named annotation, a couple of tests, and docs... what do you think?

jstasiak commented 3 years ago

There are design decisions to be made before something implementing named dependencies is merged. The code above works by accident and only in some cases. Try adding this and it'll break:

class X:
    @inject
    def __init__(self, first_str: Named[str, "first"]) -> None:
        self.first_str = first_str

assert inj.get(X).first_str == "first_value"

Since Named is not a thing on its own and just an alias on Annotated this would effectively force everything people put in Annotated metadata to be part of the type identity as far as Injector is concerned which seems undesired (people may want to put some metadata there that's for their own use and Injector shouldn't care). Raw strings would be forced to have Injector-related meaning etc.

Since this is adding a layer of complexity I think we need a set of test cases first, but I don't have time to provide it right now.

r2rstep commented 5 months ago

Having a special class provided by injector should solve the issue:

assert inj.get(Annotated[str, injector.ByName("first")]) == "first_value"

frameworks should ignore all metadata provided in Annotated that they're not able to process so that should be safe