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 <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 thin sync wrapper around that, but we are a long > way from there: I'm not sure it bears thinking about. > > Part of the task on the decorators work is to try not to duplicate > everything. e.g. a process_request only middleware doesn't necessarily need > to consume the `get_response()` callable, so often times can make use of > `markcoroutinefunction()` rather than needed to duplicate the whole > middleware function. (And so on.) — But those patterns aren't 100% clear just > yet — we'll find them as we go through in this next phase. > > 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. > > So, yes... — my waiting-for-time-to-try-it-idea™ there is to sort signals > into two groups on registration and call any async ones in a group > concurrently, with only the single switch into the concurrent context. > **Something** along those lines should be possible... > > 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. > > 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 <j...@jonjanzen.com > <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. >> >> >> ## Part 1: Async API >> The “simple” part of this proposal is just to define a bunch of functions >> with “a” prefixes in `__init__.py` and use `sync_to_async` to call the >> synchronous versions as has been done to, for example, QuerySet. This would >> involve asynchronous versions of the following functions in >> `auth/__init__.py`: >> >> * `get_user` >> * `authenticate` >> * `login` >> * `update_session_auth_hash` >> * `logout` >> >> >> ## Part 2: async backends >> I think once that interface is defined it makes sense to try and make as >> much of this framework as natively async-compatible as possible. That would >> mean adding support for async auth backends and then connecting the async >> API methods (as enumerated above) to the async versions of the backend >> methods. >> >> This could mean that there would be no `sync_to_async` calls within the >> `auth` app itself (except for sessions and signals, see Open Questions >> below), and any sync/async boundaries would be “below” the `auth` app. For >> example, for `ModelBackend` and its children the sync/async boundary would >> be in the ORM layer, as `QuerySet` presents an async API that is currently >> just a wrapper around the sync internals. >> >> Allowing async backends would involve introducing the following async >> functions to `BaseBackend` that just call the sync versions by default, and >> can be overridden in child classes to have natively async versions: >> >> * `authenticate` >> * `get_user` >> * `get_user_permissions` >> * `get_group_permissions` >> * `get_all_permissions` >> * `has_perm` >> >> Then `ModelBackend` would be augmented to have async-native versions of the >> above. `RemoteUserBackend` would also need some tweaks to be async-native, >> but overall this isn’t 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 <carlton.gib...@gmail.com >>> <mailto:carlton.gib...@gmail.com>> 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 <j...@jonjanzen.com >>> <mailto:j...@jonjanzen.com>> wrote: >>>> 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 >>>> <mailto: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 >>>> >>>> <https://groups.google.com/d/msgid/django-developers/CAP1wFiCp_y%3DVygQxmat-JVR2cr_LjGTKDyQ7rXtg3ERrzdfXXw%40mail.gmail.com?utm_medium=email&utm_source=footer>. >>> >>> >>> -- >>> 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 >>> <mailto:django-developers+unsubscr...@googlegroups.com>. >>> To view this discussion on the web visit >>> https://groups.google.com/d/msgid/django-developers/CAJwKpyRp5OLrEB%3DRD4zONCsChZMcBSEpqyiSCmoR9XB83Y6w5w%40mail.gmail.com >>> >>> <https://groups.google.com/d/msgid/django-developers/CAJwKpyRp5OLrEB%3DRD4zONCsChZMcBSEpqyiSCmoR9XB83Y6w5w%40mail.gmail.com?utm_medium=email&utm_source=footer>. >> >> >> -- >> 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 >> <mailto:django-developers+unsubscr...@googlegroups.com>. >> To view this discussion on the web visit >> https://groups.google.com/d/msgid/django-developers/6CF0F34A-A116-481A-A0E3-6EB0E2BBA302%40jonjanzen.com >> >> <https://groups.google.com/d/msgid/django-developers/6CF0F34A-A116-481A-A0E3-6EB0E2BBA302%40jonjanzen.com?utm_medium=email&utm_source=footer>. > > > -- > 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 > <mailto:django-developers+unsubscr...@googlegroups.com>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/django-developers/CAJwKpyREQT-9tHaeyFMSB5dDiwienjK6r-1y1TgnpdME8gRXPg%40mail.gmail.com > > <https://groups.google.com/d/msgid/django-developers/CAJwKpyREQT-9tHaeyFMSB5dDiwienjK6r-1y1TgnpdME8gRXPg%40mail.gmail.com?utm_medium=email&utm_source=footer>. -- 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/3D848ECE-47E1-4C2D-A1B6-3A3F9008635F%40jonjanzen.com.
