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

use a single-line progress bar, addresses #1880 #2035

Closed
wants to merge 17 commits into from

Conversation

mariusbarth
Copy link

@mariusbarth mariusbarth commented Aug 12, 2021

Hi all,

along the lines of #1880, this is an attempt at creating a more compact progress bar.

  • creates a globally accessible, bespoke progress bar knit_progress()
  • create set_knit_progress() to update value and/or printed text
  • also use cat() (instead of message() for printing information about processed files, to prevent mixing of streams from stdout and stderr

If knitr::opts_knit$set(verbose = TRUE), new lines are added between the following lines:

processing file:  test.rmd
 chunk: setup                              |..............                  |  43%
 text                                      |..................              |  57%
 chunk: cars                               |..................              |  57%
 summary(cars)                             |..................              |  57%
 x <- sample(2e6)                          |.......................         |  71%
 text                                      |...........................     |  86%
 chunk: pressure                           |...........................     |  86%
 plot(pressure)                            |...........................     |  86%
 x <- sample(2e6)                          |................................| 100%
 text                                      |................................| 100%
output file:  test.knit.md 

If verbose = FALSE, code bits are not printed, and all lines (except proceccing file: test.rmd and output file: test.knit.md) are overwritten. I hope this is quiet close to what @hadley had in mind.

To avoid messages messing up the console (and the progress bar), I now use capture.output(..., type = "message") to first capture messages and then cat() them above the progress bar. I'm fully aware that this might be a bit too much, but I couldn't come up with a better solution. EDIT: I came up with a better solution, a proper message handler, and could switch back to withCallingHandlers().

If there are any necessary changes, just let me know.

- creates a globally accessible, bespoke progress bar `knit_progress()`
- create `set_knit_progress()` to update value and/or printed text
- also use `cat()` (instead of `message()` for printing information about processed files, to prevent mixing of streams from `stdout` and `stderr`
@CLAassistant
Copy link

CLAassistant commented Aug 12, 2021

CLA assistant check
All committers have signed the CLA.

@mariusbarth
Copy link
Author

mariusbarth commented Aug 12, 2021

I just added some more changes that are intended to better manage the different output streams, especially messages that are printed to the console while knitting.

If my .Rmd contains something like this:

message("A message that will maybe hint me to a problem.")
message("Another message from the same code chunk.")

it will now be printed in the console as such:
Bildschirmfoto von 2021-08-12 23-45-13

If verbose = TRUE, it looks like the following:
Bildschirmfoto von 2021-08-12 23-46-24


With the current code, it would be super easy to get similar output for warnings, which I think would be very helpful when debugging. However, this would only work if we set options(warn = 1) in the respective R session.

@yihui yihui added the next Issues/PRs considered for the next release label Aug 24, 2021
@etiennebacher
Copy link

Hello, I just stumbled on this PR, and I was wondering if you had thought of a way to include chunk options as well. I find it useful to show them in the current progress bar, e.g:

 |...............................                                       |  44%
label: reg-all (with options) 
List of 2
 $ cache  : logi TRUE
 $ results: chr "asis"

  |...................................                                   |  50%
  ordinary text without R code

  |.......................................  

Otherwise, this looks nice to me, so thanks

@mariusbarth
Copy link
Author

mariusbarth commented Sep 3, 2021

Hi @etiennebacher, it would definitely be possible to print chunk options, though it would require some more changes. The challenge is that what can be overwritten (i.e., "updated") is only the current line, so everything that is supposed to be updated has to fit into a single line of the console. Printing chunk options to this single line would most likely make it rather unwieldy, which is why I opted against adding them.

However, while working on the progress bar, I figured that the current implementation serves two rather distinct and partially conflicting purposes: The first being to inform about the processing status of the document, and the second to provide some kind of log. The new progress bar as I implemented it here (with verbose = FALSE) clearly prioritizes the "processing status" aspect (e.g., information is overwritten, everything fits into a single line). By contrast, the same does not apply to the verbose = TRUE option, and I think this might be the best place to print information about chunk options. If you (and maybe @yihui) agree, I would be more than happy to provide a draft for printing chunk options to the verbose = TRUE output.

@etiennebacher
Copy link

Well I don't have a clear-cut opinion on how chunk options info should be displayed and implemented, I just wanted to make sure that they wouldn't be forgotten in the process of updating progress indicators. I see that you have already thought about that and have some ideas on how to include them, so I'll step aside and let you and knitr maintainers discuss about that ;)

@yihui
Copy link
Owner

yihui commented Sep 4, 2021

@etiennebacher Thanks! I'll review this PR a couple of weeks later. For now, I just wonder if we could avoid copying code from base R.

@cderv
Copy link
Collaborator

cderv commented Sep 6, 2021

For the sake of at least mentioning them before committing to a decision, there are a few CRAN packages which offer progress bar tools:

There are probably other, but it interesting to know what is out there. Specifically because they have some options for the user to tweak the style and control the progress bar, while the developer (us) still controls what is reported and when.

Regarding the discussion above, I agree we should try to not copying directly from base R. I believe we could adapt the logic to our need, and implement using usual style of this package.

Here what I think we need and could do for the progress bar:

  • Use an environment to store state of the progress
  • Some functions would allow to create, init, update and clear the progress bar.
    • create would return the env with some default structure: pb <- knit_progress_create(...)
    • init would initiate the environment to set the init state of the progress: pb <- knit_progress_init(pb, ...)
    • update would modify the env values during the progression: pb <- knit_progress_update(pb, ...)
    • clear would would be called when progress is done to close the progress bar : knit_progress_clear(pb)
  • Another one would allow to print the state of the progress bar: knit_progress_show(pb, ...)

I guess the logic is similar than txtProgressBar() and other progress bar packages.

Anyway, just sharing thoughts on this. @yihui, feel free to reach out if you want to brainstorm more on this.

Small feedback on the current PR: That would be best if we can start avoiding using some stringr functions. That would help us later for #1549

@mariusbarth
Copy link
Author

mariusbarth commented Sep 13, 2021

Hi @cderv and @yihui,

I just removed the stringr dependency. Regarding the copied code from base R: It was my impression that neither base-R functions, nor any of the simple progress-bar packages, provides progress bars where a message is updated side-by-side with a status bar. The progress package could be used to accomplish this goal, but I figured with its dependency graph, it was a bit over to the top. Therefore, I chose to use modified base-R code, because this seemed to me the most solid foundation to start from.

@maxheld83
Copy link
Contributor

maxheld83 commented Dec 2, 2021

@mariusbarth:

However, while working on the progress bar, I figured that the current implementation serves two rather distinct and partially conflicting purposes: The first being to inform about the processing status of the document, and the second to provide some kind of log.

⬆️ this.

Perhaps it might make sense separate these two concerns:

  1. let knitr::knit(quiet = FALSE) control the emission of a log on stdout, including options, whatever else people find useful.
    (This would have always have to be knitr specific)
    This output can, even with knitr::opts_knit$set(verbose = FALSE) be more verbose, than people might want a progress bar to be.
  2. let a new knitr::knit(progress = FALSE) control the emission of a progressr::progressor() object, whose handling/presentation is determined by the caller (that's @HenrikBengtsson design).
    Via progress, this could default to the kind of condensed progress info specced in Rethink the progress bar #1880, perhaps simply set as a default option with something like:
    knitr::opts_knit$set(progress_handler = progressr::handlers(progressr::handler_progress())
    (I've separately logged implementing 2) in emit progressr::progressor() progress bar from knit() #2077)

This may conflicts/overlap with existing verbose and progress knitr options, though if I understand the progressr docs correctly, a progressr progress bar need not interfere with other stdout, but is simply presented underneath it.

@maxheld83
Copy link
Contributor

@cderv 👍

Here what I think we need and could do for the progress bar:

  • Use an environment to store state of the progress

  • Some functions would allow to create, init, update and clear the progress bar.

    • create would return the env with some default structure: pb <- knit_progress_create(...)
    • init would initiate the environment to set the init state of the progress: pb <- knit_progress_init(pb, ...)
    • update would modify the env values during the progression: pb <- knit_progress_update(pb, ...)
    • clear would would be called when progress is done to close the progress bar : knit_progress_clear(pb)
  • Another one would allow to print the state of the progress bar: knit_progress_show(pb, ...)

Isn't this what progressr does (already)?
I think it'd be great to reuse that.

@pedropark99
Copy link
Contributor

I want to invest some time to contribute in this PR. I understand from the discussion that packages for progress bar are available (like progress and progressr), but, they bring unwanted dependencies. Hence, a custom solution using minimal dependencies is more desirable. Because of it, I will use @mariusbarth work as a start;

I very much agree with @maxheld83 that the progress bar is being used for two distinct purposes (report progress, and, logging information), and, that these systems should be separated. I can try to build a logging system in the way, but, maybe it should be treated in a different PR, I guess....

To maintain the code standard of knitr, I translated some <- of @mariusbarth to equal signs =;

I like very much of @cderv suggestions to store the current state of the progress bar in a environment. And to reuse the code standard of knitr, I thought as a good idea, to use new_defaults() to build such environment. In other words, knit_progress_create() will create a environment using new_defaults(), and return such environment as a result;

Therefore, knit_progress_update() will use the $set() method to update the values in this environment, and knit_progress_show() will use the $get() method to use the variables in this environment to build and print the progress bar in the console;

For now, these ideas make sense to you? Did I understand something wrong? Is what you all meant in this discussion?

@cderv
Copy link
Collaborator

cderv commented Sep 16, 2022

@pedropark99 thanks for your interest. We are looking closer to the topic of progress bar for our next version. Main discussion is in #1880

We will take some time to think about the new design based on all the feedbacks for the different issues. Please do share your views so that we take it into account. Especially, we need to take time develop clearly the goals to identify the best path to tackle them before doing anything new in the code.

@yihui
Copy link
Owner

yihui commented Nov 22, 2022

Just a quick update: the current dev version of knitr uses a single-line progress bar now, and the PR #2196 allows users to choose their own custom progress bars. Feedback welcome!

@HenrikBengtsson
Copy link
Contributor

I've just verified that PR#2196 works with progressr. See my #2196 (comment) for how to set it up.

Regarding @mariusbarth's comment on:

If knitr::opts_knit$set(verbose = TRUE), new lines are added between the following lines: ...

Note with progressr, this is not a problem, because progressr buffers any stdout, messages, and warnings until the next progress update is rendered. This means you can use cat(), message(), ... just as when there wasn't any progress output to worry about (https://progressr.futureverse.org/#use-regular-output-as-usual-alongside-progress-updates).

@yihui yihui closed this in f440712 Nov 24, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
next Issues/PRs considered for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants