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

Support for Async Django #1394

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
6a5b28d
Resolve DjangoObjectType getNode when in an async context
jaw9c Mar 30, 2023
74998af
Support Django Connection resolving in an async context
jaw9c Mar 30, 2023
f04f0d3
Support foriegn key connections running async
jaw9c Mar 30, 2023
e78fb86
handle regualr django lists
jaw9c Mar 30, 2023
28846f9
drop in an async view
jaw9c Mar 30, 2023
7ddaf9f
Handle coroutine results from resolvers in connections and filter con…
jaw9c Mar 31, 2023
ebbc578
Strange scope
jaw9c Mar 31, 2023
66938e9
async hates csrf
jaw9c Mar 31, 2023
1b2d5e0
handle async serlizer mutations
jaw9c Mar 31, 2023
0a84a6e
Handle async get_node
jaw9c Apr 2, 2023
64d311d
Copy tests for query to test async execution
jaw9c Apr 2, 2023
bdb8e84
Update cookbook for async testing
jaw9c May 4, 2023
4d5132d
Remove tests for now
jaw9c May 4, 2023
4e5862f
Add logging of errors in execution
jaw9c May 4, 2023
c10753d
most recent changes
jaw9c May 4, 2023
e9d5e88
Handle the default django list field and test the async execution of …
jaw9c May 5, 2023
76eeea4
Update tests for queries
jaw9c May 5, 2023
c501fdb
swap back to python 3.11
jaw9c May 5, 2023
58b92e6
linting
jaw9c May 5, 2023
791209f
Rejig concept to use middleware
jaw9c May 9, 2023
b69476f
improve async detection
jaw9c May 9, 2023
b134ab0
Handle custom Djangoconnectionresolvers
jaw9c May 9, 2023
8c068fb
Update to pull out mutations and ensure that DjangoFields don't get d…
jaw9c May 10, 2023
d3f8fcf
handle connections without middleware
jaw9c May 16, 2023
930248f
Refactor out async helper functions
jaw9c May 16, 2023
c27dd6c
cleanup middleware
jaw9c May 16, 2023
fba274d
refactor middleware
jaw9c May 16, 2023
84ba7d7
Remove unused path
jaw9c May 16, 2023
848536e
Clean up wrapped list resolver
jaw9c May 16, 2023
2659d67
updates tests
jaw9c May 16, 2023
c16276c
Merge branch 'main' into support-async
jaw9c May 16, 2023
37ebd63
follow main removing validate call
jaw9c May 16, 2023
5a19111
Fix graphiql for async
pcraciunoiu May 25, 2023
2ae927f
Merge branch 'main' into support-async
firaskafri Aug 10, 2023
28bc858
resolve conflicts and fix format
firaskafri Aug 10, 2023
45fb299
Fix bug when running sync view under asgi
jaw9c Aug 16, 2023
b35f3b0
Merge branch 'main' into support-async
firaskafri Feb 9, 2024
c2d601c
Fix newline
firaskafri Feb 9, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion examples/cookbook/cookbook/recipes/schema.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,41 @@
from graphene import Node
import asyncio

from asgiref.sync import sync_to_async

from graphene import Field, Node, String
from graphene_django.filter import DjangoFilterConnectionField
from graphene_django.types import DjangoObjectType

from cookbook.recipes.models import Recipe, RecipeIngredient


class RecipeNode(DjangoObjectType):
async_field = String()

class Meta:
model = Recipe
interfaces = (Node,)
fields = "__all__"
filter_fields = ["title", "amounts"]

async def resolve_async_field(self, info):
await asyncio.sleep(2)
return "success"


class RecipeType(DjangoObjectType):
async_field = String()

class Meta:
model = Recipe
fields = "__all__"
filter_fields = ["title", "amounts"]
skip_registry = True

async def resolve_async_field(self, info):
await asyncio.sleep(2)
return "success"


