Async implementation #8617

em1208 · 2022-08-19T09:03:57Z

Note: Before submitting this pull request, please review our contributing guidelines.

Description

This PR is related to issue refs #8496. It is a tentative implementation of async for APIView.

Regarding the check if not async_to_sync I have time it and the overhead is small (if not is faster than is None):

In [1]: %timeit if None is None:pass
11.3 ns ± 0.0369 ns per loop (mean ± std. dev. of 7 runs, 100,000,000 loops each)

In [2]: %timeit if not None:pass
5.8 ns ± 0.00717 ns per loop (mean ± std. dev. of 7 runs, 100,000,000 loops each)


In [3]: def func():
   ...:     pass
   ...: 

In [4]: %timeit if func is None:pass
17.9 ns ± 0.162 ns per loop (mean ± std. dev. of 7 runs, 100,000,000 loops each)

In [5]: %timeit if not func:pass
16.3 ns ± 0.115 ns per loop (mean ± std. dev. of 7 runs, 100,000,000 loops each)

tomchristie · 2022-08-22T11:27:15Z

Great - thanks for your time on this - all looks neatly done.

My concerns here are around introducing false expectations to our users.

I'll explore this over on #8496

rest_framework/views.py

tests/test_views.py

rest_framework/views.py

tomchristie · 2022-08-31T10:48:44Z

How would we like the documentation for this to look? Should we add a new section to the "Views"?
I'd really really only want to merge this if we could ensure we didn't have scope-creep. For example once we have async here, it potentially makes sense to have async variants of the generic views etc. Perhaps we'd want to have basic support built in, but with a third party package for anything more.
I find the test cases a bit confusing. Do async API views work okay with the regular APIRequestFactory? Does this differ from how Django's RequestFactory works with regular Django async views?

Archmonger · 2022-08-31T20:16:22Z

rest_framework/views.py

@@ -482,7 +482,7 @@ def raise_uncaught_exception(self, exc):
    # Note: Views are made CSRF exempt from within `as_view` as to prevent
    # accidental removal of this exemption in cases where `dispatch` needs to
    # be overridden.
-    def dispatch(self, request, *args, **kwargs):
+    def sync_dispatch(self, request, *args, **kwargs):


We might want to keep this function named dispatch to prevent people's overrides from breaking.

Which means the only reasonable name for the other function is probably adispatch.

Scratch that, overriding dispatch would still work for most people with the current implementation, but would definitely lead to some unexpected behavior if they're trying to upgrade to async.

Don't think there's anything we can do about that.

However, the comment above sync_dispatch may need to be updated now.

Archmonger · 2022-08-31T20:33:23Z

rest_framework/views.py

+        """
+        `.dispatch()` is pretty much the same as Django's regular dispatch,
+        but with extra hooks for startup, finalize, and exception handling.
+        """


Needs a docstring update

Archmonger · 2022-08-31T20:46:05Z

Docs

Yeah this probably makes the most sense as a Async Views section in the Views docs.

Scope creep

Since this solution was implemented within the class APIView(View), there shouldn't be any scope creep on anything that inherits from that.

The only thing I can think of is the current solution doesn't think about DRF function views. Based on the way rest_framework.decorators.api_view is implemented, this current solution might just automatically work? New tests to cover async function views are required regardless.

em1208 · 2022-09-01T00:24:25Z

I updated the docstring and added support for async function based views.

em1208 · 2022-09-01T01:04:31Z

I also added the documentation under Views.

dongyuzheng · 2022-09-01T01:06:55Z

rest_framework/compat.py

+if django.VERSION >= (4, 1):
+    from asgiref.sync import async_to_sync
+else:
+    async_to_sync = None


How about

def async_to_sync(*args, **kwargs): raise NotImplementedError("DRF async only supports Django >= 4.1")

A type checker would not be happy about async_to_sync = None.

async_to_sync is available in Django since version 3.1. In this case I'm using it just to run the test. If pytest-asyncio is added then it can be removed.

