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

Improve Python and additional exercise engine support #724

Merged
merged 27 commits into from
Aug 29, 2022

Conversation

gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Aug 4, 2022

Improves Python exercise support in learnr. This PR replaces previous transformations at key steps in the exercise rendering process with generic functions that dispatch on the class of exercise, where we now add the exercise engine as a subclass of tutorial_exercise. This allows us to provide a generic workflow that can be extended as needed by additional engine-specific methods.

All of the generics now use the prefix render_exercise_ since they are part of the render_exercise() flow.:

  • render_exercise_prepare() replace prepare_exercise() and possibly modifies the exercise in preparation for rendering.

  • render_exercise_rmd_prep() and render_exercise_rmd_user() provide a vector of source code lines that are used in the .Rmd rendered by render_exercise() to evaluate setup code (prep) and user code.

  • render_exercise_post_stage_hook() defines hooks that are called after the prep and user stages and can potentially modify envir_prep and envir_result respectively.

  • render_exercise_result() can take all of the various exercise result pieces and may modify them all as needed to provide the final exercise result object.

One big change from the original python support implementation is that the global python environment is duplicated into .__py__ in both envir_prep and envir_result. The new name ensures there is zero chance that we accidentally overwrite something in those environments.

Broader impact: because we're adding additional classes to the tutorial exercise object, I've incremented the exercise version number to 4 and have added a migration path in upgrade_exercise().

nischalshrestha and others added 21 commits August 4, 2022 09:48
Finalize the result and prepare the returned environments, etc.
Replaces `prepare_exercise()`
Replaces `exercise_code_chunks_user_rmd()`
Default is to simply call `exercise_code_chunks_prep()`
Also refactored actual rendering of Rmd into `render_exercise_evaluate_{prep,user}()` so we can call the post-stage hooks via exit handlers
@gadenbuie gadenbuie self-assigned this Aug 4, 2022
Copy link
Contributor

@nischalshrestha nischalshrestha left a comment

Choose a reason for hiding this comment

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

This is super super exciting, and I love the generics pattern! ❤️

As far as the Python bits go, this is pretty good. I like the fact that we now have a post "user" stage to include the py environment as .__py__, the $py always felt unclean. I just had a small suggestion which is really just fixing my bad documentation for clear_py_env

I will do some testing locally on my side for slightly more complex exercises as well to make sure we're all good.

R/utils.R Outdated Show resolved Hide resolved
Co-authored-by: Nischal Shrestha <bitsorific@gmail.com>
@nischalshrestha
Copy link
Contributor

nischalshrestha commented Aug 4, 2022

I think I may have found a bug? So the tests currently for the new exercise structure will pass when using mock_exercise() because you set the engine as part of the class of the exercise object.

  if (version == 4) {
    class <- c(engine, class)
  }

However, for the latest exercise version we simply return exercise object without adding the new classes.

  if (identical(exercise$version, current_exercise_version)) {
    return(exercise)
  }

That would prevent us from invoking the generic methods throughout the exercise prep, render, post stage, check process based on debugging learnr. And, I don't see the new classes being set for the exercise object.

@gadenbuie
Copy link
Member Author

@nischalshrestha The classes are added in the knitr hooks, which means that the exercise objects in the cache also have those classes... but classes (like other attributes) are dropped by append() when we combined the user input with the full exercise object. I've fixed things in a few places to make sure that, basically no matter what, these classes are added correctly — and that the class related to the exercise engine is always the first class.

@nischalshrestha
Copy link
Contributor

@nischalshrestha The classes are added in the knitr hooks, which means that the exercise objects in the cache also have those classes... but classes (like other attributes) are dropped by append() when we combined the user input with the full exercise object. I've fixed things in a few places to make sure that, basically no matter what, these classes are added correctly — and that the class related to the exercise engine is always the first class.

That's a tricky one! Good catch in where the class got lost. The append() has bit me similarly before. 😅

R/exercise.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
gadenbuie and others added 2 commits August 9, 2022 10:34
Co-authored-by: Nischal Shrestha <bitsorific@gmail.com>
@gadenbuie gadenbuie merged commit 8d6bc72 into main Aug 29, 2022
@gadenbuie gadenbuie deleted the feat/python-support branch August 29, 2022 16:42
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.

None yet

2 participants