Conversation
| # to free memory from terminated processes | ||
| if (private$task_loop_counter %% 20 == 0) { | ||
| gc(verbose = FALSE, reset = FALSE, full = TRUE) | ||
| } |
There was a problem hiding this comment.
Turns up, forcing garbage collection in each step was responsible for the 70% increase in process runtime. Now it is executed every 20 passes which is still quit often, certainly frequent enough to not run out of the processes, but at the same time does not extend maintenance process too much.
There was a problem hiding this comment.
Maybe this should just live in process$finish() and when self$is_done() so that it only runs when we're cleaning up terminated processes.
R/checker.R
Outdated
There was a problem hiding this comment.
There was a typo in that condition (missing list extraction) makin it always return FALSE and, as a result, mark meta task as done too early (one of the nodes finish, not when both)
R/reporter_ansi_tty.R
Outdated
There was a problem hiding this comment.
There is a lot to uncouple here. First of all, the original condition was not working as the second part !v$name %in% reporter$buffer$node was always returning true. That's because buffer data.frame is using node IDs, and not names, their look for names was always returning false (negated to TRUE). However, even after fixing it, it was still not behaving as expected, or more like not covering all the necessary nodes. Because it was leaving unaddressed all the nodes that were finished recently, but already were in the buffer for a few iterations, ergo they were not new for the buffer but still had updates to report. Therefore, I reworked it to cover either those not in buffer, or those that are in buffer but were not marked as final, meaning they are still subject to change.
| reportable_lines[seq(which.max(reportable_lines) + reporter$buffer_height, | ||
| length(reportable_lines))] <- FALSE | ||
| } | ||
|
|
There was a problem hiding this comment.
The core of this change. Make sure that lines marked as final are not redrawn and are ignored when in the buffer. Thanks to that, we can have some kind of a moving window over the buffer that, at given times, redraws only the max height of the tty at once.
| )))) | ||
| })) | ||
|
|
||
| cat(failures_output, "\n") |
There was a problem hiding this comment.
Failures should be displayed after the process ends (either finished or is interrupted) to not waste precious interactive buffer space.
| #' @export | ||
| report_step.reporter_ansi_tty <- function(reporter, checker) { | ||
| checker$step() | ||
| checker$start_next_task() >= 0 |
There was a problem hiding this comment.
Using a step is a double-edged sword because it makes the reporting a bit faster (fewer cli calculations), but also really spaced out, especially at the beginning, which kinda negates the purpose of the reporter at all.
|
@dgkf Hello! I'm not sure how much you want to be involved in this project at this stage, but as this feature is your "child", I found that you might be interested in these changes. That said, don't feel any pressure to contribute if you don't need/want/can. I'll leave this open for a while, then merge if you don't have anything to add. |
dgkf
left a comment
dgkf
left a comment
There was a problem hiding this comment.
Happy to still provide occasional feedback!
I focused on the parts with comments and skimmed the rest. Left some suggestions, but don't feel like you need to satisfy my requests to merge.
| # to free memory from terminated processes | ||
| if (private$task_loop_counter %% 20 == 0) { | ||
| gc(verbose = FALSE, reset = FALSE, full = TRUE) | ||
| } |
There was a problem hiding this comment.
Maybe this should just live in process$finish() and when self$is_done() so that it only runs when we're cleaning up terminated processes.
That's lovely! Thank you very much. And the comments sound really reasonable, so I will definitely look at them. By the way, what do you generally think about that idea of that moving window to address ansi tty height limits? |
Co-authored-by: Doug Kelkhoff <18220321+dgkf@users.noreply.github.com>
…nentech/checked into 78-add-error-messaging-to-reporters-2
2 participants
When implementing error messaging, I encountered a serious issue with the current ansi tty design. Bacilly, whenever there were enough packages to make the buffer exceed the max tty height, the approach of returning n lines to redraw the line no longer works, because any value returned higher than the max height was trimmed to the max height. Therefore, I decided to rework it, introducing the concept of final lines. Those are lines that are no longer subject to change and thus can be removed from the interactive redrawing. Also, the buffer got changed into a moving window of active lines, which is the height of the tty-1. Meaning that at any point, the output includes all finalized lines, starting from the header, plus a window of 60 subsequent lines, which can be interactive. Whenever the number of consecutive finalized lines increases, the window moves. Of course, all processes, including those connected to not-yet-displayed lines, are still running in the background; they are simply temporarily not displayed because there is no room for them.