I would personally rather we take a testing dependency on pytest-asyncio rather than implement a test-only compat helper.

I believe it's best to defer the decision to add an additional dependency to project leads. Especially if the new dependency can be easily circumvented, such as in this case.

As an a user of drf I would not happy to have any async tests related dependency in my project

rest_framework/decorators.py

dongyuzheng · 2022-09-01T01:30:38Z

tests/test_views.py

+
+    def test_get_succeeds(self):
+        request = factory.get('/')
+        response = async_to_sync(self.view)(request)


Should this pytest ext be added?

https://pypi.org/project/pytest-asyncio/

I'd expect more async code be added to this repo in the future.

I'll be happy to add it if there is an agreement instead of using async_to_sync

I'm personally indifferent. I tend to avoid additional dependencies when possible, so I personally support using asgiref.async_to_sync instead of pytest-asyncio.

I don't expect this repo to have much more in terms of asyncio related tests beyond this, since we're already covering Django's CBV and FBV.

Unless Tom Christie feels otherwise, I'd say it's safe to stay as-is.

tests/test_views.py

docs/api-guide/views.md

dongyuzheng · 2022-09-01T01:37:20Z

docs/api-guide/views.md

+    class ListUsers(APIView):
+        async def get(self, request):
+            return Response({"message": "This is an async class based view."})
+
+
+    @api_view(['GET'])
+    async def view(request):
+        return Response({"message": "This is an async function based view."})


wrap in

```python
code
```

I'm using the same format as the rest of the documentation.

Agreed with em1208. Current formatting should be prioritized for legacy repos, and changed later via refactoring if project owners decide.

tests/test_views.py

dongyuzheng · 2022-09-19T00:30:55Z

@em1208, great work on the fast fix.

Regarding issue #8657 this is expected since ORM queries need to be wrapped by the sync_to_async function or need to use the new async API of the ORM.

This might be a problem, because returning serializer.data should be a common use case. Wrapping sync_to_async will make all of these async APIs a bit worse.

I once worked in a codebase where to add true async support, they had to copy paste everything and rewrite a v2 as async.

I am worried we will add too many if async; else all over the place

rest_framework/views.py

em1208 · 2022-09-19T11:27:34Z

@dongyuzheng thanks! Unfortunately regarding #8657 there is no other solution except the ones I mentioned. A complete async rewrite is definitely possible but if we want to keep using the same class APIView there will be many if async; else to handle both the sync and async cases.

Archmonger · 2022-09-19T21:34:58Z

For user APIs like serializer.data, we should follow Django's current conventions and create *.adata counterparts that are simple wrappers around *.data combined with database_sync_to_async.

Archmonger · 2022-09-19T21:40:42Z

rest_framework/views.py

+
+        if getattr(self, 'view_is_async', False):
+            async def handler():
+                return func()


