-
Notifications
You must be signed in to change notification settings - Fork 820
Description
I know graphql-core dropped support for promises, but the author seems to think promise-support can be added via hooks like middleware and execution context (see response to my identical issue in graphql-python/graphql-core#148).
Since most people using syrusakbary's promise library are probably already graphene users, if anyone is going to help make graphql-core 3 and promises play together, it makes sense for that to be done in graphene.
Why not just use async?
I think I have a decent use-case for non-async, promise-based resolution. Async is nice and all, and having a standard is great, but many of us are just using dataloaders as an execution pattern, not because we actually have async data-sources. Moving everything to run in async environment can have consequences.
We are calling the django ORM from our dataloaders. Because django 3.0 forces us to isolate ORM calls and wrap them in sync_to_async
, we stuck with promise based dataloaders for syntactic reasons. Examples below:
What we'd like to do, but django doesn't allow
class MyDataLoader(...):
async def batch_load(self, ids):
data_from_other_loader = await other_loader.load_many(ids)
data_from_orm = MyModel.objects.filter(id__in=ids) # error! can't call django ORM from async context.
# return processed combination of orm/loader data
What django would like us to do
class MyDataLoader(...):
async def batch_load(self, ids):
data_from_other_loader = await other_loader.load_many(ids)
data_from_orm = await get_orm_data()
# return processed combination of orm/loader data
@sync_to_async
def get_orm_data(ids):
return MyModel.objects.filter(id__in=ids)
What we settled on instead (use generator-syntax around promises)
class MyDataLoader(...):
def batch_load(self,ids):
data_from_other_loader = yield other_loader.load_many(ids)
data_from_orm = MyModel.objects.filter(id__in=ids)
# return processed combination of orm/loader data
A simple generator_function_to_promise
is used as part of dataloaders, as well as a middleware that converts generators returned from resolvers into promises. I have hundreds of dataloaders following this pattern. I don't want to be stuck isolating all the ORM calls as per django's recommendations. That would be noisy and decrease legibility.
It seems there may be other issues around using async dataloaders with connections. Admittedly that problem sounds more easily surmountable and wouldn't require supporting promises.
Activity
Topher-the-Geek commentedon Feb 1, 2022
Graphene's own docs still talk about using promises for DataLoaders. How does one actually implement DataLoaders in v3?
MaehMaeh commentedon Feb 4, 2022
Probably no longer per Promise. One possibility should be aiodataloader.
https://github.com/syrusakbary/aiodataloader
There is an entry about it here:
Graphene 3 dataloader #1273
Here you can see the modified documentation:
https://github.com/graphql-python/graphene/pull/1190/files/380166989d9112073c170d795801e2cd068ea5db#diff-3c9528a2b7a1be6a0549aeea89b4090c51f2ae426fd9ffb72d96112abecd02d8
If you can get a working solution, I would be very interested :)
p.s.: Your linked manual should still be for Graphene 2!
DenisseRR commentedon Apr 6, 2022
tested with
aiodataloader
as suggested. It doesn't work.I get:
There is no current event loop in thread 'ThreadPoolExecutor-0_0'.
superlevure commentedon Jun 23, 2022
I run into the same error as @DenisseRR, @AlexCLeduc did you manage to make
aiodataloader
work with Django?If yes, could you share an example of doing so?
AlexCLeduc commentedon Jun 23, 2022
@superlevure
I'm still on graphene 2 and promise-based dataloaders, but the async dataloaders definitely work. The docs should ideally cover this, I can't think of a scenario where I'd use the
GraphQLView
as is, without subclassing it with a bunch of stuff.Below, an explicit async loop is created within a synchronous view. The important part is that you make sure you're executing the resolvers in "async mode" and make sure you're using the same dataloader instances across resolver calls. There's some extra stuff you'll almost certainly want, like logging and attaching the user to the context:
Now your resolvers can access the user dataloader through context, e.g.
By the way, it would be nice if graphene provided an
AsyncGraphqlView
, like Strawberry. Directly messing with event loops can be a little daunting.erikwrede commentedon Jun 24, 2022
@AlexCLeduc w.r.t. the AsyncGraphqlView, I think this is a
graphene-django
issue. Maybe there is a way to upgradegraphql-server
instead, supporting both sync and async views for all possible backends. But that is definitely a larger task.I'm not using Django - how's the progress on event loops there? Can we expect event loops to be available for future versions or will it always be necessary to create our own graphene-exclusive event loop?
superlevure commentedon Jun 27, 2022
Thank you for the explanation and the snippet @AlexCLeduc.
AsyncioExecutor
is not available anymore in graphene 3, I'm working on porting your example to the new API, I'll post the result if I manage to make it work.AlexCLeduc commentedon Jun 27, 2022
@superlevure, after playing around a bit with v3, getting async resolvers/dataloaders working correctly is quite easy when calling the schema directly (e.g.
schema.execute_async
) but there's more work involved in getting graphene_django'sGraphQLView
class to play nice with that.There is an open PR adding an
AsyncGraphqlView
, although that's more ambitious. It's an asgi django view, not just a plain wsgi view that uses async purely for batching purposes (Like I had working in v2 above). Since I'm not particularly interested in an asgi view, I just copied graphene_djangos'sGraphQLView
'sexecute_graphql_request
to call a wrapped version of schema.execute_async:It seems to work, and I validated that the batching is working properly, but it would not surprise me if mutations were broken, they sometimes behave strangely when executed asynchronously
superlevure commentedon Jun 28, 2022
Thank you @AlexCLeduc !
In the mean time I had found about graphql-python/graphene-django#1256 too and especially this repo https://github.com/fabienheureux/graphene-async (thanks to @fabienheureux) that provides a working example of
iaodataloader
with Django and theAsyncGraphqlView
of that PR.I could make both of your examples work (the gain in performances is quite significant, I went from 1102 SQL queries to.. only 2 in my specific use case) but the introduction of
asyncio
in my app leads to many issues. In particular, it seems that I need to wrap every calls to the ORM (even the ones not part of adataloader
) inside async_to_async
which is not feasible in my case.I'm considering going back to
Graphene 2
and promised baseddataloader
.felixmeziere commentedon Sep 22, 2022
This working would be life-changing for us!
jkimbo commentedon Sep 23, 2022
So after investigating lots of other options for supporting DataLoaders with Django I've come to the conclusion that the best approach is by implementing a custom execution context class in graphql-core to handle "deferred/future" values. I've proposed this change here graphql-python/graphql-core#155 but until we can get it merged into graphql-core I've published a library to let you integrate it with Graphene (v3) now: https://github.com/jkimbo/graphql-sync-dataloaders
@superlevure @felixmeziere @AlexCLeduc I hope this is able to help you.
AlexCLeduc commentedon Sep 23, 2022
Thanks @jkimbo! Will this work in a nested context, e.g. dataloaders that compose one another, or a resolver that calls multiple dataloaders? It's not obvious to me how to chain futures
jkimbo commentedon Sep 23, 2022
@AlexCLeduc can you give an example of what you mean?
AlexCLeduc commentedon Sep 23, 2022
@jkimbo Promises were chainable using the
.then(handler)
api. Are your future objects also chainable?If a resolver requires 2 dataloader calls, e.g. a grandparent field that calls a parent_loader twice, using the old promise API would look something like this,
jkimbo commentedon Sep 23, 2022
@AlexCLeduc ah yeah that's not possible yet but I'll have a look at adding it.
ericls commentedon Oct 4, 2022
Thanks @jkimbo for graphql-sync-dataloader.
We were having the same issue and decided to translate the default executecontext to be promise aware and promise based. I started translating it as a fork of graphql-core a few weeks ago, but decided to publish the execute context as a separate package here: https://github.com/fellowapp/graphql-core-promise.
AlexCLeduc commentedon Oct 4, 2022
@jkimbo thanks for adding chaining! I tried to use it but I couldn't find an analog to
Promise.all
orDataloader.load_many
Thanks @ericls, your execution-context class was a drop in replacement and works beautifully! I didn't think this was possible without modifying graphql-core.
ericls commentedon Oct 5, 2022
Glad you find it useful!
That was basically a re-write of execution context, I'm glad graphql-core provides an API to override it. By the way, are you in Ottawa Canada?
felixmeziere commentedon Oct 5, 2022
Thanks a lot @jkimbo !!!
AlexCLeduc commentedon Oct 5, 2022
@ericls yes I'm in that Ottawa haha. Glad to hear fellowapp is still using promises, makes me feel less crazy 😄
If anyone is interested in the generator syntactic sugar I described in the OP,
it's quite simple to implement. Not having to rewrite it is a motivation to keep using the old promise libraryIt's now its own packageAlexCLeduc commentedon Oct 21, 2022
When I opened this issue, I was unaware the execution context was capable of adding promise support. I'm happy enough with @ericls's solution, we can close this
erikwrede commentedon Jun 22, 2024
Closing this as this, as further work on this is currently not planned and there is no available capacity to bring this to the main library.