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

First pass #1

Closed
adriangb opened this issue Jul 26, 2021 · 22 comments
Closed

First pass #1

adriangb opened this issue Jul 26, 2021 · 22 comments

Comments

@adriangb
Copy link
Owner

@graingert this was my weekend project, an attempt to elaborate upon / understand tiangolo/fastapi#3516 (comment)

I'm still not sure if I captured what you were thinking there (there's no CM factory functions really), but I'm still curious what you think of this in general.

The goal was to take FastAPI's DI system and:

  • generalize it a bit, e.g. not hardcore parameters related to requests/responses, instead use containers to bind these at runtime
  • make it a bit faster (I don't think FastAPI parallelizes the DAG via task groups)
  • use anyio as the backend
  • move as much work as possible to compile / wiring time, in particular I tried to resolve all of the caching during compile time so that at runtime you just run through the DAG, pulling overrides / solved values from a flat dict
@graingert
Copy link
Contributor

Oh awesome, would be good to adapt some of the pytest-trio tests in here, eg https://github.com/python-trio/pytest-trio/blob/master/pytest_trio/_tests/test_async_yield_fixture.py

@adriangb
Copy link
Owner Author

Yep will try that while I write out some more tests

@adriangb
Copy link
Owner Author

I parametrized the (currently single bad test) under anyio via 9f59fba, seems to be working great! I like how it auto-parametrizes multiple backends, make it super easy to test

@graingert
Copy link
Contributor

this might be interesting also: python/cpython#27089

@adriangb
Copy link
Owner Author

Interesting, that's pretty fresh, I'll keep an eye on it.

@adriangb
Copy link
Owner Author

I added a basic comparison of what this would look like vs. FastAPI here: https://github.com/adriangb/anydep/tree/main/comparisons

I'm fairly confident that this can be a drop-in replacement for the current DI system

@adriangb
Copy link
Owner Author

@graingert this may be of interest to you.

I was able to simplify things a lot by using closures to wrap functions instead of executing them directly, like FastAPI does.

For example, in Starlette:

async def run_in_threadpool(
    func: typing.Callable[..., T], *args: typing.Any, **kwargs: typing.Any
) -> T:
    ...
    return await anyio.to_thread.run_sync(func, *args)

Instead:

def callable_in_thread_pool(call: Callable[..., T]) -> Callable[..., Awaitable[T]]:
    async def inner(*args: Any, **kwargs: Any) -> T:
        # the same code
        return await anyio.to_thread.run_sync(func, *args)
    return inner

This let me convert every callable (even generators) to a coroutine accepting the same parameters without executing anything. It then became much easier to create a DAG of coroutines awaiting their dependencies, using an anyio task group when there are multiple dependencies.

@graingert
Copy link
Contributor

graingert commented Jul 30, 2021

Here's a thread about how this could be used in pytest: pytest-dev/pytest#3834

Eg with anydep and an @anydep.pytest.inject decorator to support "session", "module", "function" scoped fixtures

@graingert
Copy link
Contributor

graingert commented Jul 30, 2021

also I was wondering on your thoughts about using typing.Annotated instead of kwarg defaults, eg before:

import contextlib
from anydep import Depends

@contextlib.asynccontextmanager
async def run_some_server(
    commons: dict[str, int] = Depends(common_parameters),
) -> AsyncGenerator[Example, None]:
    async with anyio.create_task_group() as tg:
        port = await tg.start(some_server, commons)
        yield port

after:

import contextlib
from anydep import Depends
from typing import Annotated

@contextlib.asynccontextmanager
async def run_some_server(
    commons: Annotated[dict[str, int], Depends(common_parameters)],
) -> AsyncGenerator[Example, None]:
    async with anyio.create_task_group() as tg:
        port = await tg.start(some_server, commons)
        yield port

or using https://www.python.org/dev/peps/pep-0593/#aliases-concerns-over-verbosity

import contextlib
from anydep import AnnotatedDepends

@contextlib.asynccontextmanager
async def run_some_server(
    commons: AnnotatedDepends[dict[str, int], common_parameters],
) -> AsyncGenerator[Example, None]:
    async with anyio.create_task_group() as tg:
        port = await tg.start(some_server, commons)
        yield port

@graingert
Copy link
Contributor

graingert commented Jul 30, 2021

@adriangb could you add a test for entering concurrent request contexts, eg with a Starlette app with one Lifespan context and several concurrent HTTP request contexts?

@adriangb
Copy link
Owner Author

Eg with anydep and an @anydep.pytest.inject decorator to support "session", "module", "function" scoped fixtures

That certainly sounds like a good feature once the basics are more settled.

I was wondering on your thoughts about using typing.Annotated

I think it's a nice syntax, but the issue with Annotated is that you can't pass keyword arguments. So it becomes a bit cumbersome to e.g. pass scope=False or (for FastAPI) security_scopes=[...]. For example, you'd have:

def dep() -> int:
    return 1

# current
run_some_server(v: int = Security(dep, scope=False, security_scopes=["openid"])):
    ...

# new
run_some_server(v: Security[int, dep, False, ["openid"]]):
    ...
# or maybe
run_some_server(v: Security(dep, scope=False, security_scopes=["openid"])[int]):
    ...

Aside from that, it's also now 2 ways to do something, and both need to be supported.
Maybe we can re-evaluate at a later point? It's just syntactic sugar after all.

could you add a test for entering concurrent request contexts, eg with a Starlette app with one Lifespan context and several concurrent HTTP request contexts?

This is a good point, something I thought about but decided to punt.
As you probably noticed, binds / caching right now are global, hence if you can only enter one request scope at a time.
My original thought was "just take the existing state (a bunch of dicts), throw them in a contextvar and donzo".
Unfortunately it's not that simple. Starlette doesn't process requests in the same context as the lifespan:

from contextvars import ContextVar

from starlette.applications import Starlette
from starlette.responses import JSONResponse
from starlette.routing import Route


ctx = ContextVar[dict]("ctx")


async def homepage(request):
    return JSONResponse(ctx.get())  # LookupError


async def lifespan(app):
    token = ctx.set({})
    try:
        yield
    finally:
        ctx.reset(token)


app = Starlette(
    routes=[
        Route('/', homepage),
    ],
    lifespan=lifespan
)

This means that if state was managed by contextvars, entering a scope within a lifespan event wouldn't work.

One way to solve this is to have 2 states: one global and one context specific and then trying to merge them at runtime:

async with container.enter_global_scope("app"):  # edits instance variables
    async with container.enter_local_scope("request"):   # uses contextvars
        ...

Some issues that arise immediately are what happens if you enter a global scope within a local scope within a global scope? So this seems like there might be some rough edges.
I'm open to other ideas.

@adriangb
Copy link
Owner Author

adriangb commented Jul 30, 2021

Well I think I figured the context issue out. Here's the test:

https://github.com/adriangb/anydep/blob/adefb1eb2ca787436a86f16f0adeb7bb37837f7c/tests/test_binding.py#L34-L68

I'm sure there's still some rough edges though.

@graingert
Copy link
Contributor

graingert commented Jul 30, 2021

Starlette doesn't process requests in the same context as the lifespan:

that's an interesting idea, and would probably fix a lot of problems I'm having with asgi servers and cancellation. Starlette could starts its own taskgroup in lifespan and run other all asgi tasks in it

@adriangb
Copy link
Owner Author

yeah that sounds, generally, like a solid idea. although I don't know how that would impact the ASGI protocol.

@adriangb
Copy link
Owner Author

adriangb commented Jul 30, 2021

from a users perspective, it would be nice to be able to assume that the lifespan wraps requests, so that setting a contextvar in the lifespan propagates into every request

@agronholm
Copy link

How does this project compare to python-dependency-injector? Seems that the aim is to create something similar, but with SC/AnyIO support?

@adriangb
Copy link
Owner Author

adriangb commented Jul 31, 2021

That's an interesting question @agronholm.

Firstly, python-dependency-injector does support coroutines, and is able to parallelize collection of dependencies. It is tightly coupled to asyncio, although I'm sure they could change that without breaking their API.

Secondly, this package's primary goal is to further develop FastAPI's DI system. As such, it's not very useful in a lot of the situations where python-dependency-injector might shine. For example, this package only executes code via a coroutine, even if all of the dependencies are sync and non-parallelizable. So right off the bat if your project is all sync stuff, this is probably not the right choice.

That said, I think there are some interesting comparisons that can be made:

  • This package only requires a reference to the dependency provider (a callable), even via a type annotation, to wire up a graph. So you don't need to reference your container all over the place, nor do you need to bind every dependency to the container. The downside to this is that python-dependency-injector can easily handle two copies of the same dependency w/ one being a Singleton and another being a Factory or something, while this package doesn't handle that very gracefully (at least via the current API).
  • This package arbitrary lifetimes for dependencies. python-dependency-injector supports multiple caching scopes and startup/teardown lifetimes (via it's various providers and markers), but for lifetimes you get to choose between function-scoped or container-scoped (as far as I can tell), so I think this package offers more flexibility.

Ideally, I would like this package to support most of the feature set of python-dependency-injector but be simpler and easier to use, while maintaining compatibility with FastAPI so that (hopefully) it can be folded back in there.

Edit: I added some simple comparisons between anydep, python-dependency-injector and FastAPI here https://github.com/adriangb/anydep/tree/main/comparisons/simple
The thing that stands out is that (in this trivial example) anydep and pdi are 2x as fast as FastAPI (because they parallelize the sleep).

@adriangb
Copy link
Owner Author

If you can think of things that python-dependency-injector can do that this API can't or struggles with, please share and we can brainstorm changes 😄

@adriangb
Copy link
Owner Author

also I was wondering on your thoughts about using typing.Annotated instead of kwarg defaults, eg before:

I'm actually reconsidering this.

The main reason is that I ended up having to make a mixin between the dependency marker and Pydantic's FieldInfo to get things like multipart bodies working. I really dislike this pattern. Pydantic does allow you to use Field in Annotated, which solves the problem, but it might be good to support this here.

I think it's a nice syntax, but the issue with Annotated is that you can't pass keyword arguments.

I think this was mostly misguided knee jerk reaction on my end. I think that example would look like:

# current
run_some_server(v: int = Security(dep, scope=False, security_scopes=["openid"])):
    ...

# new
run_some_server(v: Annotated[int, Security(dep, scope=False, security_scopes=["openid"])]):
    ...

I think I would go with plain Annotated syntax since it is more composable than having aliases.

One thing that I do think would be a loss is that there would no longer be an error if the type returned by the dependency does not match the annotation: https://mypy-play.net/?mypy=latest&python=3.10&gist=55444ec8a6b5af50414fdf7dbad6d1a4. Maybe that is an argument for supporting both syntaxes?

As far as the change goes, it would not even be a change to the container or the underlaying protocol, it would just be a change to the implementation of Dependant here.

If you have time @graingert , I would appreciate your thoughts.

@adriangb
Copy link
Owner Author

How does this project compare to python-dependency-injector? Seems that the aim is to create something similar, but with SC/AnyIO support?

Interestingly, the current implementation is basically sans-io. The only place IO is used is in the implementation (not even in the interface) of the executors (here). Everything else is sans-io.

@adriangb
Copy link
Owner Author

also I was wondering on your thoughts about using typing.Annotated instead of kwarg defaults, eg before:

I'm actually reconsidering this.

Well it's done. The implementation wasn't all that bad (2474c14#diff-8506c77d2d3d79a4d95249dd601dd7d3dcc113f8496db6dd370c146e9f8ef213) which I think is a good sign that the API has the right flex points 🤓

@adriangb
Copy link
Owner Author

Unfortunately it's not that simple. Starlette doesn't process requests in the same context as the lifespan.

Well, this fix for this has turned out to be in architecture. In #39 I was able to decouple the container state from contextvars, so now in https://github.com/adriangb/asterisms I can manage the container state manually (here and here). There's still a thin wrapper that uses contextvars for convenience.

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

3 participants