Should use sync_to_async on func as an optimization. Unless we believe self.metadata_class().determine_metadata(request, self) does nothing beyond reading in-memory variables (haven't checked).

I checked the code and under some conditions self.metadata_class().determine_metadata(request, self) can make a database query, so I wrapped it with sync_to_async.

tomchristie · 2022-09-21T11:47:31Z

So, my honest perspective based on the above is that it's strengthened my position on not really wanting to add async support to REST framework. It looks to me like we'd end up pushing folks into making poor development choices(*), and it'll also personally make my life more tedious because we'll end up with a whole bunch of new kinds of support tickets, that I could really do without.

There aren't any technical reasons why a third party package couldn't fulfil this space. If folks really want async view support in REST framework, then that's a route I'd suggest.

(*) Tech for the sake of tech, rather than actually improving a clear business use-case / demonstrably improving some important metrics etc.etc.

Archmonger · 2022-09-22T03:22:55Z

The main benefit of async comes from IO-bound scenarios. For example, these benchmarks show that fully async python web stacks are considerably faster thanks to the ability for python to execute during HTTP transmit. The amount of time Python views take to execute is generally an order of magnitude faster than any IO operation.

For example, a large portion of DRF endpoints utilize database queries. The ability for the webserver to process other views while performing database queries and HTTP responses/receives is absolutely significant.

In my opinion, passing on integrated async support would be deferring something that DRF will eventually implement. Having an external async package will most likely cause more maintainability issues (more specifically, OSS developers going MIA) for something that will only become more important as Django core embraces async.

If you disagree, this PR can be closed.

TraderSamwise · 2022-09-25T02:52:35Z

I can only imagine the headache involved in handling async support on top of all the existing tickets regarding normal DRF. That being said, this is something my team absolutely needs due to being IO-bound on many of our endpoints (forwarding requests to other APIs etc). If the plan is to spin this off into a 3rd party package, then I suppose that is fine. But I really hope we can figure out a way to get quality async view support into DRF asap.

Really appreciate all the hard work here!!

tomchristie · 2022-09-26T11:53:45Z

This pull request shows exactly how to subclass APIView and create an AsyncAPIView.
Y'all can add this into your projects now, today, and immediately, without being blocked by REST framework at all. 🎉

Good steps for folks wanting to make that more easily accessible...

Create a gist with just this subclass, and a bit of example text, showing folks how to create an AsyncAPIView that they can use in their own projects.
Next step beyond that, is wrapping that up into a Python package.
And then expanding on that if wanted... eg. adding the decorator, or whatever other feature requests end up coming in.

It'd also be valuable to have someone demo an AsyncAPIView with an outgoing async HTTP request and actually benchmark if you're able to achieve higher throughput than the sync equivalent case.

TraderSamwise · 2022-09-26T16:59:23Z

@tomchristie we may attempt to integrate async support in an upcoming sprint using this PR. If so, I will try and get our team to put together a gist and / or other useful for others.

PureTryOut · 2022-11-02T16:04:41Z

I fail to see how this pull request shows how to create an AsyncAPIView? Do you mean the README with this:

class AsyncView(APIView):
    async def get(self, request):
        return Response({"message": "This is an async class based view."})

If so, that won't work without the other changes in this PR. This minimal view is enough to make DRF call into the ORM which doesn't like being called synchronously in an async context, and it errors out. I might be (and probably am) missing something obvious, but so far I see no way to use DRF in an async way. I much rather use built-in async support than having to use yet another dependency which we have to keep up-to-date and verify for stability and security, but if there is no other option we'll (have to) resort to it.

With the comments made by you here @tomchristie, can I assume this PR won't be accepted and there will be no native async support in DRF?

tomchristie · 2022-11-08T12:52:16Z

@em1208 Your PR on this is a really great starting point.

As I see it, the next step is taking that work, and making it more accessible to users to start trying out.

I'm wondering if you (or anyone else) would be interested in adapting it to into a repository that exposes an AsyncAPIView class and @async_api_view decorator?

This wouldn't be clone of REST framework, but would instead just subclass APIView...

class AsyncAPIView(rest_framework.APIView):
    # The code changes demonstrated in this pull request

Together with a nicely put together README, this would give us something easily installable for users to start working with, and giving feedback on. That is our step 0 here.

I'm happy to collaborate with anyone who wants to work on this if it's unclear.

em1208 · 2022-11-08T16:35:55Z

@tomchristie sure, I will be happy to work on it. I will post here again once ready.

em1208 · 2022-11-26T20:18:52Z

I just released a separate package to implement the async views support for Django REST framework following the approach of this PR. Here is the link to the code: https://github.com/em1208/adrf. The package has been published in PyPI as well: https://pypi.org/project/adrf/.

There are already two other packages in PyPI that aim to add async support to Django REST Framework: django-rest-framework-async and async-drf. Both wraps Django REST framework code with sync_to_async where needed to allow async views. While using sync_to_async works too, it is not as performant as fully async code, which is the aim of adrf.

Any feedback would be highly appreciated!

auvipy · 2022-11-27T10:28:17Z

I just released a separate package to implement the async views support for Django REST framework following the approach of this PR. Here is the link to the code: https://github.com/em1208/adrf. The package has been published in PyPI as well: https://pypi.org/project/adrf/.

There are already two other packages in PyPI that aim to add async support to Django REST Framework: django-rest-framework-async and async-drf. Both wraps Django REST framework code with sync_to_async where needed to allow async views. While using sync_to_async works too, it is not as performant as fully async code, which is the aim of adrf.

Any feedback would be highly appreciated!

that is great. IMHO, async support in django is not yet a first class/complete from developers and technical points of view. so we should still wait for further django releases and try async drf as a 3rd party extension for some time.

tomchristie · 2022-12-02T08:59:28Z

Next step would be to add this to the docs.

Adding "Third Party Packages" section to https://www.django-rest-framework.org/api-guide/views/ would probably make sense, right?

auvipy · 2022-12-02T09:47:52Z

Next step would be to add this to the docs.

Adding "Third Party Packages" section to https://www.django-rest-framework.org/api-guide/views/ would probably make sense, right?

I do agree with this

smittysmee · 2023-02-20T18:48:42Z

Bump

auvipy · 2023-02-27T10:56:45Z

Do we expect this to be merged anytime soon, or will adrf be the way to go as for now?

adrf will be the way to go for now

kwood · 2023-03-28T04:44:53Z

Hi — I'm trying to understand how practical using something like adrf is at the moment. From what I understand, it provides only a base ViewSet that supports async. I don't think the more powerful features of DRF (e.g., ModelViewSet) are supported yet — am I understanding that correctly?

On the surface it seems like supporting ModelViewSet would require changes to a lot of downsteam components of the DRF since all the "magic" of DRF happens in the mixins and backends, and there are a lot of sync assumptions being made in there.

I wonder if someone who has investigated this more deeply has thoughts — please correct me if I'm wrong.

jameshilliard · 2023-05-10T16:09:43Z

There aren't any technical reasons why a third party package couldn't fulfil this space. If folks really want async view support in REST framework, then that's a route I'd suggest.

So I looked into this a bit and this doesn't really seem like a feasible path forward from what I can tell, there's a lot of plumbing needed to make async work properly in a way that allows for incremental migrations of projects to asyncio drf views. Additionally a 3rd party package would likely introduce dependency hell issues in larger projects.

The issue here is that async code needs to be cross compatible with sync code in a number of places that isn't all that feasible to handle in a 3rd party package as one may be mixing sync and async 3rd party packages in various ways.

From what I was seeing the best approach is to plumb the async view code into drf in a way that makes mixing async/sync code during a migration is feasible.

I made an initial attempt at this in #8978 which also handles async/sync bidirectional cross compatibility for permissions and throttles for sync and async views.

On the surface it seems like supporting ModelViewSet would require changes to a lot of downsteam components of the DRF since all the "magic" of DRF happens in the mixins and backends, and there are a lot of sync assumptions being made in there.

I think my granular autodetecting approach is what is needed to make it possible to mix and/or migrate these sync components with/to async.

Tentative async implementation

79ce07b

em1208 mentioned this pull request Aug 19, 2022

Async Support #8496

Closed

enrico added 3 commits August 19, 2022 18:01

Fixed post unit tests

67ebe90

Fixed tests for all the environments

7d2b73a

Moved compatibility code into compat.py

8fa1b7c

enrico added 2 commits August 25, 2022 07:41

Update to support async dispatch in AsyncAPIView

daac20a

Removed separate AsyncAPIView

0a40010

Archmonger reviewed Aug 25, 2022

View reviewed changes

rest_framework/views.py Outdated Show resolved Hide resolved

Removed redundant check

ae84d97

tomchristie reviewed Aug 30, 2022

View reviewed changes

tests/test_views.py Show resolved Hide resolved

amin-basiri approved these changes Aug 30, 2022

View reviewed changes

sstoops reviewed Aug 30, 2022

View reviewed changes

rest_framework/views.py Outdated Show resolved Hide resolved

tomchristie mentioned this pull request Aug 31, 2022

Version 3.14.0 proposal #8599

Merged

Archmonger reviewed Aug 31, 2022

View reviewed changes

Updated docstring and added support for async function based views

351ea7d

enrico added 2 commits September 1, 2022 09:02

Added documentation

0400834

Updated docs

5e3140a

dongyuzheng reviewed Sep 1, 2022

View reviewed changes

rest_framework/decorators.py Outdated Show resolved Hide resolved

Added missing bracket in docs

472ca5b

dongyuzheng reviewed Sep 1, 2022

View reviewed changes

tests/test_views.py Show resolved Hide resolved

dongyuzheng reviewed Sep 1, 2022

View reviewed changes

docs/api-guide/views.md Outdated Show resolved Hide resolved

dongyuzheng reviewed Sep 1, 2022

View reviewed changes

dongyuzheng reviewed Sep 19, 2022

View reviewed changes

tests/test_views.py Show resolved Hide resolved

dongyuzheng reviewed Sep 19, 2022

View reviewed changes

tests/test_views.py Show resolved Hide resolved

Archmonger reviewed Sep 19, 2022

View reviewed changes

rest_framework/views.py Outdated Show resolved Hide resolved

Fix for missing await and using getattr instead of hasattr

ec1d754

Archmonger reviewed Sep 19, 2022

View reviewed changes

Wrapped func with sync_to_async

de6df46

sarayourfriend mentioned this pull request Nov 10, 2022

Move off of grequests WordPress/openverse-api#486

Closed

1 task

Archmonger mentioned this pull request Nov 26, 2022

Handle review comments from DRF em1208/adrf#1

Closed

auvipy added the Needs design decision label Nov 27, 2022

jameshilliard mentioned this pull request May 10, 2023

Async view implementation #8978

Closed

auvipy closed this May 11, 2023

Async implementation #8617

Async implementation #8617

Conversation

em1208 commented Aug 19, 2022 • edited Loading

Description

tomchristie commented Aug 22, 2022

tomchristie commented Aug 31, 2022

Choose a reason for hiding this comment

Archmonger Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Archmonger commented Aug 31, 2022 • edited Loading

Docs

Scope creep

em1208 commented Sep 1, 2022

em1208 commented Sep 1, 2022

Choose a reason for hiding this comment

dongyuzheng Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongyuzheng Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Archmonger Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

dongyuzheng commented Sep 19, 2022 • edited Loading

em1208 commented Sep 19, 2022

Archmonger commented Sep 19, 2022

Archmonger Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomchristie commented Sep 21, 2022

Archmonger commented Sep 22, 2022 • edited Loading

TraderSamwise commented Sep 25, 2022

tomchristie commented Sep 26, 2022

TraderSamwise commented Sep 26, 2022

PureTryOut commented Nov 2, 2022

tomchristie commented Nov 8, 2022

em1208 commented Nov 8, 2022

em1208 commented Nov 26, 2022

auvipy commented Nov 27, 2022

tomchristie commented Dec 2, 2022

auvipy commented Dec 2, 2022

smittysmee commented Feb 20, 2023

auvipy commented Feb 27, 2023

kwood commented Mar 28, 2023

jameshilliard commented May 10, 2023 • edited Loading

em1208 commented Aug 19, 2022 •

edited

Loading

Archmonger Aug 31, 2022 •

edited

Loading

Archmonger commented Aug 31, 2022 •

edited

Loading

dongyuzheng Sep 1, 2022 •

edited

Loading

dongyuzheng Sep 1, 2022 •

edited

Loading

Archmonger Sep 2, 2022 •

edited

Loading

dongyuzheng commented Sep 19, 2022 •

edited

Loading

Archmonger Sep 19, 2022 •

edited

Loading

Archmonger commented Sep 22, 2022 •

edited

Loading

jameshilliard commented May 10, 2023 •

edited

Loading