-
Notifications
You must be signed in to change notification settings - Fork 767
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
Support for Async Django #1394
base: main
Are you sure you want to change the base?
Support for Async Django #1394
Changes from all commits
6a5b28d
74998af
f04f0d3
e78fb86
28846f9
7ddaf9f
ebbc578
66938e9
1b2d5e0
0a84a6e
64d311d
bdb8e84
4d5132d
4e5862f
c10753d
e9d5e88
76eeea4
c501fdb
58b92e6
791209f
b69476f
b134ab0
8c068fb
d3f8fcf
930248f
c27dd6c
fba274d
84ba7d7
848536e
2659d67
c16276c
37ebd63
5a19111
2ae927f
28bc858
45fb299
b35f3b0
c2d601c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,10 @@ | ||
from django.conf.urls import url | ||
from django.contrib import admin | ||
from django.urls import re_path | ||
from django.views.decorators.csrf import csrf_exempt | ||
|
||
from graphene_django.views import GraphQLView | ||
from graphene_django.views import AsyncGraphQLView | ||
|
||
urlpatterns = [ | ||
url(r"^admin/", admin.site.urls), | ||
url(r"^graphql$", GraphQLView.as_view(graphiql=True)), | ||
re_path(r"^admin/", admin.site.urls), | ||
re_path(r"^graphql$", csrf_exempt(AsyncGraphQLView.as_view(graphiql=True))), | ||
] | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
from asgiref.sync import sync_to_async | ||
from django.db import connections | ||
from graphql.type.definition import GraphQLNonNull | ||
|
||
from ..utils import is_running_async, is_sync_function | ||
from .exception.formating import wrap_exception | ||
from .sql.tracking import unwrap_cursor, wrap_cursor | ||
from .types import DjangoDebug | ||
|
@@ -67,3 +70,28 @@ def resolve(self, next, root, info, **args): | |
return context.django_debug.on_resolve_error(e) | ||
context.django_debug.add_result(result) | ||
return result | ||
|
||
|
||
class DjangoSyncRequiredMiddleware: | ||
def resolve(self, next, root, info, **args): | ||
parent_type = info.parent_type | ||
return_type = info.return_type | ||
|
||
if isinstance(parent_type, GraphQLNonNull): | ||
parent_type = parent_type.of_type | ||
if isinstance(return_type, GraphQLNonNull): | ||
return_type = return_type.of_type | ||
|
||
if any( | ||
[ | ||
hasattr(parent_type, "graphene_type") | ||
and hasattr(parent_type.graphene_type._meta, "model"), | ||
hasattr(return_type, "graphene_type") | ||
and hasattr(return_type.graphene_type._meta, "model"), | ||
info.parent_type.name == "Mutation", | ||
] | ||
): | ||
if is_sync_function(next) and is_running_async(): | ||
return sync_to_async(next)(root, info, **args) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on what I understand from the documentation, this is going to wrap any synchronous resolver that accesses the DB into a separate thread, isn't it? If this is the case indeed, isn't this going to have a substantial bad impact on performances? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For CPU bound resolvers the overhead will make the performance a lot worse. Since most Django apps are backed by an external DB, most resolvers are IO bound - so the advantage comes from the main loop can continue to serve other requests/continue resolving other parts of the query. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree with @jaw9c, also this is the main approach of Django core team on the way to making Django async There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I agree most resolvers are IO bound, we still miss benchmarks to assert that performance will improve with this pattern. The cost of creating a thread for each resolver + event loop task context switching VS blocking DB calls tradeoff is not obvious at all and I wouldn't be surprised if the synchronous version performs better for simple queries. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's a bit more complicated than that. The Django team is working on bringing async to the project but it is still a huge WIP. The ORM is still fully synchronous and the
Regarding performances, this pattern has a cost and it's absolutely not obvious switching to async will improve them.
As far as I understand, we don't have any true async code here, as we're just wrapping DB calls into threads. Hence, I suggest performing benchmarks to assert we're indeed bringing a performance boost and not the opposite. Depending on the results, we may have to wait for a true async ORM, and implementing the asynchronous variants of QuerySet instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jaw9c It sounds like you have a way to get some sort of benchmarks to show the performance impact, but if not I can help add those. I'm not sure what the best way would be to get them, but I can do my best 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @superlevure but what is the alternative to use for dataloaders ? In the documentation there are only async dataloaders mentioned. Dataloaders are an important part of any graphql API, I don't understand how the upgrade was done to v3 without taking into consideration dataloaders?! Linked issue: #1425 There is the sync dataloaders package(https://github.com/jkimbo/graphql-sync-dataloaders) but that is an external package, what solution did you guys have in mind for dataloaders in the new graphene versions? If async support is bad with performance degradation, then make sync dataloaders work in your latest graphene version, with examples in the documentation so people know how to use them and don't get blocked. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, the current situation regarding dataloaders is unfortunate. We've been working on adding them back on v3 thanks to
We've been using this in production for a few months now with success, our plan is to eventually submit this to upstream when we are confident enough and we have the bandwidth. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that our implementation of dataloaders for many-to-one relations is not covered in those two PRs as it's implemented in a private repo, but it basically looks like this: @classmethod
def get_node(cls, info, id):
if not graphene_settings.USE_DATALOADERS:
return super().get_node(info, id)
model_name: str = cls._meta.model._meta.model_name
queryset: QuerySet = cls.get_queryset(cls._meta.model.objects, info)
if not hasattr(info.context, "dataloaders"):
info.context.dataloaders = {}
if model_name not in info.context.dataloaders:
def load_many(keys: list[UUID | str]):
qs: QuerySet = queryset.filter(id__in=keys)
object_map: dict[str, Any] = {str(object_.id): object_ for object_ in qs}
return [object_map.get(str(key), None) for key in keys]
info.context.dataloaders[model_name] = SyncDataLoader(load_many)
return info.context.dataloaders[model_name].load(id) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not realise that for many-to-one relations you actually need another fix to get this working. Thanks for your comments though, @superlevure |
||
|
||
return next(root, info, **args) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from asgiref.sync import async_to_sync | ||
|
||
|
||
def assert_async_result_equal(schema, query, result, **kwargs): | ||
async_result = async_to_sync(schema.execute_async)(query, **kwargs) | ||
assert async_result == result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to make the async view the only, and default, one in the cookbook? I feel we should have both examples present, the sync one will most likely still be the most common one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaw9c I can help add an example with the sync view. I think having both examples present makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaw9c I created a PR to have both an async & sync example of the cookbook. I basically took these changes, stuck them in
cookbook-async
and put the sync code back intocookbook
. Should be good to merge into this PR, but let me know what you think!