hynek / svcs Goto Github PK

A minimal reproduction:

from __future__ import annotations

from svcs import Container, Registry


def get_int() -> int:
    return 1


def get_str(container: Container) -> str:
    return str(container.get(int))


reg = Registry()

reg.register_factory(int, get_int)
reg.register_factory(str, get_str)

container = Container(reg)

assert str(container.get(int)) == container.get(str)

The reason this fails is because inspect.signature does not resolve stringified annotations by default.

This is a minor nit, but I find that app.config is usually filled with things which might be set from an external config.py file, environment variable, or some other source. I often even like to be able to render these things back to e.g. JSON to save the current configuration for debugging.

OTOH, flask provides app.extensions as a specific place for extensions to store state, and many flask extensions do that (e.g. Flask-SQLAlchemy).

What do you think about having svcs store the Registry object (which is managed by svcs, and wouldn't make much sense to set as the end user) in app.extensions?

Originally posted by @alexrudy in #71

A minimal reproduction:

from svcs import Registry
from typing import get_type_hints

get_type_hints(Registry)

The reason for this is the changing of the module of the different types here. This fails since get_type_hints depends on the __module__ of the given object to find the global namespace in order to resolve stringified annotations.

EDIT: Finished writing the description since I hit enter by accident before finishing the writing the issue descritpion.

currently, Container.get starts with the following

if (svc := self._instantiated.get(svc_type)) is not None:
    return svc

meaning that if a factory has been run, and returns None, this value is not considered cached when requesting it the next time and the factory will be run again, overriding any potential closing logic.

I could imagine a user wanting to do something along the lines of:

def int_factory():
    yield None
    ... # some cleanup logic


qol.register(int, int_factory)

# later
result = qol.get(int)
if result is not None:
    ... # some logic
else:
    ... # some other logic

I'm not sure exactly what a use case for this might be, but the current implementation disallows the user to have None be a meaningful value which is generally considered not a good practice for a library. And it's easy to fix by just substituting the first dictionary lookup default value by a sentinel

Currently, cyclic dependencies raise a recursion error. It might be nice to obtain a more user friendly error message

def test_detects_circular_dependency_and_raises(registry, container):
    """
    If a factory causes a circular dependency lookup, a friendly error is raised
    showing the dependency cycle
    """

    def int_factory(svcs_container):
        svcs_container.get(str)
        return 42

    def str_factory(svcs_container):
        svcs_container.get(int)
        return "42"

    registry.register_factory(int, int_factory)
    registry.register_factory(str, str_factory)

    with pytest.raises(Exception) as e:
        container.get(int)

    assert 'cyclic dependency detected:  int -> str -> int'  in str(e.value)

See discussion in #40

As @glyph mentioned to me, having many services can get unnecessarily verbose and even slow.

We should consider:

db, web_api, working_q = svcs.get_many(Database, WebAPI, WorkingQueue)

It's still a for loop, but saves boilerplate and reduces business code from 3 lines to 1.

Better names appreciated.

async factories are supposed to be invoked using await Container.aget. However, currently there's nothing stopping a user from using the normal Container.get method. For example, this works for an async factory:

async def int_factory():
    await asyncio.sleep(0)
    return 42

registry.register_factory(int, int_factory)
await container.get(int) # obtains 42 even though the sync .get method was used

A user that doesn't know about aget might think all is well. But all is not well, since the value stored in the container is not 42 but the coroutine, which has now been awaited.

Solution could be to detect if .get is used for an async factory and raise an error. Luckily there already is the RegisteredService.is_async flag to help out with that

When using the FastAPI integration the server fails to start up:

RuntimeError: The server does not support "state" in the lifespan scope.

This can be reproduced with this minimal reproducer:

import svcs
from fastapi import FastAPI


@svcs.fastapi.lifespan
async def lifespan(app: FastAPI, registry: svcs.Registry):
    yield


app = FastAPI(
    lifespan=lifespan,
)

This is using:

• Python 3.12.2
• FastAPI 0.110.1
• svcs 24.1.0
• Hypercorn 0.16.0

Interestingly I can't reproduce this with Gunicorn or Uvicorn, so I will dig into this some more.

Hey @hynek,

I'm the guy that did a similar thing to svcs (https://github.com/elfjes/gimme-that). At EuroPython you asked for some feedback, so here we go :)

First of all, I like the simplicity of the registry/container design. Registries are for factories, and containers contain instances. Easy.

However, the register_value method seems to break this pattern. Now an instance is present at the registry-level instead of the container level. What if there'd be a Containter.add method that adds an instance (and a Container.remove to remove/forget an instance) (perhaps implement __getitem__, __setitem__ and __delitem__) Seems to me that'd make it more clean. During testing you can then also just create a new container (fixture) for every test and don't need to mess with the registry itself.

I like the ability to close/cleanup instances and the fact that it can do async cleanup. In general, having async support is nice!

Factories currently do not accept parameters. However, they can ask for other services through the 'Quality of Life' module. Would it make sense to you to allow a factory to take an (optional) container argument that can be used to get other services to reduce the
dependency on a global object. This would also be beneficial for dealing with some locality issues, such as when using threadlocal data or ContextVar. You could make it a truly optional argument by inspecting the factory function to check if it requires an argument and only then supply it.

The health check support seems nice, although it seems to me it pollutes both the registry and the container with something that is really specific. However, I currently do not have a better idea on how to implement it. There also seems to be some overlap with the on_registry_close functionality in that it allows for associating a factory with some callback. Maybe there is something there to make it cleaner?

Speaking of health checks. Would it make sense to allow ServicePing.ping to return a value? It would allow for more detailed health checks.

Finally, if you'd be open to it, we could combine our efforts and merge functionality of our two packages into one. I'm currently thinking of the following

• you said you liked the name gimme-that. We can keep that if you want
• I like your approach and simplicity. I do think that I added some unnecessary complexity into my design
• I do like having a package-global registry+container+api. Like a 'Quality of Life' module, but builtin.
• I would also like to have (optional) type hint support for defining dependencies / DI like approach. I'm thinking of an inject-factory function (name pending) that could be used like the following:

registry.register_factory(
    MyService,
    factory=inject(MyService)
)

the inject function would return a callable that looks at the signature/type hints and request those services from the container (requires a supplied Container as an argument)

Discussed in #81

Should there be a way to force a new value either at factory level or at get() level – or both?

As pointed out in #8 (comment), recursive services are currently only possible if you use a global-based container storage like our Flask integration.

To keep the envelope as narrow as possible, we should look at the arguments of the factory and automatically pass it the current Container.

The question is, how are we gonna detect it?

I can think of two ways and I think it's fine to support both:

1. take the first argument that is annotated to take a svcs.Container. This allows to freely name the argument.
2. take the first argument that is named svcs_container. This is a bit long, but it's unlike to clash with other parameter names that are either with defaults, or if we ever choose to allow argument passing via get(Type, argument=42). This also works in lambdas.

Opinions @elfjes?

As brought up in #8 (comment), it might make sense to allow pings to return a value that is then used by the health endpoints.