cmu-delphi / epipredict

Tools for building predictive models in epidemiology.
https://cmu-delphi.github.io/epipredict/
Other
8 stars 9 forks source link

`print` implementations should not mix output to stdout and messages from `cli` functions #277

Closed brookslogan closed 9 months ago

brookslogan commented 9 months ago

E.g., mixing cat (printing to output stream) and {cli} functions ([issuing messages that knitr intercepts or] printing to message stream) in print.epi_recipe and print.frosting leads to some trouble with knitting documents.

E.g., trying to rebuild forecast-framework.qmd in delphi-tooling book has this in a chunk with #| collapse: true earlier in the document for printing an epi_recipe:

#> ── Operations
#> 1.
#> Lagging: case_rate by 0, 7, 14 | Trained
#> 2.
#> Lagging: death_rate by 0, 7, 14 | Trained
#> 3.
#> Leading: death_rate by 7 | Trained
#> 4.
#> • Removing rows with NA values in: lag_0_case_rate, ... | Trained
#> 5.
#> • Removing rows with NA values in: ahead_7_death_rate | Trained
#> 6.
#> • # of recent observations per key limited to:: Inf | Trained

or, when printing a frosting with some other set of options:

2023-12-15-023636_759x588_scrot

It also messes with capture.output, which only captures one stream at a time (ordering is lost).

Potential fixes:

brookslogan commented 9 months ago

Workaround for capture.output at REPL:

with_message_in_stdout <- function(code) {
  sink(stdout(), type = "message")
  on.exit({
    sink(type = "message")
  })
  code
}

capture.output(with_message_in_stdout(print(extract_recipe(out_gb$epi_workflow))))
#> ...
#> [14] "1. Lagging: case_rate by 0, 7, 14 | Trained"                                  
#> ...

But this doesn't seem to fix the qmd/knitr issues; in this context, the streams still appear to be separate, and capture.output can only capture one or the other at a time. [And I'm not sure how to apply this setting in .qmd.]

brookslogan commented 9 months ago

Workaround for .qmd:

withCallingHandlers(
  print(extract_recipe(out_gb$epi_workflow)),
  message = function(m) {cat(m$message); tryInvokeRestart("muffleMessage")}
)

[See here for people also suggesting this approach and discussion + link to the NA setting I couldn't figure out how to apply.]

dajmcdon commented 9 months ago

I've tried dealing with this in various ways in the past. The version in {recipes} was somewhat recently updated but {workflows} still mixes: https://github.com/tidymodels/workflows/blob/main/R/workflow.R.

A while back I'd found an open issue for this problem (cli resulting in breaks via knitr html) in one of the tidy packages, but I can't find it now.

dajmcdon commented 9 months ago

@rachlobay This happens in your #274 as well.