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

Query decorated function if result is from cache #51

Closed
wants to merge 7 commits into from

Conversation

kitmonisit
Copy link

@kitmonisit kitmonisit commented Apr 15, 2021

Decorated function gains a new attribute function, is_from_cache, which returns True if the result comes from the cache; otherwise False.

For example, if the decorated function is named foo():

>>> foo()
>>> foo.is_from_cache()
False
>>> foo()
>>> foo.is_from_cache()
True

@kitmonisit
Copy link
Author

@shaypal5 I know this is not what you suggested in #46 (comment), but hear me out for a bit:

I realized there was a need to encapsulate the is_from_cache() information, i.e. localized to the decorated function. In the existing code, there are provisions for manipulating the cache specific to the decorated function, such as clear_cache(). My implementation here follows that paradigm.

@shaypal5
Copy link
Collaborator

API-wise, you're right ― there are existing precedents to modifying, and actually enhancing, the decorated function's API with additional attributes, such as clear_cache().

However, the use case here is different. clear_cache() clears an attribute that is unique to the function ― its cache ― and it thus make sense that the function is bound to the decorated function. In your case, the action is bound to a specific call of the function, and thus this is inherently not appropriate to bound it to the function, but rather to the call.

The practical implication is that this API is indeed not thread-safe: You might call is_from_cache() straight after the call to foo(), and still get an irrelevant result if another call to foo() was made in another thread between the two calls. Since at the moment a significant feature of cachier ― and one which some work went into ― is that it is thread-safe, I would prefer not to break it for the current implementation suggestion.

I still think a simple listener object is the way to go, since it is bound to the call rather than the function. This is like future objects in various programming languages, that are implemented as per-call instances you get and query. So yeah, maybe something like this:

call_info = cachier.Info()
foo(cachier_info=call_info)
if call_info.is_from cache:
    # do something

The more general name and structure of a call information object is intentional, since this is an easily expandable API, onto which we can graft additional functionalities for call-specific information (an obvious and useful example: get the time in which the value in the cache was produced).

Let me know what you think!
Shy

@shaypal5 shaypal5 self-requested a review April 16, 2021 09:01
Copy link
Collaborator

@shaypal5 shaypal5 left a comment

Choose a reason for hiding this comment

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

See my comment about the fundamental problems in this implementation - first among them is breaking thread-safety.

@kitmonisit
Copy link
Author

Ah, wonderful. And here I thought I was being thread-safe by localizing the query but as you point out, it's exactly the opposite. Let me take another swing at it!

@kitmonisit
Copy link
Author

kitmonisit commented Apr 17, 2021

Regarding thread-safety, here is how I imagine a demonstration:

from random import random
from cachier import cachier, Info
from concurrent.futures import ThreadPoolExecutor

@cachier()
def foo(arg):
    return random()

# This is thread-safe
# Info object is unique to every call of caller (and subsequently, foo)

def caller(arg):
    call_info = Info()
    result = foo(arg, cachier_info=call_info)
    return {
        'result': result,
        'arg': arg,
        'is_from_cache': call_info.is_from_cache
    }

with ThreadPoolExecutor(max_workers=2) as pool:
    arglist = [1, 2, 3, 1]
    results = list(pool.map(caller, arglist))


# This is NOT thread-safe
# Info object is shared among all concurrent calls of caller

call_info = Info()

def caller(arg):
    result = foo(arg, cachier_info=call_info)
    return {
        'result': result,
        'arg': arg,
        'is_from_cache': call_info.is_from_cache
    }

with ThreadPoolExecutor(max_workers=2) as pool:
    arglist = [1, 2, 3, 1]
    results = list(pool.map(caller, arglist))

Is that how you envisioned it, @shaypal5?

@shaypal5
Copy link
Collaborator

shaypal5 commented May 1, 2021

Just writing to say I didn't forget this. This is on my TO-DO list for very soon. :)

@shaypal5
Copy link
Collaborator

Yes, that looks good!

I'm sorry for the extremely belated response!
You want to change the PR accordingly, so we can iron out the details?

@shaypal5 shaypal5 added the stale Issues and PRs that seem abandoned by their authors. label Nov 15, 2021
@shaypal5 shaypal5 closed this Nov 28, 2023
@shaypal5
Copy link
Collaborator

Closed after being stale for a long time.
Please open a new PR if you with to contribute this feature at some point in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement stale Issues and PRs that seem abandoned by their authors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notify successful fresh cache retrieval
2 participants