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

Async Support #8496

Closed
robd003 opened this issue May 22, 2022 · 37 comments
Closed

Async Support #8496

robd003 opened this issue May 22, 2022 · 37 comments
Assignees

Comments

@robd003
Copy link

robd003 commented May 22, 2022

Django 4.1 alpha1 was just released and now supports async ClassBasedViews

Could we get DRF APIViews to support async since it's already available in the master branch of Django?

@stale
Copy link

stale bot commented Jul 31, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 31, 2022
@sun337
Copy link

sun337 commented Aug 5, 2022

This is much needed.

@stale stale bot removed the stale label Aug 5, 2022
@TheWeirdDev
Copy link

Are there any plans to implement this?

@PaoloC68
Copy link

PaoloC68 commented Aug 5, 2022

well, honestly I cannot imagine has the whole DRF can be future-proof without adding a fully fledged async support.

@robd003
Copy link
Author

robd003 commented Aug 5, 2022

@tomchristie any chance this could get put on the roadmap?

@firminoneto11
Copy link

Up

@hguitar
Copy link

hguitar commented Aug 10, 2022

DRF async support would much appreciated!

@helenap
Copy link

helenap commented Aug 16, 2022

I have an API that depends on a third party https call and this will really make my life better, when can this be out?

@em1208
Copy link

em1208 commented Aug 18, 2022

@tomchristie I would like to work on this!

@tomchristie
Copy link
Member

Noted. I expect that a third-party package implementing an AsyncAPIView would be a good start here.

Has anyone attempted an implementation of an equivalent to the existing APIView? What blockers are there to being able to implement one? What particular support is needed from the maintenance team here?

@em1208
Copy link

em1208 commented Aug 19, 2022

I haven't tried before but I would like to start with a test implementation. I will report any blockers. When would you like to review a PR for DRF?

@em1208
Copy link

em1208 commented Aug 19, 2022

I opened this PR #8617 with a tentative implementation of the async support.

@robd003
Copy link
Author

robd003 commented Aug 20, 2022

You rock @em1208

@tomchristie
Copy link
Member

Okay, so @em1208 has a neatly done pull request which adds async view support.

My concerns around this are to do with introducing false expectations to our users. I'd rather explore that through conversation instead of dictating, so here goes...

Could somebody start by giving an example of what you want this support for and why?

@em1208
Copy link

em1208 commented Aug 22, 2022

As an extension of my PR, I think it would be good to have separate async mixins using the new async API of the queryset and it should be enough for most users.

@firminoneto11
Copy link

With async support from DRF users would be able to fully go async for their apis, allowing them to use awsome tools such as httpx, async db drivers in the future and also use the new queryset api that Django's team is working on, without much effort, by just creating a regular async DRF view.

I also think that going full async with new DRF's views could improve even more concurrency on apis, because we would be able to write non blocking code, which is amazing!

@tomchristie
Copy link
Member

Okay, so I'll address a couple of concerns based on those comments...

With async support from DRF users would be able to fully go async for their apis

Okay. Though the pull request as it stands adds "async within a thread" support, rather than "async all-the-way-through support".

I think the dispatch method would need to return an optionally-awaitable instance in order to properly support a fully async calling stack, rather than using sync_to_async.

(At least, that's what I understood based on a look at the dispatch view of Django's View class. Anyone?)

I think it would be good to have seperate async mixins [...]

Agreed. If async support is being added, it'd make sense for it to extend into the mixins. But... I'm not keen on REST framework taking on new functionality.

Anyways, my preference at the moment is the same as with almost every other REST framework proposal. Which is: A third party package would probably be a perfectly okay place for this functionality to be supported in.

@em1208
Copy link

em1208 commented Aug 23, 2022

I can update the PR to make it ""async all-the-way" if it would make it acceptable, I tried to minimise the impact on the existing codebase. I understand the preference is for a third party package but I still think the new async functionality of Django could be nicely supported by DRF together with the existing functionality.

@tomchristie
Copy link
Member

I can update the PR to make it ""async all-the-way" if it would make it acceptable

It'd certainly be interesting to know what that'd look like. (It's still very likely I'd prefer to see work there in a third party package, but.)

The sync-to-async wrapper makes it really nice and easy, but we're also essentially indulging in false advertising. Async in that case isn't going to improve overall request/response concurrency at all, and really only potentially helps in exceedingly narrow use-cases such as "we need a view that makes two outgoing HTTP requests in parallel".

@em1208
Copy link

em1208 commented Aug 24, 2022

Thanks @tomchristie for taking this into consideration. I updated the PR #8617 to have an async dispatch method.

