graphql-python / graphql-server

This is the core package for using GraphQL in a custom server easily
MIT License
120 stars 72 forks source link

feat: flask asyncio support for dataloaders #66

Open wakemaster39 opened 4 years ago

wakemaster39 commented 4 years ago

Resolves #65

This takes the approach that no one should already have an event loop and makes one and runs it in context. This is also only python 3.7+ supported because of the use of asyncio.run. I am not sure if the run logic should be backported into here so it works on 3.6 as well.

KingDarBoja commented 3 years ago

Looking deeper into Flask itself, adding async capabilities seems to be a PITA as the framework itself does not support (AFAIK) async tasks by itself. However I found this alternative that seems to resemble Flask pattern: https://gitlab.com/pgjones/quart.

This is even recommended from the Pallets Team as stated on this issue: https://github.com/pallets/werkzeug/issues/1322#issuecomment-600926145

Cheers!

wakemaster39 commented 3 years ago

@KingDarBoja you are 100% right that flask does not use asyncio and more specifically does not support ASGI. But this PR isn't about supporting ASGI, it is about supporting async calls so dataloaders can work.

ASGI is full async parallel calls craziness and that is honestly not something I am interested in or care to use. (This is my understanding of it at least). What this does is creates an event loop in the context of a single WSGI request and allows you to use async/await calls in that request. The request itself is still blocking.

I had to move away from this for a bit but I am going to be resuming it again next week. I am good if it is decided against allowing the creation of the event loop. But can we open up a hook inside of the call chain where someone else can with minimal work? With the removal of Promises, the original dataloaders library no longer works, which has blown apart my application and this seemed like the better call than injecting promise resolution in the same spot.

I originally went the route of adding the creation of the event loop to make it easy on people and I wanted to make it a simple override of the function if you wanted to remove it as I doubt many people are using asyncio in flask right now because it doesn't make sense in a-lot of situations.

I'll play around with the code a bit more next week and clean it up some if you are good with supporting this to some extent.

KingDarBoja commented 3 years ago

I had to move away from this for a bit but I am going to be resuming it again next week. I am good if it is decided against allowing the creation of the event loop. But can we open up a hook inside of the call chain where someone else can with minimal work?

Yeah, that should be the way to go, like the enable_async flag behaviour is gonna be used slighty different than sanic or aiohttp does and should be documented as well. I will refactor it a bit and try to provide a method call like you did but also should be great to add some tests because the CI passed as result of what you did but we don't know if it would break the server under certain circumstances.

Still opened #68 to add support for Quart as well as it is not so complicated to support, and keep this PR for the dataloader request.

Cheers!

wakemaster39 commented 3 years ago

@KingDarBoja I finally had the bandwidth to come back to this, its been far too long.

I cleaned up and minimized the changes. There is now a single hook point you can overload if you use async somewhere else in flask and setup your own thread. But out of the box it "just works"

colelin26 commented 2 years ago

Hi there! Are there any updates on this? I tried using this in my Flask, graphene V3 application and it still does not work for aiodataloader. I got this error message:

Task <Task pending name='Task-16' coro=<ExecutionContext.resolve_field.<locals>.await_result() got Future <Future pending> attached to a different loop

It seems to me that asyncio.run tried to create a new event loop, but there already exists another event loop for this task due to the implementation of aiodataloader. A normal async resolver (non-dataloader) would work for this approach tho.

Problem fixed by constructing aiodataloader inside resolver

fffergal commented 2 years ago

I recently had to deal with this at work while upgrading graphene. Would you like some help getting this out? I can review/write code/update the docs. I copy pasted the graphql_server.flask.graphqlview module and made changes, but would like to upstream that if possible.

As one user, my preference is to not start the event loop inside a library. asyncio usage is often very opinionated, bespoke, and hard to unwind. I would rather this library return a coroutine and leave the event loop to users, with some examples in the docs for starting an event loop inside the user's Flask view (with asyncio.run) to get people started. For example I had a similar problem to coelin26 where I needed to init the DataLoaders within the same event loop. I imagine this will be a common thing as DataLoaders are the reason we need to move to async.

Please let me know if the maintainers are interested in some help to merge and release this update! Thank you.

kiendang commented 1 year ago

@fffergal I agree that starting an event loop inside the library is not ideal. If you're still interested in working on this and submitting a PR I'd be happy to review.

kiendang commented 1 year ago

Just opened #102 which implements this for Flask 2.

fffergal commented 1 year ago

@fffergal I agree that starting an event loop inside the library is not ideal. If you're still interested in working on this and submitting a PR I'd be happy to review.

I'm sorry, I don't work at the same place any more so I'm not using graphql-python at the moment. I did notice while using asyncio DataLoaders, queries weren't being grouped together anymore, so there may be more to getting asyncio DataLoaders working effectively, vs. just working.