-
-
Notifications
You must be signed in to change notification settings - Fork 872
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
cache engine API #1518
cache engine API #1518
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With some minor touches, I think we will be ready to go. Thanks!
R/engine.R
Outdated
#' The cache engine function should load the cache environment and should | ||
#' know the extension appropriate for the language. | ||
#' @export | ||
#' @examples cache_engines$set(python = reticulate::load_python_session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reticulate::load_python_session
is not available in reticulate yet, so I cannot use this example at the moment. Once rstudio/reticulate#167 is merged, I'll just set this caching engine for python in knitr.
R/engine.R
Outdated
@@ -178,6 +196,19 @@ eng_python = function(options) { | |||
} | |||
} | |||
|
|||
eng_python_cache = function(options) { | |||
if (isFALSE(options$python.reticulate)) { | |||
eng_interpreted(options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, you can just return()
instead of running the python code chunk again. Results in a previously run have been saved and will be loaded.
R/engine.R
Outdated
if (isFALSE(options$python.reticulate)) { | ||
eng_interpreted(options) | ||
} else { | ||
if (!loadable('reticulate')) warning2( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove this warning, since users will see it in the first time. The whole function here can be
eng_python_cache = function(options) {
if (isFALSE(options$python.reticulate)) return()
# TODO: change this hack to reticulate::load_python_session(options) after
# https://github.com/rstudio/reticulate/pull/167 is merged and released
if (!'load_python_session' %in% ls(asNamespace('reticulate'))) return()
fun = getFromNamespace('load_python_session', 'reticulate')
fun(options)
}
R/parser.R
Outdated
#' that future chunks can (re)use the code by chunk label references. | ||
#' If an external chunk has the same label as a chunk in the current session, | ||
#' chunk label references by future chunks will refer to the | ||
#' external chunk. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know how to git rebase
against my master branch here? If you do, please rebase and force push to get rid of the superfluous changes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments! I'm busy all weekend, so I'll incorporate the changes next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. There is no hurry at all. Please take your time!
…engine-specific caches
Okay, I rebased both the knitr and the reticulate PR. Thanks for the suggestion. I also made some minor changes based on your feedback, which I tested in the reticulate PR: rstudio/reticulate@ff39889 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked it a little bit. Looks good to me now. Thanks!
My proposal is to add a hook for knitr language engines that allows an additional caching mechanism for that language's environment. This addresses issue #1505.
The basic idea is to add a new object that can attach cache engines:
This is also relevant to pull request #167 on the
reticulate
repo, which implements a caching mechanism using this API. These pull requests are intertwined, because they use the other's new API.I wasn't sure how to implement testing, since these two issues overlap. So I refer you to the tests in the
reticulate
pull request. In particular, this unit test implements the issue described in #1505 and verifies that the solution works.This does not implement any
autodep
for the cache engine.