yihui / knitr

A general-purpose tool for dynamic report generation in R
https://yihui.org/knitr/
2.37k stars 874 forks source link

Fix an issue with NULL object on cache lazy db loading #2249

Closed cderv closed 1 year ago

cderv commented 1 year ago

This fixes #1842

Small fix as it seems this is NULL which is causing with the lazy load db.

If we want broader change, we can make sure that cache_save change NULL in keys to character(0) or do nothing if only NULL in keys

https://github.com/yihui/knitr/blob/039b6cfa45dea4ece31f2bcb65ed3b34b1330daa/R/cache.R#L17-L35

Or something different if you have idea.

Though this PR fix is easy enough and should not create more issues.

yihui commented 1 year ago

FYI the knitr-examples issue should be fixed now: https://github.com/yihui/knitr-examples/commit/d76fe69b917a3458c430515945816c16de1fe73b

cderv commented 1 year ago

Let's apply a broader change instead and cache the objects only when length(keys) > 0. Thanks!

One "tricky" thing in our cache management is that we rely on the assumption that if cache.lazy = TRUE (the default), then we expect a lazyload db to exist (in addition to .RData file). We check that for cache_exists() . If we do not create the lazydb files when length(keys) > 0 in cache_save, then we need to modify cache_exists to handle this assumption. Especially here https://github.com/yihui/knitr/blob/039b6cfa45dea4ece31f2bcb65ed3b34b1330daa/R/block.R#L96-L104 or https://github.com/yihui/knitr/blob/d90b0161ddc298b49ec183ac481f4101ad46d8ce/R/block.R#L250-L256

We still need to consider the cache to exists when .RData is here, so that cache$output() is correctly applied.

We could adapt cache_exists to be less strict about rdb or rdx file existance but this a broad change for sure. I wanted to confirm before going this road (I started to look at how to adapt though). If you want to screenshare to discuss, ping me.

yihui commented 1 year ago

Okay, let's coerce keys to character then, i.e., use as.character(keys). Thanks!

cderv commented 1 year ago

@yihui this was indeed a very good fix. I wasn't sure what keys could be really, but now it makes sense, and this is a better fix than mine.

Thanks