Persist setup chunks#346
Merged
Merged
Conversation
Would allow us to later retrieve these setup chunks without having to write them to the client.
We don't want to risk exposing a setup chunk which might be sensitive in memory to users running exercises. Here, we just clobber the same value with the appropriate setup chunk so we don't risk exposing anything inadvertantly.
…global_setup in the `evaluate_exercise` expression
schloerke
reviewed
Apr 3, 2020
schloerke
reviewed
Apr 3, 2020
schloerke
reviewed
Apr 3, 2020
schloerke
reviewed
Apr 3, 2020
Contributor
Author
|
@schloerke just pushed a commit that addresses the feedback. Thanks for catching those! |
schloerke
requested changes
Apr 3, 2020
Collaborator
schloerke
left a comment
There was a problem hiding this comment.
If we could add the "reset setup chunks" as a function, then it looks good to me.
schloerke
approved these changes
Apr 13, 2020
* Pass in the raw exercise to evaluators Previously, we only got the expression associated with the exercise to evaluate, but couldn't easily access the exercise or its metadata. * Add remote evaluator * More consistent error handling and env var vs option integration * Rearrange * Enable remote evaluators that choose to use this function to include global_setup in the `evaluate_exercise` expression * Uninstall our knitr source hook * Add a clear function * Test blocking session initiation * Make initiate more robust, more tests * Refactor remote to make async * Add testing for remote evaluator * Add NEWS * Regen roxygen * Add docs for remote evaluator. * Fix null encoding in JSON and misnamed callback. * Work around some edge cases when serializing, update swagger * Run global setup prior to the checker. * Include an RMD that has a series of tests that can vet the remote evaluator * Update R/evaluators.R Co-Authored-By: Barret Schloerke <barret@rstudio.com> * new_remote_evaluator -> remote_evaluator * Remove JSON OpenAPI spec * remote_evaluator -> external_evaluator * Note that external evaluators are experimental * Added usethis lifecycle dependencies. Co-authored-by: Barret Schloerke <barret@rstudio.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This stashes the setup chunks in an environment called
setup_chunks. This will enable two things:-setupchunks private instead of serializing them through the client. This work has not been taken on yet, though.You'll find the following changes:
rmarkdown::run.-setupsuffix.setupchunk is stored under a special name__setup__(to avoid conflicts with a chunk calledsetup-setup.setup-global-exercisewhich -- if it exists -- will take the place of thesetupchunk in thissetup_chunksenvironment. We do this because a.) the globalsetupchunk may include initialization code that takes time and isn't relevant to the exercises, and b.) the globalsetupchunk may include sensitive information that we don't want to leak into the user's exercise environment. (I presume that by sending a setup chunk to run with arbitrary user code, you're effectively giving access to that code.) Therefore users can opt to define asetup-global-exercisechunk which only prepares the specific environment that their exercises will need. Exercise-specific-setupchunks continue to behave as they did in the past.source. These hooks are documented here. See below for description.TODO
testsNEWS(no user-visible changes)remove hooktesting & validation notesdocs(no user-visible changes, the corresponding docs will be folded into the next PR)knitr hooks
Currently, we set a global knitr option to set
tutorial = TRUE: https://github.com/rstudio/learnr/blob/master/R/knitr-hooks.R#L7This has the same effect as writing
tutorial=TRUEas an option on top of each of the code chunks in the learnr document. Then when we want to act on the chunks in the document, we can just use thetutorialhook to do so, since all chunks have that options set toTRUE. https://github.com/rstudio/learnr/blob/master/R/knitr-hooks.R#L118Unfortunately, setting tutorial to TRUE only affects latter chunks, not the current one. From
?knitr::opts_chunk:Since the knitr hook gets defined in
.onAttach(see here), this means that whatever chunk callslibrary(learnr)will invoke this code, and it will impact all the chunks below that one -- but not the current one. That's a problem for us, sincelibrary(learnr)usually gets called in the setup chunk.Therefore we can't just hook into the tutorial option like we've done in the past -- instead we have to hook onto something that globally has access to all chunks including the
setupchunk. Therefore, we add a new hook onsource =which handles that. Unfortunately, these hooks are all singletons so we clobber any existing hooks when we assign. Therefore you'll see that we take precaution to preserve the existing hook and invoke it later at the end of our hook. So this hook only has side-effects of stashingsetupchunks, but the return value continues to be from whatever hook was defined before us.Testing & Validation
There are no user-visible changes in this PR, but we can still test a few things.
Run
remotes::install_github("rstudio/learnr@save-setup")Confirm we didn't break exercise-specific setup chunks
In the RStudio IDE, click
New>R Markdown>From Template>Interactive Tutorialto get a learnr template that you can play withabove the chunk named

two-plus-two, add a new chunk namedtwo-plus-two-setup. This is exercise-specific setup code that should be run ahead of whatever code you provide in the exercise. In this-setupchunk, define a variable namedx.click
Run Documentand in the first exercise, runx. You should see it print out whatever value you assigned in the-setupchunk.In another exercise chunk, set the code to
xand observe the error:object 'x' not found.This confirms that the exercise-specific setup chunks still work. They run only for the exercise whose name they match, but not for any others.
Test that
setupbecomes the global setup chunkThe desired behavior is that the global exercise setup would come from a chunk named
setup-global-exercise. If no chunk by that name exists, then it would use the chunk namedsetup.[1] "__setup__" "two-plus-two") and the second call should print out whatever you have in yoursetupchunk at the top of your document. You can modify that chunk and rerun the tutorial to confirm that it's updated.setup-global-exercisewith some code inside of it. When you rerun the tutorial, you should now see that the above code, when run in an exercise, prints the contents of this new code chunk instead of the originalsetupchunk.setup-global-exercisechunk is empty with no code in it, it's still preferred as the global setup chunk (this exercises a different code path than when it has content in it).