r-lib / memoise

Easy memoisation for R
https://memoise.r-lib.org
Other
317 stars 56 forks source link

Use caching objects from cachem package #115

Closed wch closed 3 years ago

wch commented 4 years ago

This PR updates memoise to use the caching objects from the cachem package. These caching objects support automatic pruning, so that they won't grow indefinitely.

wch commented 4 years ago

I believe the covr tests are failing because covr manipulates the AST of the code, but this code expects things to be in a specific place:

https://github.com/wch/memoise/blob/b1c628d5e9cd7fcac957bca918c516a2d7324573/R/memoise.R#L185-L195

The has_cache() and drop_cache() functions do something similar, but they don't result in problems with covr.

jimhester commented 4 years ago

I don't think you should try to handle old style caches at all, people will just need to generate new caches.

wch commented 4 years ago

As for supporting old-style caches, I wanted to make sure that if people had made their own cache objects, it wouldn't break for them.

How about this: I'll have wrap_old_cache() print a message if it's called, and I'll update the existing caching objects in memoise to use the new API. Actually, now that I think of it, it could be problematic to update the API of existing caching objects because it could break other (non-memoise) code that uses them.

wch commented 3 years ago

I've updated the PR so memoise() now takes an argument hash, which is a function that takes an R object as input and returns a hash of that object. The default value is:

  hash = function(x) digest::digest(x, algo = "spookyhash"))

Because the default is defined in the memoise() function, it captures the environment. I don't know if this is a problem or not. It looks like efforts were made to avoid capturing the function environment, by using memo_f_env, but it's possible that the purpose of that environment is not necessarily to avoid capturing the environment, but rather to make the memoized function have the original function's environment as an ancestor.

One possible way to avoid this issue: now that rlang 0.4.10 has a built-in hashing function, we could take a dependency on rlang instead of digest, and the default could be:

  hash = rlang::hash