Support for Async Django #1394
Support for Async Django #1394
Conversation
|
Progress update: All the fields resolvers are supporting both being executed in the async context and wrapping django sync code into a sync context. I've made the assumption that any resolving of Currently people switching to using the async view will need to wrap all their top level resolvers (those that are directly on the Next steps are to run through all the test files and adding the helper. It would great if people could test this either on their production projects (speciically when running under ASGI), as 'm interested in how swapping in/out of the sync context affects the performance of the execution. |
This is great news! Would discuss with my team to try this on production once its ready! |
|
@superlevure i think it is good to go as soon as we clarify the docs |
|
@dima-kov I had a look at your benchmarks and I have few remarks. First, it looks like you're comparing the Second, I noticed the I took the liberty of pushing a PR to your repo that covers those point, as well as setting up a I obtain the following results: Data tends to show a huge perf hit for the I suspect the I'll continue to play with the PR a bit tonight to make sure there's nothing wrong with my setup, but I'm curious if others can reproduce similar results. |
|
@superlevure thank you for looking into it! I missed that part |
|
regarding the results you've got: hmm, thats really strange. Both sync and async (100 requests concurrently and 1k requests in total) showed me +- 20s. 100+s is smth unbelievable for me. I'm playing with main version now. |
Django currently doesn't support persistent connections for ASGI https://code.djangoproject.com/ticket/33497 |
|
Thank you for all your work! How can we help to get it merged? |
https://docs.djangoproject.com/en/5.1/releases/5.1/#postgresql-connection-pools django 5.1 will support connection pool and this might be an opportunity to merge the PR |
|
I was getting an error that the view is not async on the latest Django release Based on this, there must be an async method or the property returns false. As a quick fix, I pushed this classproperty to be true: Feel free to add that in. |
|
Apologies for the radio silence on this thread. Appreciate the feedback and efforts from everyone on this PR - in particular the benchmarking. A fair few months ago, we rolled this into production at my work, utilising the new I think this is safe to merge, but with an experimental warning for those willing to test and use the I've been carefully watching the movements by the Django team on making the ORM fully async, this PR is the latest, which is really encouraging! |
Looks like Django 5.1 got released! |
|
Are there plans to publish a tagged release? Happy to support if anything is missing. |
@jaw9c, do you think it's safe to merge even though my benchmarks showed this PR degrades performance for |
|
any luck with this? |
|
Is there any update of this? |
|
Maybe I'm missing something, but what's the point of graphene version 3 as long as it is simply not working?! or at least data loaders are not working. Not even with Django 5.1 |
|
@androane graphene-django project died long ago, but I wasn't able to give an EOL status to it when I was a maintainer. (then somebody revoked my rights without communicating first) This is not a maintained project, it's just a couple of people at the time didn't want to archive it. Occasional merges of a couple of small things here and there is not even remotely enough to keep an API-layer library modern. I helped multiple startups to remove graphene from their stack, or replace with Strawberry. Trying to keep this tool alive was a lot of work even 5 years ago and today, it's generally "Oh we made a mistake, please save us!" level thing. It's amazing to see how much time and energy is wasted just because there is no clear "NOT MAINTAINED" warning in the place. |
|
@ulgens Thanks so much for your answer, that explains so much and confirms my hunches as well... |
There was a problem hiding this comment.
Merging something of this importance into a dead project will only confuse people, and (considering the project's history and lack of latest activities) I don't think that people will be around to improve or fix it when it will be needed.
Keeping the PR open gives an impression that someday this will be a working part of graphene, so I guess the least I can do is to reject it.
|
That's extremely useful context, @ulgens. Thanks. |
|
I've been pinged about this thread two times in the last week.
I'll be taking on ownership of graphene-django back to add support of the learnings on using it with dataloaders over the last years in @wasmerio (we are using it heavily). Stay tuned |
|
Also, @jaw9c thanks for your hard work on this PR. I have no idea why no one on the maintainers team decided to merge your PR, but I'll be reviewing it soon and hopefully merge. As part of the team of maintainers of Graphene Django, my sincere apologies for taking so much time to review it. |
|
@syrusakbary thank you! Graphene-Django still falls back to sync behavior in key execution paths. Given posted async load tests, do you see a feasible roadmap for full async compatibility? Do we need to wait full async compatibility from django core (if this will be merged: django/django#18408)? Happy to contribute if there’s a focused plan—what do you see as the major blockers? |
|
Glad to have you back @syrusakbary! Thanks for your kind words - I think the main blocker became that the performance didn't increase due to the reasons outlined by @dima-kov above. The majority of people will be hitting the ORM during their queries, and running async with results in more (slightly more) computation from nested use of |
|
I took a deeper look on the PR.
|
Using async with Django Channels / ASGI improves things a tiiny bit (since they use the For non-asgi views performance will be on par (if no middleware is added). |
Feedback requested.
This PR adds support for Django async views. The general strategy follows the same principal as the base graphene library when supporting async. It implements the following things:
sync_to_asyncThe current progress is as followed:
Other thoughts:
sync_to_asyncif they are performing ORM ops. It would be nice to detect this error and throw a warning on existing codebases.