Skip to content

Use caching objects from cachem package#115

Merged
jimhester merged 25 commits into
r-lib:masterfrom
wch:cache
Jan 6, 2021
Merged

Use caching objects from cachem package#115
jimhester merged 25 commits into
r-lib:masterfrom
wch:cache

Conversation

@wch
Copy link
Copy Markdown
Member

@wch wch commented Oct 31, 2020

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
Copy link
Copy Markdown
Member Author

wch commented Oct 31, 2020

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
Copy link
Copy Markdown
Member

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

Comment thread R/memoise.R Outdated
@wch
Copy link
Copy Markdown
Member Author

wch commented Nov 2, 2020

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
Copy link
Copy Markdown
Member Author

wch commented Dec 18, 2020

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

Comment thread R/memoise.R Outdated
@jimhester jimhester merged commit 5e73136 into r-lib:master Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants