_PickleCore._get_cache_by_key() returns dict but is used as a dataclass

[Leinadium]

This library is required by copernicusmarine, and recently one of my data pipelines raised an exception with the following message:

dict has no attribute "._completed"

Unfortunaly I couldn't get the full exception, so I had to trace all references to the ._completed attribute, and found this python module.

While checking the code, I found that the PickleCore get_entry_by_key() internally calls _get_cache_by_key(), expecting a return type of CacheEntry. But the _get_cache_by_key() method has a return type of Dict[str, CacheEntry]]

I would open a PR, but I couldn't understand what is the expected return type of _get_cache_by_key().

This bug seems to be triggered whenever cachier() is called with the pickle implementation and with the separate_files set as True.

[shaypal5]

@Borda, this seems related to the new types you defined.

[Borda]

well it seems that our tests are passing: https://github.com/python-cachier/cachier/actions/runs/11455222345/job/31870885544?pr=248#step:5:92

so not sure how to reproduce it... 😖

[Borda]

ok, seems I have another example here: https://github.com/Lightning-AI/torchmetrics/actions/runs/11454171728/job/31867880744?pr=2797

args = (['i like python', 'what you mean or swallow'], ['i like monthy python', 'what do you mean, african or european swallow'])
kwds = {}, _allow_none = False, ignore_cache = False, overwrite_cache = False
verbose = False
_stale_after = datetime.timedelta(days=999999999, seconds=86399, microseconds=999999)
_next_time = False
kwargs = OrderedDict([('preds', ['i like python', 'what you mean or swallow']), ('target', ['i like monthy python', 'what do you mean, african or european swallow'])])
_print = <function cachier.<locals>._cachier_decorator.<locals>.func_wrapper.<locals>.<lambda> at 0x7f4022d1b790>
key = 'be998a286c65dcd42f199cbfd31b2ede04850e0cd5986ae5961f30e88747424b'
entry = {'being_calculated': False, 'stale': False, 'time': datetime.datetime(2024, 5, 12, 0, 26, 45, 249480), 'value': 0.5104166666666667}

    @wraps(func)
    def func_wrapper(*args, **kwds):
        nonlocal allow_none
        _allow_none = _update_with_defaults(allow_none, "allow_none", kwds)
        # print('Inside general wrapper for {}.'.format(func.__name__))
        ignore_cache = _pop_kwds_with_deprecation(
            kwds, "ignore_cache", False
        )
        overwrite_cache = _pop_kwds_with_deprecation(
            kwds, "overwrite_cache", False
        )
        verbose = _pop_kwds_with_deprecation(kwds, "verbose_cache", False)
        ignore_cache = kwds.pop("cachier__skip_cache", ignore_cache)
        overwrite_cache = kwds.pop(
            "cachier__overwrite_cache", overwrite_cache
        )
        verbose = kwds.pop("cachier__verbose", verbose)
        _stale_after = _update_with_defaults(
            stale_after, "stale_after", kwds
        )
        _next_time = _update_with_defaults(next_time, "next_time", kwds)
        # merge args expanded as kwargs and the original kwds
        kwargs = _convert_args_kwargs(
            func, _is_method=core.func_is_method, args=args, kwds=kwds
        )
    
        _print = print if verbose else lambda x: None
    
        if ignore_cache or not _global_params.caching_enabled:
            return (
                func(args[0], **kwargs)
                if core.func_is_method
                else func(**kwargs)
            )
        key, entry = core.get_entry((), kwargs)
        if overwrite_cache:
            return _calc_entry(core, key, func, args, kwds)
        if entry is None or (
>           not entry._completed and not entry._processing
        ):
E       AttributeError: 'dict' object has no attribute '_completed'

[Leinadium]

After some examination, from my understanding:

PickleCore.get_entry_by_key is supposed to return an entry (and its own key).

If reload is set to True, the method calls _get_cache(), which returns a cache (dict), and then calls get to get an entry.
But, if separate_files is set to True, then it just calls get_cache_by_key, which returns a full cache, not just a cache entry.