class RecipeIngredientNode(DjangoObjectType):
class Meta:
Expand All @@ -28,7 +52,13 @@ class Meta:

class Query:
recipe = Node.Field(RecipeNode)
raw_recipe = Field(RecipeType)
all_recipes = DjangoFilterConnectionField(RecipeNode)

recipeingredient = Node.Field(RecipeIngredientNode)
all_recipeingredients = DjangoFilterConnectionField(RecipeIngredientNode)

@staticmethod
@sync_to_async
def resolve_raw_recipe(self, info):
return Recipe.objects.first()
9 changes: 5 additions & 4 deletions examples/cookbook/cookbook/urls.py
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))),
Copy link
Collaborator

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.

Copy link

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.

Copy link

@cpd67 cpd67 Aug 25, 2023

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 into cookbook. Should be good to merge into this PR, but let me know what you think!

]
28 changes: 28 additions & 0 deletions graphene_django/debug/middleware.py
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_sync_function
from .exception.formating import wrap_exception
from .sql.tracking import unwrap_cursor, wrap_cursor
from .types import DjangoDebug
Expand Down Expand Up @@ -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):
return sync_to_async(next)(root, info, **args)
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

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 sync_to_async pattern is a temporary shortcut before we have a true async ORM:

We’re still working on async support for the ORM and other parts of Django. You can expect to see this in future releases. For now, you can use the sync_to_async() adapter to interact with the sync parts of Django. There is also a whole range of async-native Python libraries that you can integrate with.

Regarding performances, this pattern has a cost and it's absolutely not obvious switching to async will improve them.
From Django's own documentation:

You should do your own performance testing to see what effect ASGI versus WSGI has on your code. In some cases, there may be a performance increase even for a purely synchronous codebase under ASGI because the request-handling code is still all running asynchronously. In general you will only want to enable ASGI mode if you have asynchronous code in your project.

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 sync_to_async, before releasing an async view for Graphene.

Copy link

Choose a reason for hiding this comment

The 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 😄

Copy link

@alexandrubese alexandrubese Dec 29, 2023

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 graphql-sync-dataloaders on our fork of graphene-django at my company. You can check out our tentative here:

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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)

Copy link

@alexandrubese alexandrubese Jan 9, 2024

Choose a reason for hiding this comment

The 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.
At this point, if someone does not fix dataloaders, I will just start using another graphql library because its getting ridiculous.

Thanks for your comments though, @superlevure


return next(root, info, **args)
69 changes: 55 additions & 14 deletions graphene_django/fields.py
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
from functools import partial

from asgiref.sync import sync_to_async
from django.db.models.query import QuerySet
from graphql_relay import (
connection_from_array_slice,
cursor_to_offset,
get_offset_with_default,
offset_to_cursor,
)
from promise import Promise

from graphene import Int, NonNull
from graphene.relay import ConnectionField
from graphene.relay.connection import connection_adapter, page_info_adapter
from graphene.types import Field, List

from .settings import graphene_settings
from .utils import maybe_queryset
from .utils import is_running_async, is_sync_function, maybe_queryset


class DjangoListField(Field):
Expand Down Expand Up @@ -46,19 +46,44 @@ def model(self):
def get_manager(self):
return self.model._default_manager

@staticmethod
@classmethod
def list_resolver(
django_object_type, resolver, default_manager, root, info, **args
cls, django_object_type, resolver, default_manager, root, info, **args
):
queryset = maybe_queryset(resolver(root, info, **args))
if is_running_async():
if is_sync_function(resolver):
resolver = sync_to_async(resolver)

iterable = resolver(root, info, **args)

if info.is_awaitable(iterable):

async def resolve_list_async(iterable):
queryset = maybe_queryset(await iterable)
if queryset is None:
queryset = maybe_queryset(default_manager)

if isinstance(queryset, QuerySet):
# Pass queryset to the DjangoObjectType get_queryset method
queryset = maybe_queryset(
await sync_to_async(django_object_type.get_queryset)(
queryset, info
)
)

