-
Notifications
You must be signed in to change notification settings - Fork 12
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
BETA: Global progress handler #95
Comments
I tried to follow your instructions but I get
which was not the case before. Any idea what went wrong and how to fix it? |
@Bisaloo, thanks for the feedback. That error message comes from an internal sanity check. I'd like to be able to reproduce this. Can you give me a bit more details, e.g.
|
No, I get
traceback
sessionInfo
RStudio You can probably reproduce this issue with: remotes::install_github("ropensci/lightr@progressr-globalhandlers")
library(progressr)
register_global_progression_handler()
library(lightr)
lr_get_spec(system.file("testdata", package = "lightr"), ext = "jdx") |
Thanks, I misunderstood your initial "I tried to follow your instructions" to mean that the example in #95 (comment) failed. Do you mind confirming that at least that one works for you? |
Ah yes, this was confusing indeed. I confirm the example in the OP works perfectly for me. |
Thxs. I can reproduce the error with your package example. |
I can reproduce the problem with: library(progressr)
options(progressr.demo.delay = 0.2)
options(progressr.clear = FALSE)
foo <- function(steps = 3L) {
p <- progressor(steps + 1L)
y <- future.apply::future_lapply(1:steps, function(n) {
p()
slow_sum(1:2, stdout=TRUE, message=FALSE)
})
}
register_global_progression_handler()
foo()
## |========================================| 100%
## Error: 'is.null(stdout_file)' is not TRUE with the end of the traceback being:
The problem goes away if I allow for one more progressor step, e.g. This means I've got something to work with to fix this. |
…se active sinks and the stdout_file connection due to other, blocking sinks [#95]
I think I fixed it. @Bisaloo, please re-install the develop version and retry. Details: The problem was that the internal sanity check assumed that the sink+connection used to buffer stdout could be closed at any time, e.g. when the progressor reaches 100%. This was not possible when it reached 100% inside a future because the future had another sink active blocking the progressors sink from being close. Here's a simpler reproducible example of this problem without futures; library(progressr)
options(progressr.demo.delay = 0.2)
register_global_progression_handler()
local({
p <- progressor(1L) ## this sets up a sink to capture and buffer stdout
bfr <- utils::capture.output({
p() ## this completes and closes the progressor but it can't close
## its sink because of the active capture.output()
slow_sum(1:2, stdout=TRUE, message=FALSE)
})
}) |
Yes, I can confirm it is fixed for me. Thank you very much for the quick fix! |
Instead of the end-user having to call: register_global_progression_handler() I'm thinking of providing # Enable
handlers(global = TRUE)
# Disable
handlers(global = FALSE) |
I've pushed support for: # Enable
> handlers(global = TRUE)
# Disable
> handlers(global = FALSE)
# Query
> handlers(global = NA)
[1] FALSE Next, I'll remove |
…andlers(global=TRUE/FALSE/NA) instead [#95]
I've updated the README and the vignette (they're the same) to mainly use global progress handling in all of the examples. |
Finally managed to run revdep checks where all package tests are run with `progressr::handlers(global = TRUE) - took some hacking. The good news is that is no negative impact to enable the global progression handler. |
I've implemented support for registering a global condition handler (requires R >= 4.0.0) that will listen to progress updates (formally 'progression' conditions) and render them. When used, there is no need for
with_progress()
. As the below example illustrates, it buffers and flushes standard output and messages (and warnings). Just like before, it also works when parallelizing using the future framework.Here's an example:
To disable the global handlers, use:
Please give it a spin by installing the develop branch;
Tasks
Test it in real-world scenarios:
The name
register_global_progression_handler()
is a bit much and should be renamed to something shorter. If someone got a better name, please let me know. UPDATE 2020-12-04: We're now usinghandlers(global = TRUE)
andhandlers(global = FALSE)
for this.An alternative is to make
handlers("void")
the default, which will also unregister any global progression handler. When setting any other progression handler(s), sayhandlers("progress")
, then the global progression handler will be automatically registered as well. This way the end-user interface will be to usehandlers()
.The text was updated successfully, but these errors were encountered: