[Django] #32076: Adding async methods to BaseCache
Django
-----------------------------------------------+------------------------
Reporter: Andrew Chen Wang | Owner: nobody
Type: New feature | Status: new
Component: Core (Cache system) | Version: 3.1
Severity: Normal | Keywords: cache
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------------+------------------------
I've recently created a new package for Redis and Django integration at
[https://github.com/Andrew-Chen-Wang/django-async-redis django-async-
redis]. I'd like to add the missing methods, e.g. `get_async` or
`set_async` to the BaseCache so people can get type hints or
autocompletion suggestions when using `django.core.cache.cache` with
async.
Additionally, in order to be compatible with the async methods, there
would need to be a new async method for closing cache connections/pools at
[https://github.com/django/django/blob/999cddd58d30469f3ee85278985313fdf528323d/django/core/cache/__init__.py#L116-L121
django.core.cache.backends.base]. I believe the only good solution to that
would be:
{{{
async def close_caches_async(**kwargs):
# Some caches -- python-memcached in particular -- need to do a
cleanup at the
# end of a request cycle. If not implemented in a particular backend
# cache.close is a no-op
for cache in caches.all():
await cache.close_async()
}}}
Please let me know if that is out of scope though. I also believe
implementing async methods for the current backends is also out of scope
of this ticket (and my time :P).
For reference, the Google Group Discussion:
[https://groups.google.com/forum/#!topic/django-developers/sGq5Bnc2JMg
here] and [https://groups.google.com/forum/#!topic/django-
developers/NX03LNahZPo here]
--
Ticket URL: <https://code.djangoproject.com/ticket/32076>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Andrew Chen Wang | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: cache | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Andrew Chen Wang):
* version: 3.1 => master
Old description:
New description:
I've recently created a new package for Redis and Django integration at
[https://github.com/Andrew-Chen-Wang/django-async-redis django-async-
redis]. I'd like to add the missing methods, e.g. `get_async` or
`set_async` to the BaseCache so people can get type hints or
autocompletion suggestions when using `django.core.cache.cache` with
async.
Additionally, in order to be compatible with the async methods, there
would need to be a new async method for closing cache connections/pools at
[https://github.com/django/django/blob/999cddd58d30469f3ee85278985313fdf528323d/django/core/cache/__init__.py#L116-L121
django.core.cache.backends.base]. I believe the only good solution to that
would be:
{{{
async def close_caches_async(**kwargs):
# Some caches -- python-memcached in particular -- need to do a
cleanup at the
# end of a request cycle. If not implemented in a particular backend
# cache.close is a no-op
for cache in caches.all():
await cache.close_async()
}}}
Please let me know if that is out of scope though. I also believe
implementing async methods for the current backends is also out of scope
of this ticket (and my time :P). Edit: on second thought, I will add the
methods to DummyCache as well.
For reference, the Google Group Discussion:
[https://groups.google.com/forum/#!topic/django-developers/sGq5Bnc2JMg
here] and [https://groups.google.com/forum/#!topic/django-
developers/NX03LNahZPo here]
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:1>
Django
Reporter: Andrew Chen Wang | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: cache | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Andrew Chen Wang):
* needs_tests: 0 => 1
Comment:
> Please let me know if that is out of scope though.
I think it's out of scope since Django signals are still synchronous I
believe...
I've also added test for the DummyCache by when inheriting BaseCache, I
just set up the methods like so:
{{{
async def set_async(self, *args, **kwargs):
return self.set(*args, **kwargs)
}}}
Let me know if I should just use the argument names themselves.
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:2>
Django
Reporter: Andrew Chen Wang | Owner: nobody
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution: needsinfo
Keywords: cache | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* cc: Andrew Godwin (added)
* status: new => closed
* resolution: => needsinfo
* needs_tests: 1 => 0
Comment:
It looks that your proposition doesn't match with accepted
[https://github.com/django/deps/blob/master/accepted/0009-async.rst#caching
DEP-9]:
> Default implementations of these that just call the existing API via
sync_to_async will be provided in BaseCache.
Moreover, I'm not sure if we want to release any partial solution without
a real async support in caching. DEP 9 suggests that a separate DEP is
required:
> Some features, like templating and cache backends, will need their own
separate DEPs and research to be fully async. This DEP mostly focuses on
the HTTP-middleware-view flow and the ORM.
Initially closing as needsinfo. Andrew, Can I ask for your opinion?
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:3>
Django
Reporter: Andrew Chen Wang | Owner: nobody
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution: needsinfo
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andrew Godwin):
My interpretation of DEP-9 is that we need to add default `_async` variant
methods to BaseCache that pass through to the synchronous versions - this
seems to be what the ticket is proposing, on its surface.
The cache closing problem is a bit larger, though - if we need to do more
work than just slapping async methods into the system that use
sync_to_async to make it all work properly, then I suspect we're going to
have to do this in a bit more of a formal process than just a single
ticket/PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:4>
Django
Reporter: Andrew Chen Wang | Owner: Andrew
| Chen Wang
Type: New feature | Status: assigned
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andrew Chen Wang):
> The cache closing problem is a bit larger, though
I wanted to put this in a separate comment because, while writing my PR
comment, the issue of adding async cache to Django might be larger but
this ticket is important in order to start porting the rest of Django to
async. Lots of things like the SessionCache backend and generally closing
the cache using Signals requires adding a lot of code; without those async
methods i.e. this ticket, we can't actually implement the code that uses
the cache.
To me, if DEP 9 is to implement Django templates, middleware, signals,
etc. as asynchronous, the base layer of the cake needs to be set first:
that means getting the cache and database access ready.
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Andrew Chen Wang | Owner: Andrew
| Chen Wang
Type: New feature | Status: assigned
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
Keywords: cache | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Andrew Chen Wang:
Old description:
> I've recently created a new package for Redis and Django integration at
> [https://github.com/Andrew-Chen-Wang/django-async-redis django-async-
> redis]. I'd like to add the missing methods, e.g. `get_async` or
> `set_async` to the BaseCache so people can get type hints or
> autocompletion suggestions when using `django.core.cache.cache` with
> async.
>
> Additionally, in order to be compatible with the async methods, there
> would need to be a new async method for closing cache connections/pools
> at
> [https://github.com/django/django/blob/999cddd58d30469f3ee85278985313fdf528323d/django/core/cache/__init__.py#L116-L121
> django.core.cache.backends.base]. I believe the only good solution to
> that would be:
>
> {{{
> async def close_caches_async(**kwargs):
> # Some caches -- python-memcached in particular -- need to do a
> cleanup at the
> # end of a request cycle. If not implemented in a particular backend
> # cache.close is a no-op
> for cache in caches.all():
> await cache.close_async()
> }}}
>
> Please let me know if that is out of scope though. I also believe
> implementing async methods for the current backends is also out of scope
> of this ticket (and my time :P). Edit: on second thought, I will add the
> methods to DummyCache as well.
>
> For reference, the Google Group Discussion:
> [https://groups.google.com/forum/#!topic/django-developers/sGq5Bnc2JMg
> here] and [https://groups.google.com/forum/#!topic/django-
> developers/NX03LNahZPo here]
New description:
I've recently created a new package for Redis and Django integration at
[https://github.com/Andrew-Chen-Wang/django-async-redis django-async-
redis]. I'd like to add the missing methods, e.g. `get_async` or
`set_async` to the BaseCache so people can get type hints or
autocompletion suggestions when using `django.core.cache.cache` with
async.
Additionally, in order to be compatible with the async methods, there
would need to be a new async method for closing cache connections/pools at
[https://github.com/django/django/blob/999cddd58d30469f3ee85278985313fdf528323d/django/core/cache/__init__.py#L116-L121
django.core.cache.backends.base]. I believe the only good solution to that
would be:
{{{
async def close_caches_async(**kwargs):
# Some caches -- python-memcached in particular -- need to do a
cleanup at the
# end of a request cycle. If not implemented in a particular backend
# cache.close is a no-op
for cache in caches.all():
await cache.close_async()
}}}
Please let me know if that is out of scope though. I also believe
implementing async methods for the current backends is also out of scope
of this ticket (and my time :P). Edit: on second thought, I will add the
methods to DummyCache as well.
For reference, the Google Group Discussion:
[https://groups.google.com/forum/#!topic/django-developers/sGq5Bnc2JMg
here] and [https://groups.google.com/forum/#!topic/django-
developers/NX03LNahZPo here]
Edit: I've decided to add the three main cache backends -- BaseCache,
DummyCache, LocMemCache -- with the necessary test cases which can be used
for all future async test cases for stuff like DBCache. Please read the
latest commit for which test cases are notable exceptions to the main
BaseCacheTests mixin.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Andrew Chen Wang | Owner: Andrew
| Chen Wang
Type: New feature | Status: assigned
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
Keywords: cache | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andrew Chen Wang):
Replying to [comment:4 Andrew Godwin]:
Hey Andrew, do you mind checking out if the LocMem and Base Cache is what
you thought about when implementing async caching? I'd also like to know
if there's a way to be more DRY with the docstrings.
Sorry if the commit is HUGE. The majority is just copying the test cases
and making use of the async versions. The LocMem backend changes just use
asyncio.Lock(). Thanks guys!
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:9>
Django
Reporter: Andrew Chen Wang | Owner: Andrew
| Chen Wang
Type: New feature | Status: assigned
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
Keywords: cache | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
Please don't change a scope of accepted tickets. This ticket is only about
`BaseCache`:
> My interpretation of DEP-9 is that we need to add default _async variant
methods to BaseCache that pass through to the synchronous versions - this
seems to be what the ticket is proposing, on its surface.
Any attempt to implement async cache backends should be preceded by a
discussion (e.g. [https://forum.djangoproject.com/t/async-cache-and-a
-helper-for-porting-sync-code/302/3 "Async Cache, and a helper for porting
sync code"] and probably an accepted DEP.
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:10>
Django
Reporter: Andrew Chen Wang | Owner: Andrew
| Chen Wang
Type: New feature | Status: assigned
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
Keywords: cache | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andrew Chen Wang):
Re-implemented in a new PR without all the extras (I'm sorry about that):
https://github.com/django/django/pull/13547 I took into account what
Andrew Godwin said, but I'm a little unclear about what I should actually
be changing. Are you saying change all the async functions to use
sync_to_async or how I've currently done it?
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:12>
Django
Reporter: Andrew Chen Wang | Owner: Andrew
| Chen Wang
Type: New feature | Status: assigned
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
Keywords: cache | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:13>
Django
Reporter: Andrew Chen Wang | Owner: Andrew
| Chen Wang
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: cache | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:14>
Django
Reporter: Andrew Chen Wang | Owner: Andrew
| Chen Wang
Type: New feature | Status: assigned
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:15>
Django
Reporter: Andrew Chen Wang | Owner: Andrew
| Chen Wang
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution: fixed
Keywords: cache | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"301a85a12f3c2c9427c7ff581fd4683bab1f29f6" 301a85a]:
{{{
#!CommitTicketReference repository=""
revision="301a85a12f3c2c9427c7ff581fd4683bab1f29f6"
Fixed #32076 -- Added async methods to BaseCache.
This also makes DummyCache async-compatible.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32076#comment:16>