return await sync_to_async(list)(queryset)

return resolve_list_async(iterable)

queryset = maybe_queryset(iterable)
if queryset is None:
queryset = maybe_queryset(default_manager)

if isinstance(queryset, QuerySet):
# Pass queryset to the DjangoObjectType get_queryset method
queryset = maybe_queryset(django_object_type.get_queryset(queryset, info))

return queryset
return list(queryset)

def wrap_resolve(self, parent_resolver):
resolver = super().wrap_resolve(parent_resolver)
Expand Down Expand Up @@ -226,20 +251,36 @@ def connection_resolver(

# eventually leads to DjangoObjectType's get_queryset (accepts queryset)
# or a resolve_foo (does not accept queryset)

if is_running_async():
if is_sync_function(resolver):
resolver = sync_to_async(resolver)

iterable = resolver(root, info, **args)

if info.is_awaitable(iterable):

async def resolve_connection_async(iterable):
iterable = await iterable
if iterable is None:
iterable = default_manager

iterable = await sync_to_async(queryset_resolver)(
connection, iterable, info, args
)

return await sync_to_async(cls.resolve_connection)(
connection, args, iterable, max_limit=max_limit
)

return resolve_connection_async(iterable)

if iterable is None:
iterable = default_manager
# thus the iterable gets refiltered by resolve_queryset
# but iterable might be promise
iterable = queryset_resolver(connection, iterable, info, args)
on_resolve = partial(
cls.resolve_connection, connection, args, max_limit=max_limit
)

if Promise.is_thenable(iterable):
return Promise.resolve(iterable).then(on_resolve)

return on_resolve(iterable)
return cls.resolve_connection(connection, args, iterable, max_limit=max_limit)

def wrap_resolve(self, parent_resolver):
return partial(
Expand Down
13 changes: 13 additions & 0 deletions graphene_django/filter/fields.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from collections import OrderedDict
from functools import partial

from asgiref.sync import sync_to_async
from django.core.exceptions import ValidationError

from graphene.types.argument import to_arguments
Expand Down Expand Up @@ -92,6 +93,18 @@ def filter_kwargs():

qs = super().resolve_queryset(connection, iterable, info, args)

if info.is_awaitable(qs):

async def filter_async(qs):
filterset = filterset_class(
data=filter_kwargs(), queryset=await qs, request=info.context
)
if await sync_to_async(filterset.is_valid)():
return filterset.qs
raise ValidationError(filterset.form.errors.as_json())

return filter_async(qs)

filterset = filterset_class(
data=filter_kwargs(), queryset=qs, request=info.context
)
Expand Down
13 changes: 13 additions & 0 deletions graphene_django/rest_framework/mutation.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from collections import OrderedDict
from enum import Enum

from asgiref.sync import sync_to_async
from django.shortcuts import get_object_or_404
from rest_framework import serializers

Expand All @@ -11,6 +12,7 @@
from graphene.types.objecttype import yank_fields_from_attrs

from ..types import ErrorType
from ..utils import is_running_async
from .serializer_converter import convert_serializer_field


Expand Down Expand Up @@ -158,6 +160,17 @@ def mutate_and_get_payload(cls, root, info, **input):
kwargs = cls.get_serializer_kwargs(root, info, **input)
serializer = cls._meta.serializer_class(**kwargs)

if is_running_async():

async def perform_mutate_async():
if await sync_to_async(serializer.is_valid)():
return await sync_to_async(cls.perform_mutate)(serializer, info)
else:
errors = ErrorType.from_errors(serializer.errors)
return cls(errors=errors)

return perform_mutate_async()

if serializer.is_valid():
return cls.perform_mutate(serializer, info)
else:
Expand Down
6 changes: 6 additions & 0 deletions graphene_django/tests/async_test_helper.py
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
Loading