Skip to content
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

Fixes for learnr to external evaluator #420

Merged
merged 4 commits into from
Aug 20, 2020
Merged

Conversation

nischalshrestha
Copy link
Contributor

@nischalshrestha nischalshrestha commented Aug 13, 2020

Description

Problems

When using the "new" (commit >= bd29e0c) learnr against an "old" (commit: cf5fe34) learnr on the external evaluator, there are evaluator and feedback return issues:

  • the evaluator always returned four empty lines for every exercise.
  • the engine for R exercise needs to be capital "R" not "r"
  • the exercise$options$code for some reason gets reused as exercise code instead of exercise$code, which causes the external server to always run the original exercise code (from knitr chunk) instead of the user code.
  • regular setup code will not work because the exercise$setup is always NULL since we now store all setup chunks in a exercise$chunks list.
  • exercise.checker for new learnr caches function which causes issues on deserializing on external evaluator.

This is because the exercise structure for new learnr changed by: 1) forwarding all knitr options to evaluation, which causes a problem when sending some exercise$options fields like eval (produces for empty lines), and 2) exercise.checker was treated as a function when caching exercise.

Fixes

  • In knitr-hooks.R, removing the exercise$eval field fixes the empty lines issue, while removing exercise$options$code makes sure the old learnr executes user code, NOT original knitr chunk code.
  • The exercise$options$engine will be removed on external evaluator (as both old learnr and new learnr do not directly use it).
  • To fix regular setup* code for exercises, I added back an exercise$setup in knitr-hooks.R (another field for cache), which contains either NULL if there is not setup code, or a character containing all of the setup code required (newline separated).
  • exercise.checker for new learnr is back to being a character, so now all learnr versions will stick to this format.

*Note: this means that regular exercise setup will work, but chained will not, especially if there is non-R setup chunks.

You can test the solution by running evaluator-tests.Rmd learnr document and going through all tests in the tutorial.

Copy link
Contributor

@trestletech trestletech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I think my intuition is that we'd want to keep the all_setup_code change to ensure backwards compatibility. But the other changes strike me as things that could live inside the external evaluator instead of learnr having to eternally jump through hoops to present the exercise in a compatible format.

R/utils.R Outdated Show resolved Hide resolved
R/knitr-hooks.R Outdated Show resolved Hide resolved
R/knitr-hooks.R Outdated Show resolved Hide resolved
@trestletech
Copy link
Contributor

We discussed and the root of the problem here is that learnr recently attached arbitrary properties it gets from knitr to each exercise. It turns out that some of the properties cause problems when interacting with old learnrs in a remote evaluator.

Given that these changes were recently introduced and we're basically just scrubbing a couple of known-problematic fields off of the exercise, this doesn't feel so out of place to me.

I vote we move forward with this approach but moving forward do some work to think about how we can better capture what structure an exercise is supposed to have so that others (e.g. remote evaluators) can confidently interact with them.

@nischalshrestha nischalshrestha changed the title External evaluator no longer prints empty lines for exercise output. Fixes for learnr to external evaluator Aug 14, 2020
Copy link
Collaborator

@schloerke schloerke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supporting v0 learnr exercises will get hard quickly moving forward. The changes made to support all_setup_code are isolated, but I'd like to time box (via CRAN releases) the v0 support to allow for growth.

@nischalshrestha nischalshrestha merged commit d03456c into master Aug 20, 2020
@nischalshrestha nischalshrestha deleted the rmote-fix/empty-lines branch August 20, 2020 15:54
schloerke added a commit that referenced this pull request Sep 2, 2020
* master:
  Add events to website (#423)
  Add a checker test with error as solution (#422)
  Fixes for learnr on external evaluator (#420)
  Add shinytest support (#407)
  Validate the ability to create an idb store (#417)
  Disable completion of R code inside quotes, closes #401 (#413)
  Bump version (#414)
  Save tutorial output to temp folder when tutorial folder does not have write permissions (#412)
  Throw an informative error if an exercise chunk is NULL (#411)
  Remove outdated exercise option
  Checking for exercise errors (#403)
  Add event handler system (#398)
  External evaluator fixes (#399)
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.

3 participants