Skip to content

Conversation

@uranusjr
Copy link
Contributor

@uranusjr uranusjr commented Feb 13, 2022

The functools.cached_property can now decorate an async function. It would cache a wrapper to the coroutine returned by the function, and await it when the wrapper is first awaited.

https://bugs.python.org/issue46622

The functools.cached_property can now decorate an async function. It
would cache a wrapper to the coroutine returned by the function, and
await it when the wrapper is first awaited.
Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

I have minor notes regarding the feature implementation.
Also, I'd like to get a consensus for the question on bpo first.

val = cache.get(self.attrname, _NOT_FOUND)
if val is _NOT_FOUND:
val = self.func(instance)
if inspect.iscoroutinefunction(self.func):
Copy link
Contributor

Choose a reason for hiding this comment

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

inspect.isawaitable(val) check covers more cases, please use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would change the behaviour of

class MyAwaitable:
    def __await__(self):
        print("called")

class Foo:
    @cached_property
    def get_awaitable(self):
        return MyAwaitable()

Currently this would cache the MyAwaitable object and calls __await__. If we use isawaitable here, MyAwaitable would be wrapped and only awaited once.

IMO iscoroutinefunction is more correct here.

)

def __get__(self, instance, owner=None):
import inspect
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the import just before usage, when the attribute name is not in cache yet.
The import instruction is not for free, it requires a lock at least IIRC.
The move allows returning cached results as fast as possible.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.



class _CachedAwaitable:
"""Wrapper to return if @cached_proeprty is used on a coroutine function.
Copy link
Contributor

Choose a reason for hiding this comment

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

spelling: cached_property

A coroutine shouldn't simply be cached since it can only be awaited once.
This wrapper is cached instead, which awaits the underlying coroutine at
most once and cache the result for subsequent calls.
Copy link
Contributor

Choose a reason for hiding this comment

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

"and caches"

@uranusjr
Copy link
Contributor Author

Closing for now to work on lru_cache first, as discussed in the issue.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants