Repository navigation
Simplify progressr - #1313
Simplify progressr#1313VisruthSK wants to merge 27 commits into
Conversation
This uses the [`progressr`](https://progressr.futureverse.org/index.html) framework to enable a progress bar for sampling operations. By default, this replaces standard iteration messages, but not other informative messages produced during sampling. From a user point of view, this adds two arguments to the `$sample()` method: - `show_progress_bar`: Default = FALSE If TRUE, registers a progress bar via `progressr` and signals an update for every output line that matches "Iteration:". The user is responsible for registering those progress updates with an appropriate handler. - `suppress_iteration_messages`: Defaults to the value of show_progress_bar, but can also be set directly. If TRUE, disables display of output lines matching "Iteration:", while still causing the progress bar to update. This keeps all of Stan's other informative output, but just removes the superfluous iteration messages when using a progress bar. I've tried to keep all additions to the code as non-intrusive as possible. One minor decision I made was to pass the value of `refresh` from the `$sample()` method through to the `CmdStanProcs` class so that the progress bar can report and update the number of steps in the bar to the same 'scale' as the number of iterations. (Calling `$sample()` with `iter_sampling=8000` and `refresh=100` will result in 80 'ticks' of the progress bar, each increasing the progress by 100.) By default, `progressr` doesn't register a handler to display the progress bar, as this is the responsibility of the user. Multiple packages can be used to display the progress bar. A default progress bar can be registered using the [`cli`](https://cli.r-lib.org/) library in this way: ```r library(progressr) library(cli) handlers( global=TRUE ) handlers("cli") options( cli.spinner = "moon", cli.progress_show_after = 0, cli.progress_clear = FALSE ) handlers( handler_cli( format = "{cli::pb_spin} Progress: |{cli::pb_bar}| {cli::pb_current}/{cli::pb_total} | {cli::pb_percent} | ETA: {cli::pb_eta}", clear = FALSE )) ``` A variety of alternative progress bar handlers are available, including audible and notification-based handlers, and ones that interact with RStudio's jobs pane: <https://progressr.futureverse.org/articles/progressr-11-handlers.html>
In addition to updating the progress bar on stdout lines that match "Iteration:", the CmdStanProcs object(s) now pass the current stdout line to the progress bar as a message for _every_ line, even if the number of completed iterations hasn't increased.
Added a `register_default_progress_handler()` function to `utils.R` that creates a default progress bar for sampling operations and registers it as global handler for progressr. Requires `cli` and `progressr`.
Was accidentally passing `refresh` to each informational progress bar update, rather than the calculated `progress_amount`. This caused each message line to update the progress bar, even if it didn't match an iteration message.
Moved code to close the progress bar to after the `check_finished()` function.
The nature of CmdStan's output makes pulling appropriate refresh and update values for the progress bar slightly tricky. In the original verison of the code, this led to the bar occasionally updating past the point where it was full, or terminating early due to an incorrectly calculated number of steps. This commit reworks the calculations of updates and refresh values for the progress bar, as well as adding some extra conditions to avoid the bar potentially crashing in odd scenarios.
New arguments for `iter_warmup` and `iter_sampling` were not being assigned a default value if not specified in sampling statement.
- `iter_{warmup,sampling}` arguments to MCMCProcs are allowed to be 0.
- Documentation for `register_default_progress_handler` added to
repository.
- Ensured that calls to `progressr` functions are appropriately
prefixed.
Added documentation for `show_progress_bar` and `suppress_iteration_messages` in `sample()` function. Changed `require()` calls for `progressr` and `cli` to suggested `requireNamespace()` alternatives.
|
TO BE CLOSED |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1313 +/- ##
==========================================
- Coverage 92.08% 92.05% -0.03%
==========================================
Files 15 15
Lines 6570 6672 +102
==========================================
+ Hits 6050 6142 +92
- Misses 520 530 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks, this is a big improvement!
I'm thinking this should wait until right after v1.0 or perhaps for RC-2, just so we don't rush it. After you make the changes below (unless you object to any of them) I think you should just go ahead and commit right onto the #1138 branch and close this PR, and we'll merge #1138 after the release or before the release with the second release candidate.
The comments below were partially written by Claude after I did a back and forth with it going over the changes and telling it what I preferred.
1. Signal once per poll, not once per line. update_progress_() signals for every iteration line, so with refresh = 1 a terminal handler redraws for every line: on my side 4 chains of bernoulli take 2 s at refresh = 100 with the bar, 6 s at refresh = 1 with no bar and the lines hidden, 12 s with the txtprogressbar handler and 67 s with the cli handler (the same thing I saw on #1138). Sum the deltas over the lines in one process_output() call and signal once after the loop, which brings the cli case to about 7 s, the cost of reading the lines at all. We poll every 0.1 s so the bar stays smooth. The roxygen sentence "The progress bar itself adds negligible overhead" then goes: what's true is that small refresh values slow fast models because we read every line, bar or not.
2. Verbose mode loses the iteration lines. The !(iteration_line && suppress) sits outside the || is_verbose_mode(), so options(cmdstanr_verbose = TRUE) with the bar on prints no iteration lines, where before it printed everything. Set ignore_line <- TRUE for a hidden iteration line next to the other ignore_line rules and leave the print condition as it was.
3. Drop the refresh = 0 check. With refresh = 0 CmdStan prints no iteration lines, so no progressor is ever created and nothing breaks. Delete both copies and the test, and say in the docs that refresh = 0 shows no bar.
4. The missing-progressr error. require_suggested_package() says "to use this function", which for someone with options(cmdstanr_progress_bar = TRUE) in their .Rprofile and no progressr is every $sample() call failing with a message that names neither the argument nor the option. An inline stop() here naming show_progress_bar and options(cmdstanr_progress_bar).
5. The @param show_progress_bar entry. Keep it to what it does, that it's experimental, the default and option (in the save_metric wording), and that nothing shows until a progressr handler is registered, for example progressr::handlers(global = TRUE) once per session. The rest (one bar for all chains, iteration lines hidden while it's on, refresh = 0 shows none, small refresh slows fast models, options(cmdstanr_progress_bar = interactive()) for notebooks) can go in a short "Progress bar" section on $sample(). Cut "CmdStanR does not install or modify progression handlers" and "progressr controls whether and how it is displayed", and the two semicolons and the "e.g.".
6. Tests. Drop "CmdStan iteration output is parsed" (the progress test covers the helper, and its NULL case can't happen because the grepl("Iteration:") filter runs first) and "finish_progress finishes the progressor once" (it reaches into .__enclos_env__$private, and a second finish is harmless). In "sample progress follows CmdStan's cumulative iteration count" use chains = 2 so the per-chain bookkeeping and the step count are exercised, and after item 1 assert the total and the sum of the amounts rather than the per-line amounts, which batching makes timing-dependent. In "sample can suppress CmdStan iteration messages" use iter_warmup = 10, iter_sampling = 10, refresh = 5 so it doesn't raise three warnings. The refresh = 0 test goes with item 3.
7. NEWS. The branch has no entry. Something like: $sample() can show a progress bar through the progressr package with show_progress_bar = TRUE or options(cmdstanr_progress_bar = TRUE). (#1138)
8. parse_cmdstan_iteration() has one caller and its NULL branch repeats the grepl("Iteration:") check the caller already passed. Fold the regex into update_progress_() with one guard (a user print() containing "Iteration:" can reach it), or give it an @noRd block if it stays.
9. Small things. Backticks rather than single quotes around code in error messages (moot after item 3). A few added lines over 80 characters (R/run.R print condition, two test lines). CmdStanMCMCProcs$initialize() repeats the method defaults and both callers pass both arguments, so make them required.
|
Thanks! Just pushed to #1138. |
4 participants
Submission Checklist
Summary
Please describe the purpose of the pull request.
Copyright and Licensing
Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
INSERT COPYRIGHT HOLDER HERE
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: