Async implementation by em1208 · Pull Request #8617 · encode/django-rest-framework (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation86 Commits18 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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)

piontas reacted with thumbs up emoji piontas reacted with hooray emoji maxmorlocke, pauloxnet, ginjiro1216, johnthagen, TheWeirdDev, PureTryOut, alexandresebrao, dkdndes, shanemcd, ColemanDunn, and 5 more reacted with heart emoji piontas reacted with rocket emoji

enrico added 3 commits

August 19, 2022 18:01

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

enrico added 2 commits

August 25, 2022 07:41

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?

@@ -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):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 515 to 518

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a docstring update

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

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.

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

enrico added 2 commits

September 1, 2022 09:02

I also added the documentation under Views.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 228 to 235

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."})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrap in

```python
code
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@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

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

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

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.

toburger, errrken, frleb, em1208, wh1te909, Sourc, firminoneto11, dongyuzheng, shahriar350, TraderSamwise, and 13 more reacted with thumbs up emoji

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!!

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.

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

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?

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

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

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!

tomchristie, pauloxnet, chadleong, alex-pobeditel-2004, Lukas-Schillinger, przlada, geeshta, kavdev, wh1te909, scastlara, and 11 more reacted with heart emoji earthpyy, Archmonger, NyanKiyoshi, ColemanDunn, dimyG, and aarroisi reacted with rocket emoji

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.

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

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.

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.