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

Fix and improve knitr chunk option handling #668

Merged
merged 24 commits into from
Mar 14, 2022
Merged

Conversation

gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Feb 28, 2022

Fixes the underlying problem reported in #667 and improves support for sql exercises.

Fixes #667

Fixing knitr chunk option handling

The bug in #667 is that we were not properly creating the knitr_options object that is used by the base format that renders the exercise code. We weren't moving the exercise options in opts_chunk as needed, now by putting these in the right place they are used as expected.

As part of fixing the bug above, we are now delegating more to the prepare_exercise() helper. The goal is that any preparation required to special-case different types of exercises should happen here. For example, there's some small re-arranging of exercise options that is needed to smoothly support SQL chunks; this now happens inside prepare_exercise().

After passing through prepare_exercise(), the exercise object gains an $opts_chunk item which contains the prepared knitr options for the exercise rendering. Previously we were pulling from exercise$options, but those might be slightly different. Note that we don't touch those options so that we don't lose the original exercise options — some exercise.* options are stored there but aren't needed for rendering and are removed from $opts_chunk.

As part of this work, I also refactored merge_options(). It still follows the same general structure, but I've renamed variables to be a little more explicit.

Improved SQL Exercise Support

This PR also improves support for SQL exercises (and in general non-R language exercises). SQL exercises have been a good test case since they involve a number of pieces that will likely arise in other exercise situations:

  1. Test for engine (language) types. First, we can now use is_exercise_engine(exercise, engine) to test if an exercise uses a particular engine. The engine type applies to the user code and the solution code; other supporting chunks may be in R (or other languages).

  2. Pre-process the exercise. As described above, we may need to re-arrange the exercise data to prepare for rendering. This includes setting global knitr chunk options, adjusting specific chunk options on the exercise chunk, adding setup code, etc. This action should be handled in prepare_exercise(), delegating to prep functions for specific engines, e.g. prepare_exercise_if_sql().

    In sql exercises, we add or force the output.var chunk option to the exercise chunk to something predictable. When rendered, the results of the SQL query will be stored in a predictable location, allowing us to provide the result to the checking functions as the last_value.

  3. Adjust the exercise.Rmd template. The knitr SQL engine either prints the result of a SQL query or stores the result in the variable named by output.var. Since we want to both see the printed output and capture the result, we need to add additional code to the exercise.Rmd template.

    I added exercise_code_chunks_user_rmd(), which now prepares the contents of exercise.Rmd. In general, this involves reading from the internal exercise template and appending the exercise chunk with the user's submitted code. If additional processing is required, we can detect the exercise engine and add additional lines to this file, as in the case of the SQL engine.

  4. Post-process exercise results. After rendering, we may need to post-process the results to put them in the expected places. In the SQL case, we do two things:

    1. Ensure that the database connection is available in envir_prep with the name from the connection chunk option. This lets grading code find the connection — in gradethis the connection object will automatically be present inside grade_this().
    2. If the author included a the output.var chunk option on the exercise, we ensure that the results are available under that object name in envir_results. If not, we remove our temporary variable ___sql_results since it's not needed. (If this sounds convoluted, trust me, it's easier than dealing with the many states of knitr chunk options.)

Example

I've added tests/manual/sql.Rmd as an example of a SQL exercise. A minimum version of a SQL exercise looks like this:

---
title: "SQL Demo"
output: learnr::tutorial
runtime: shiny_prerendered
---

```{r setup, include=FALSE}
library(learnr)

# Create an ephemeral in-memory RSQLite database
# Using the example in <https://dbi.r-dbi.org/#example>
mtcars$name <- rownames(mtcars)
mtcars <- mtcars[union("name", names(mtcars))]

# Open a connection to the database
db_con <- DBI::dbConnect(RSQLite::SQLite(), dbname = ":memory:")
# here we have to add our own data but your database might come with some of that
DBI::dbWriteTable(db_con, "mtcars", mtcars)
```

## A SQL Exercise

### Demo Exercise

Select 4-cylinder cars from the `mtcars` table.
Only include the columns `name`, `mpg`, and `cyl`.

```{sql db, exercise = TRUE, connection = "db_con"}
SELECT name, mpg, cyl FROM mtcars WHERE cyl = 4
```

Note that connection is required and it must be a string: connection = "db_con". Using the bare symbol will result in an error.

You don't need to include the output.var option on the exercise chunk, but if you do — e.g. output.var = "my_result" — it will be available inside envir_result. (In gradethis, you'd be able to get it via .envir_result$my_result.)

Technical Notes

This PR adds a few packages to the dependencies of learnr. I tried to find ways of minimizing those additions, but I don't see a way around them:

  1. We need methods::is() to be able to check if an object is a DBIConnection, since those are S4 methods. Since this is a base package, it won't count against us. Turns out that inherits() works fine in this situation.
  2. We need DBI and RSQLite in Suggests so that we can test that sql exercise evaluations works as expected. I added skip_if_not_installed() for both packages to those tests, but unfortunately they need to be in Suggests or R CMD Check will issue a warning. (We could get extra crafty but I'm not sure its worth the effort.)

Also, I had to spend a lot of time in exercise.R so I did a little re-organizing to help maintain my sanity. (f971aca (#668), b20ca76 (#668), f347f80 (#668))

I made knitr_engine_caption() a static export (that we immediately re-import) because it's also used by gradethis.

Speaking of gradethis, rstudio/gradethis#290 is the sibling PR that improves grading for non-R language exercises (and is required for tests/manual/sql.Rmd).

@gadenbuie gadenbuie marked this pull request as ready for review March 2, 2022 20:51
Copy link
Contributor

@rossellhayes rossellhayes left a comment

Choose a reason for hiding this comment

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

This looks great, I mostly just had a few questions!

R/exercise.R Outdated Show resolved Hide resolved
Comment on lines +833 to +841
if (is_exercise_engine(exercise, "sql")) {
rmd_src_user <- c(
rmd_src_user,
"",
'```{r eval=exists("___sql_result")}',
'get("___sql_result")',
"```"
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

As we add support for more engine, will this become a selector that calls from a list of helper functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? I think that it'll be a little more clear as we start testing more engines. In this case we're appending a chunk, but we might also need to prepend -- or maybe even use an entirely different template. So we'll see.

R/exercise.R Outdated Show resolved Hide resolved
R/knitr-hooks.R Show resolved Hide resolved
@gadenbuie gadenbuie merged commit 5b55ce1 into main Mar 14, 2022
@gadenbuie gadenbuie deleted the fix-chunk-knitr-options branch March 14, 2022 17:53
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.

Chunk options aren't preserved for SQL exercise chunks
2 participants