@Archmonger
Copy link

Archmonger commented Aug 25, 2022

I would disagree with an AsyncAPIView class. The best approach probably involves mimicking what Django Core is doing.

That is, dynamically deciding whether to dispatch as async or sync based on whether all HTTP method functions are async.

For backwards compatibility purposes, we could add an additional check that tosses an exception if trying to use async on django<4.1

@em1208
Copy link

em1208 commented Aug 25, 2022

@Archmonger thanks for your comment. I removed the AsyncAPIView and done all the changes under APIView .

@johnthagen
Copy link
Contributor

Could somebody start by giving an example of what you want this support for and why?

So that it doesn't get lost, there is a lot of historical use cases people have provided over at:

@tomchristie
Copy link
Member

tomchristie commented Nov 30, 2022

So https://github.com/em1208/adrf is looking great.
I'm wondering where the best place to link to this from the docs would be?
Perhaps an "Async Views" section in https://www.django-rest-framework.org/api-guide/views/ noting that support for those is provided by this third party package?

@smittysmee
Copy link

What is the status of this?

@tomchristie
Copy link
Member

I'd still agree with my comment above... I'd suggest that someone opens a pull request adding adrf to the documentation, as this will help improve its visibility.

@johnthagen
Copy link
Contributor

I'm curious the reason not to pull adrf fully into DRF if it is the solution?

@tomchristie
Copy link
Member

I'm curious the reason not to pull adrf fully into DRF if it is the solution?

If we're going to add complexity to a codebase, then we need to be confident that it's worth the trade-off.
Dealing with that as a third party package is a more flexible way to explore the design space, without committing ourselves to it.

@errrken
Copy link

errrken commented Feb 26, 2023

I can update the PR to make it ""async all-the-way" if it would make it acceptable

It'd certainly be interesting to know what that'd look like. (It's still very likely I'd prefer to see work there in a third party package, but.)

The sync-to-async wrapper makes it really nice and easy, but we're also essentially indulging in false advertising. Async in that case isn't going to improve overall request/response concurrency at all, and really only potentially helps in exceedingly narrow use-cases such as "we need a view that makes two outgoing HTTP requests in parallel".

@tomchristie Could you please elaborate on why it would only potentially improve concurrency when running multiple outgoing long-running HTTP requests? I've seen you really try to underline that you do not want to offer any false expectations and false marketing here in this thread (which makes sense!), but in this particular scenario I am curious if you could explain more? This is purely the only reason I want to add async behaviour to some(!) of our views

@HMaker
Copy link

HMaker commented Apr 7, 2023

I can update the PR to make it ""async all-the-way" if it would make it acceptable

It'd certainly be interesting to know what that'd look like. (It's still very likely I'd prefer to see work there in a third party package, but.)
The sync-to-async wrapper makes it really nice and easy, but we're also essentially indulging in false advertising. Async in that case isn't going to improve overall request/response concurrency at all, and really only potentially helps in exceedingly narrow use-cases such as "we need a view that makes two outgoing HTTP requests in parallel".

@tomchristie Could you please elaborate on why it would only potentially improve concurrency when running multiple outgoing long-running HTTP requests? I've seen you really try to underline that you do not want to offer any false expectations and false marketing here in this thread (which makes sense!), but in this particular scenario I am curious if you could explain more? This is purely the only reason I want to add async behaviour to some(!) of our views

Async IO doesn't necessarily speeds up your code, it just reduces the server resources usage for concurrent tasks by leveraging concurrent network IO in a single-threaded application.

@stale
Copy link

stale bot commented Jun 10, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 10, 2023
@medihack
Copy link

Bump. I think it would be nice to leave this issue open as long as adrf is evaluated (and maybe will be integrated in DRF itself).

@stale stale bot removed the stale label Jun 29, 2023
@PythonCoderAS
Copy link

Along with Django 4.2, ADRF is missing async Serializer support, and now that the ORM supports async it should be easier to implement.

@ColemanDunn
Copy link

Along with Django 4.2, ADRF is missing async Serializer support, and now that the ORM supports async it should be easier to implement.

It seems like async serializers were added in this commit about a month before you commented no?

@PythonCoderAS
Copy link

I didn't see it in the latest stable version so I assumed it wasn't done yet, my bad.

@auvipy auvipy self-assigned this Nov 25, 2023
@auvipy
Copy link
Member

auvipy commented Nov 25, 2023

this functionalities will be handled with extensions.

@tomchristie
Copy link
Member

Closing this issue now. Perfectly happy to recommend adrf in the same way as any of our other third party extensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests