Re: Sync and Async versions of the same function: guidelines for contributors

[Jon Janzen]

Hey there,

> Not that much with a 50 lines function. that could easily become a 
maintenance burden.
> what do you think about code duplication of async function ? (please 
point me to existing threads if the discussion already occurred)

I flagged this problem 
in 
https://forum.djangoproject.com/t/asyncifying-django-contrib-auth-and-signals-and-maybe-sessions/18770
and before that 
in https://groups.google.com/g/django-developers/c/T8zBnYO78YQ

There doesn't really seem to be a good resolution to this problem right 
now, unfortunately :(

> why do we see almost no sync wrapper (async_to_sync) in django's code 
base ? Is that a best practice ?

I can't give an authoritative answer but my intuition is that we are still 
in the early stages of the process of asyncifying everything and we're at 
the second bullet point from DEP-009 ("Sync-native, with an async wrapper") 
and haven't yet gotten started on an async-native implementation with a 
sync wrapper.

There are a few key things missing from core django before that's possible. 
Some of which are laid out in that forum thread I linked above, others are 
listed directly in the DEP like the ORM being fully asyncified (right now 
it's just an async wrapper around sync code) which is somewhat blocked on 
async database implementations (psycopg3 being the first one supported).

I don't really have any answers for you, just some more thoughts. Hope 
that's helpful!

Jon
On Saturday, August 5, 2023 at 10:23:00 AM UTC-7 Olivier Tabone wrote:

> Hi all,
>
> While working on async related tickets (eg #34717, and more recently 
> #34757) I noticed code duplication between sync and async version of same 
> functions:
>
> some examples: (no personal offense ❤️)
>
>
>- acheck_password /  check_password in base_user.py 
>
> 
>- get_many() / a_getmany() in cache/backends/base.py 
>
> 
>
> and of course the way I fixed #34717, now we have some code duplication in 
> aget_object_or_404 
> 
>  
> / aget_list_or_404 
> 
>
> As I'm working on #34757, and following this pattern, there would be some 
> duplication of the TestClient._handle_redirects 
> 
>  
> method to support the async case.
>
> I'm kind of ok when duplicating a 3 lines function. Not that much with a 
> 50 lines function. that could easily become a maintenance burden.
>
> I've read DEP-009  
> a few 
> times and the plan at the time was (quoted from the "Technical Overview")
>
> Each feature will go through three stages of implementation:
>
>- Sync-only (where it is today)
>- Sync-native, with an async wrapper
>- Async-native, with a sync wrapper
>
>
> I was wondering:
> 1- why do we see almost no sync wrapper (async_to_sync) in django's code 
> base ? Is that a best practice ?
> 2- what do you think about code duplication of async function ? (please 
> point me to existing threads if the discussion already occurred) What is Ok 
> / What is not ok ? Is there some cleanup to be done ?
>
>
> Cheers,
>
> - Olivier Tabone
>
>
>
>
>
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/be3a9dba-2702-4fbd-baef-c49587892616n%40googlegroups.com.

Re: Async wrappers in contrib packages

[Jon Janzen]

Hey Carlton,

Sorry I skipped over the suggestion in one of your earlier messages to post on 
the Forum.

I’ve done so now: 
https://forum.djangoproject.com/t/asyncifying-django-contrib-auth-and-signals-and-maybe-sessions/18770

I think we can consider this thread deprecated in favor of the above Forum 
thread.

Cheers,

Jon

> On Feb 10, 2023, at 03:54, Carlton Gibson  wrote:
> 
> No plan ever survives first contact with the enemy :) but, this seems 
> reasonable to me Jon, yes. 
> 
> Can I suggest we move further discussion on details to the Forum's async 
> category? It's nicer, better contained, and we more likely to catch 
> interested eyes. 
> 
> Thanks for pursuing this! 
> 
> On Thu, 9 Feb 2023 at 23:38, Jon Janzen  <mailto:j...@jonjanzen.com>> wrote:
>> Hey Carlton, 
>> 
>> Thanks for your thoughtful comments, a few things come to mind:
>> 
>> A. It sounds like we’re in agreement about the utility and severability of 
>> Phase 1 (just creating an async_to_sync-based wrapper around the auth 
>> interface). I want to make sure I don’t cause extra work on the bug tracker, 
>> so does this sound like it’s ready to be filed as a ticket there? I can file 
>> the ticket and get started on writing tests/docs if so!
>> 
>> B. In reply to your "Is there a best order here, or can we just chip away 
>> bit-by-bit? 樂” comment, I think there is a reasonable order: we mirror the 
>> dependencies between django internals. By that I mean I think we should 
>> asyncify the necessary parts of sessions or signals before asyncifying the 
>> parts of auth that depend on them. That reduces the need to insert 
>> “temporary” sync/async boundaries and reduces churn.
>> 
>> C. Based on your responses and my own thoughts in (2) I think it’s 
>> reasonable to revise the overall order into this:
>> 
>> 1. In any order: asyncify the auth API (Phase 1), asyncify signals (ticket 
>> 32172), and asyncify sessions (TODO: discussion/ticket)
>> 2. After all 3 parts to (1), asyncify auth backends and internals of the 
>> auth API (Phase 2)
>> 3. Resolve ticket 31920
>> 4. Asyncify the auth middleware (Phase 3)
>> 
>> D. I’m admittedly very new to Django and contributing code to it, but Phase 
>> 1 is certainly within my area of expertise considering the 2 PRs that I have 
>> contributed :
>> 
>> https://github.com/django/django/pull/16256
>> https://github.com/django/django/pull/16252
>> 
>> After that, I’d be interested in helping nudge this along over time. I’d 
>> like to build my experience beyond just adding async wrappers , so maybe I 
>> can pick up https://github.com/django/django/pull/13651 and hack away at 
>> your idea about categorizing during registration. If that all goes well I 
>> can start a discussion around asyncifying sessions (which, on first glance 
>> seems like a LARGE project of its own)
>> 
>> Thanks,
>> 
>> Jon
>> 
>> 
>>> On Feb 7, 2023, at 09:25, Carlton Gibson >> <mailto:carlton.gib...@gmail.com>> wrote:
>>> 
>>> Hi Jon. 
>>> 
>>> Thanks for this. 
>>> 
>>> I think your use-case is reasonable, and that you're basically on the right 
>>> track. 
>>> If you were to add test cases to your PoC, there's certainly a case for 
>>> looking seriously at it. 
>>> It should be reasonable to keep pushing the interfaces down one layer at a 
>>> time. (See comment on Sessions below.) 
>>> 
>>> I'm not sure a priori how Phase 2 plays out in advance. It's likely 
>>> something we have to think about as we go. 
>>> Targeted posts to the Async category of the Forum might help drive 
>>> discussion. 
>>> https://forum.djangoproject.com/c/users/async-channels/23
>>> 
>>> Let me inline some quick thoughts on your …
>>> 
>>> ## Open Questions
>>> 
>>> Based on my stab at the above PoC implementation I came away with several 
>>> questions:
>>> 
>>> 1. Are there proven strategies for reducing code duplication between sync 
>>> and async versions of functionality in Django or in Python broadly? I’m not 
>>> aware of any guidance here but I’m eager for resources to consider. The 
>>> implementation is verbose (doubles file size in some cases) and fragile 
>>> (what if a bug fix is only applied to the sync version and not the async 
>>> version?) right now.
>>> 
>>> Yes… you end up writing it twice... 樂 Maybe in some ideal end state we end 
>>> up with an async core with a thi

Re: Async wrappers in contrib packages

[Jon Janzen]

vering 
> this topic (just some tangential discussion in 
> https://groups.google.com/u/1/g/django-developers/c/D2Cm25yFzdI/m/bo_Ae_kgBQAJ),
>  perhaps it would be a good idea for me to dig into this area first and think 
> about asyncifying `sessions` instead of sprinkling lots of `sync_to_async` 
> calls around `sessions` callsites in `auth`? I’d appreciate some other 
> opinions/guidance here.
> 
> So, rinse and repeat no? — We should be able to the session backend API an 
> async interface, and so on. 
> Is there a best order here, or can we just chip away bit-by-bit? 樂
> 
> 4. I’ve intentionally not considered the auth decorators as that seems like 
> an orthogonal issue while https://code.djangoproject.com/ticket/31949 is 
> pending, am I missing something here? Do I need to consider decorators in 
> this proposal?
> 
> I don't think they're dependent. 
> 
> 
> Kind Regards,
> 
> Carlton
> 
> On Mon, 6 Feb 2023 at 03:22, Jon Janzen  <mailto:j...@jonjanzen.com>> wrote:
>> Hey,
>> 
>> Sorry about the delay in my response, holidays came early and stayed late 
>> for me this year.
>> 
>>> TBH I'd prefer it if you pondered the design here without opening a ticket 
>>> until such a point (if ever) that you have a concrete plan. (We'd likely 
>>> just close it as wontfix unless there's a specific idea on the table 
>>> anyway, so it's just noise at that point.) 
>> 
>> Understood, sorry for my ignorance on the process. I appreciate your 
>> patience!
>> 
>>>>> There's: https://code.djangoproject.com/ticket/31949 "Allow builtin view 
>>>>> decorators to be applied directly to async views."
>>>>> I think this is likely the next step.
>>>>> 
>>>>> There's a PR for that, which I think took a too complex approach (see 
>>>>> discussion). A simpler (more inline) take be good to see. 
>>>> 
>>>> Thanks, I saw this ticket but it didn't look relevant when I was skimming 
>>>> the tracker. I'll take a closer look.
>> 
>> Replying to myself here, I took a look at this ticket and associated PRs and 
>> that’s not quite do what I’m looking for (even if all the various 
>> constituent parts got merged) but the changes to the `auth` decorators are 
>> related.
>> 
>> I’m interested in an async interface of the auth app itself, i.e. the 
>> functionality exposed in `django/contrib/auth/__init__.py`.
>> 
>> (the next few paragraphs are background info on my personal investment in 
>> this, feel free to skip to the section marked “Proposal")
>> 
>> For some background on my interest in this: I’m running Django as an asgi 
>> app and all codepaths down to the django framework boundary are async. That 
>> means all my middleware, views, and ORM usage are all using the async 
>> versions, where applicable/possible. There are a number of reasons for this, 
>> but the most notable are simplicity (easier to reason about code if it is 
>> all either sync or async) and efficiency (most of the code is GraphQL 
>> resolvers which are more efficient to execute concurrently).
>> 
>> Also important to know that I almost exclusively use django as an API 
>> server, there is only one template for all “views” which just loads a 
>> javascript webpack bundle and renders using React, which then fetches data 
>> from the server using GraphQL. Effectively, that means I‘m calling the 
>> django auth APIs directly instead of using the default `LoginView` or 
>> `login_required` or anything like that.
>> 
>> Anyway, right now almost all of the sync/async boundaries are “invisible” to 
>> me in that they are inside Django. I’ve replaced all my own wrappers around 
>> the ORM’s synchronous-only methods with the `a`-prefixed methods provided 
>> over the last few Django releases (and that will be provided in 4.2!). So 
>> `aget` instead of `get`, `acreate` instead of `create` and so forth.
>> 
>> One area of async/sync boundaries that is currently prevalent in my codebase 
>> is my `sync_to_async` wrappers around `django.contrib.auth` methods. I’d 
>> like to push those down into Django, and then as deep into Django as is 
>> expedient right now.
>> 
>> # Proposal
>> Add asynchronous versions of the auth app’s API (i.e. `__init__.py`), then 
>> allow backends to have async-native versions and use that from the public 
>> API, and finally update the provided middleware (and maybe decorators) to 
>> use async-native functionality if they are running in ASGI mode.
>> 
>> 
>> 

Re: Async wrappers in contrib packages

[Jon Janzen]

terribly much work. Oh and to support `ModelBackend` being 
async-native `BaseUserModel` would need an async version of 
`get_by_natural_key`, but the implementation there is trivial.


## Part 3: async middleware

Once we have an async-native interface and backend support, I think it is 
natural to move the async boundary “upwards" to the middlewares. There are 2 
middleware classes that currently have synthesized async versions of their 
implementation that could be switched to async-native:

* `AuthenticationMiddleware`
* `RemoteUserMiddleware`

These could be updated to be async-native, though the actual implementation 
here is a bit tricky and requires resolution of this ticket: 
https://code.djangoproject.com/ticket/31920

I thin this is currently out-of-scope for my proposal due to the unresolved 
ticket/PR there, but I think it warrants consideration so we don’t write 
ourselves into a corner.


## Naive Implementation
I’ve taken the time to rough-out a rather simple implementation of the above 
plan: https://github.com/bigfootjon/django/pull/1 (note this is a PR from my 
fork _to_ my fork, not a PR against the main django GitHub repo)

This branch just exists to illustrate this proposal, not as code ready for 
checkin. It’s missing tests and docs and… a well-considered implementation. 
There’s duplicated code all over the place and I didn’t even bother running the 
code to see if it works. I just wanted to get a feel for how this would work 
and though I might as well publish my hacked-up PoC to show my work.


## Open Questions

Based on my stab at the above PoC implementation I came away with several 
questions:

1. Are there proven strategies for reducing code duplication between sync and 
async versions of functionality in Django or in Python broadly? I’m not aware 
of any guidance here but I’m eager for resources to consider. The 
implementation is verbose (doubles file size in some cases) and fragile (what 
if a bug fix is only applied to the sync version and not the async version?) 
right now.

2. The auth app obviously fires several signals, and Django signals currently 
has a sync-only interface. I see there is an issue on the bug tracker 
(https://code.djangoproject.com/ticket/32172) but it is currently blocked for 
performance reasons. Is it fine to just add more sync_to_async wrappers here? 
(I did so in the above PoC implementation). Another idea is to just add a async 
wrapper around the `send` method and defer the rest of that ticket until the 
perf question can be resolved.

3. The sessions app seems to only have a synchronous interface, which would 
cause a lot of friction during Phase 2. I couldn’t find any tickets covering 
this topic (just some tangential discussion in 
https://groups.google.com/u/1/g/django-developers/c/D2Cm25yFzdI/m/bo_Ae_kgBQAJ),
 perhaps it would be a good idea for me to dig into this area first and think 
about asyncifying `sessions` instead of sprinkling lots of `sync_to_async` 
calls around `sessions` callsites in `auth`? I’d appreciate some other 
opinions/guidance here.

4. I’ve intentionally not considered the auth decorators as that seems like an 
orthogonal issue while https://code.djangoproject.com/ticket/31949 is pending, 
am I missing something here? Do I need to consider decorators in this proposal?

## Summary

I think without guidance my gut tells me that doing just Phase 1 would be 
“fine” and mirrors a lot of other work around the django codebase to add async 
_interfaces_ without pushing those down to natively-async _implementations_ 
(yet). Each phase in my proposal seems severable to me, so stopping after Phase 
1 seems acceptable.

I think after Phase 1 the next step would be analyzing and adding an async 
interface (and, hopefully, implementation) for `sessions` before turning to 
Phase 2/3.

I’m well aware I dug really deep into a rabbit hole without asking if I was 
digging in the right spot, so please let me know where I’ve gone wrong in my 
analysis/approach/etiquette. I’m not invested in this plan, just in the core 
idea of asyncifying more of django. If the idea itself is not acceptable then 
I’ll be a little disappointed but I can understand the need to minimize 
complexity in a project like Django, so I won’t be that disappointed.

Thanks for reading this novella,

Jon


> On Dec 2, 2022, at 03:39, Carlton Gibson  wrote:
> 
> > But I can file a ticket just to track this one?
> 
> TBH I'd prefer it if you pondered the design here without opening a ticket 
> until such a point (if ever) that you have a concrete plan. (We'd likely just 
> close it as wontfix unless there's a specific idea on the table anyway, so 
> it's just noise at that point.) 
> 
> I hope that makes sense. 
> 
> Kind Regards,
> 
> Carlton
> 
> On Mon, 28 Nov 2022 at 16:53, Jon Janzen  <mailto:j...@jonjanzen.com>> wrote:
>> Hey Carlton,
>> 
>>> There's: https://code.djangoproject.

Re: Async wrappers in contrib packages

[Jon Janzen]

Hey Carlton,

There's: https://code.djangoproject.com/ticket/31949 "Allow builtin view
> decorators to be applied directly to async views."
> I think this is likely the next step.
>
> There's a PR for that, which I think took a too complex approach (see
> discussion). A simpler (more inline) take be good to see.
>

Thanks, I saw this ticket but it didn't look relevant when I was skimming
the tracker. I'll take a closer look.

> My personal interest in this is about django.contrib.auth (login,
> authenticate, etc.)
>
> This was missing from the PR on #31949, so if you wanted to pick it up...
> 
>

I'll take a closer look, I think I might be able to do that :D


>
> (Not sure about the value of rewriting the built-in views though if that's
> what you're thinking of 樂)
>
> > ...and django.contrib.syndication (views.Feed)
>
> Not sure what you have in mind here. Perhaps explaining that in
> more detail would help?
> (I'm not immediately sure I see the benefit of async for the feed views?
> 樂)
>

Not for the view itself, but for individual fields that compose the Feed.

I want to define an async "item_categories" method when I subclass Feed due
to an async-only permissions system I have that is out-of-scope here, but
that isn't possible right now so I pre-compute each of these values and
pass in a composite object with the source object and item_categories
precomputed.

I would rather just declare an async function and let the framework figure
out how to resolve the values reasonably efficiently for me. I don't want
to pay the cost of async_to_sync for each item in the feed :/

I'm fine with setting this one aside, as I said I already have a
workaround. But I can file a ticket just to track this one?

 - Jon

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAP1wFiCp_y%3DVygQxmat-JVR2cr_LjGTKDyQ7rXtg3ERrzdfXXw%40mail.gmail.com.

Async wrappers in contrib packages

[Jon Janzen]

Hey everyone,

Sorry if I'm not following correct protocol on this or if this has already 
been discussed elsewhere, but is there any consensus about (or needed for) 
creating async versions of contrib packages?

My personal interest in this is about django.contrib.auth (login, 
authenticate, etc.) and django.contrib.syndication (views.Feed) but I would 
guess this sort of work would fall under a general policy.

I read DEP009 
 and 
didn't see any discussion of this topic, nor could I find any discussions 
on the ticket tracker (my skills using the tracker are limited). I could 
only find 2 files mentioning "async" in django/contrib/ (git grep "async" 
django/contrib | grep "\.py:"):

* django/contrib/contenttypes/fields.py (added by me recently 
,
 
a bug fix )
* django/contrib/staticfiles/handlers.py (added as part of standing up 
async support 

)

Has there been any consensus about this? If I'm interested in async 
versions of functions/features in contrib packages should I just file 
tickets, or is this something that might need a DEP first?

Again, sorry if I'm barking up the wrong tree. It's not my intention to 
waste anyone's time!

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/70e80f26-e1fa-4aa9-a3a8-0d7dc38752c3n%40googlegroups.com.