From my understanding, as both the cache and cache entry were dicts before, whichever function called get_entry_by_key would attempt to access some field thinking it was a cache entry, fail as it is a full cache, and handle it as a cache miss. This could be why the tests weren't triggering before (although some tests were failing as warnings #247)

EDIT: While locally testing, I found out that _get_cache_by_key was returning a CacheEntry correctly. Maybe the type hint is actually supposed to be Optional[CacheEntry] instead of Optional[Dict[str, CacheEntry]]

[Borda]

From my understanding, as both the cache and cache entry were dicts before, whichever function called get_entry_by_key would attempt to access some field thinking it was a cache entry, fail as it is a full cache, and handle it as a cache miss. This could be why the tests weren't triggering before (although some tests were failing as warnings #247)

hmm think that with the refactor to entry we sufficed some hidden issues 🐰

[veenstrajelmer]

I also encountered something similar here: mercator-ocean/copernicus-marine-toolbox#183
This error only occurs with cachier 3.1.1, not with 3.0.1. So could it be that there is a bug introduced in that version?

[Leinadium]

So could it be that there is a bug introduced in that version?

It seems that this new version uses a dataclass instead of a dict for the CacheEntry implementation, and it caused some problems

hmm think that with the refactor to entry we sufficed some hidden issues 🐰

@Borda, I decided to check locally, and found out that during tests, _PickleCore._get_cache_by_key does return a CacheEntry, and not a cache dict! So maybe this is the intended behaviour, and the type hint is just wrong.

The error dict has no attributed ._completed may have happened because the 3.0.1 version used dicts, and it saved the cache entry as a dict. Some time later, when using the 3.1.1 version, it expected a dataclass object, but received the old dict implementation and failed.

So possibly this is just a product of incompatible caches between versions.

[Borda]

found out that during tests, _PickleCore._get_cache_by_key does return a CacheEntry, and not a cache dict! So maybe this is the intended behaviour, and the type hint is just wrong

@Leinadiumdo you have minimal example to reproduce which we can add to tests?

[Borda]

The error dict has no attributed ._completed may have happened because the 3.0.1 version used dicts, and it saved the cache entry as a dict. Some time later, when using the 3.1.1 version, it expected a dataclass object, but received the old dict implementation and failed.

right, this could be a transition issue as your past cache is dict but the new/expected is dataclass

[Leinadium]

@Leinadiumdo you have minimal example to reproduce which we can add to tests?

The tests are all fine. The tests expect that _get_cache_by_key returns a CacheEntry, and they succeded just fine. I modified the code to return a cache dict, and they started to fail. So the implementation is correct, just the type hint is wrong.

right, this could be a transition issue as your past cache is dict but the new/expected is dataclass

Other developers noticed the same problem here, and yes, caches are now incompatible between versions.

[Borda]

So the implementation is correct, just the type hint is wrong.

I see so we shall consider adding some typing check like mypy

Other developers noticed the same problem here, and yes, caches are now incompatible between versions.

going to fix it now, just thinking about how to test it :/

[Leinadium]

I see so we shall consider adding some typing check like mypy

That might be a good idea! I opened the project in my local IDE, and some static and type checks starting popping (e. g. multiple "possible None being used" values). It is a good idea!

going to fix it now, just thinking about how to test it :/

The pickle object is being returned without any checks. You may add some checks to see if the object is a CacheEntry, and then for testing, you can create a bad/corrupt pickle file, and check for empty caches (and not failures).

[Borda]

The pickle object is being returned without any checks. You may add some checks to see if the object is a CacheEntry

that is fine and simple

and then for testing, you can create a bad/corrupt pickle file, and check for empty caches (and not failures).

I was rather thinking about running examples with several versions installed

[Borda]

@Leinadium could you test it also on your side with the open PR:

cachier @ https://github.com/python-cachier/cachier/archive/refs/heads/fix/legacy.zip

[shaypal5]

Fixed in release v3.1.2. Please reopen if that is not the case.