Fix 2 capture output #16

kodesiba · 2022-03-08T19:12:47Z

Add support for capturing messages, output, and results. Closes #2 and Closes #7

codecov · 2022-03-08T19:54:53Z

Codecov Report

❗ No coverage uploaded for pull request base (dev@90c9d00). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head c27f72c differs from pull request most recent head 6f00c49. Consider uploading reports for the commit 6f00c49 to get more accurate results

@@          Coverage Diff           @@
##             dev      #16   +/-   ##
======================================
  Coverage       ?   74.19%           
======================================
  Files          ?        7           
  Lines          ?      372           
  Branches       ?        0           
======================================
  Hits           ?      276           
  Misses         ?       96           
  Partials       ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90c9d00...6f00c49. Read the comment docs.

nicholas-masel

I've added update suggestions for your consideration.

I have one general comment/question as well.

Executinng a script with redirect give us this:

Rscript tests/testthat/ref/safely_quietly_test_file.R > tests/testthat/ref/safely_quietly_test_file.log 2>&1

[1] "log print"
[1] "log print 2"
log catlog cat 2log inform
log inform 2
log message
log message 2
Warning message:
log warning 
Warning message:
log warning2 
Error: log error
Execution halted

The order of messages, warnings and the error provide context for troubleshooting a batch submit.

The current approach breaks this up into pieces and reports them separately, so the context is lost.

Not sure how yet, but what are your thoughts about trying to put the exact output from this redirect under the timber header? So users still get exactly what they are used to getting, but with a nice timber header as a bonus.

nicholas-masel · 2022-03-09T19:10:41Z

R/writer.R

@@ -107,7 +107,7 @@ write_log_header <- function(title_string){
 write_errors <- function() {
   errors <- get_log_element("errors")

-   paste0("errors:\n\t",
+   paste0("Errors:\n\t",
          capture.output(errors))


Suggested change

capture.output(errors))

capture.output(errors$message))

tests/testthat/ref/safely_quietly_test_file.R

R/writer.R

nicholas-masel · 2022-03-09T19:53:15Z

R/writer.R

+#' @return A formatted vector of results
+#' @export
+#'
+write_result <- function() {


Do we have a use case where the script returns a result rather than writing out a file to capture the results. If so, this might be good to highlight in documentation.

nicholas-masel · 2022-03-09T20:13:20Z

R/writer.R

+   paste0("\nResult:\n",
+          paste0(result, collapse = "\n\t"))


This can get tricky since result could be anything; dataframe, model object, etc. What do you think about letting print for each object handle how we present it in the log?

Suggested change

paste0("\nResult:\n",

paste0(result, collapse = "\n\t"))

capture.output(print(result$value))

tests/testthat/test-writer.R

Co-authored-by: nicholas-masel <61123199+nicholas-masel@users.noreply.github.com>

kodesiba · 2022-03-11T15:41:53Z

@nicholas-masel I updated the safely/quietly piece to correctly use the purrr adverbs here as well per your previous comment

…pture_output

Merge branch 'dev' of github.com:atorus-research/timber into fix-2_capture_output # Conflicts: # NAMESPACE # R/zzz.R # tests/testthat/test-log.R

…pture_output

nicholas-masel · 2022-03-23T20:18:36Z

@kodesiba, So I've got a proposal to get around the warning and sink by creating a custom purrr::capture_output. I have copied the function here and updated the following:

Commented out both invokeRestart() calls. This will make sure the warning and messages are stored by purrr and written out to the console/log.
Added split = TRUE to sink(). This will make sure the output are stored by purrr and written out to the console/log.
The last puzzle piece, set options(warn = 1). This will make the warnings immediately print, rather than print out of order.

options(warn = 1)

capture_output <- function(code) {
  warnings <- character()
  wHandler <- function(w) {
    warnings <<- c(warnings, w$message)
    # invokeRestart("muffleWarning")
  }
  
  messages <- character()
  mHandler <- function(m) {
    messages <<- c(messages, m$message)
    # invokeRestart("muffleMessage")
  }
  
  temp <- file()
  # sink(temp)
  sink(temp, split = TRUE)
  on.exit({
    sink()
    close(temp)
  })
  
  result <- withCallingHandlers(
    code,
    warning = wHandler,
    message = mHandler
  )
  
  output <- paste0(readLines(temp, warn = FALSE), collapse = "\n")
  
  list(
    result = result,
    output = output,
    warnings = warnings,
    messages = messages
  )
}

nicholas-masel · 2022-03-23T20:37:25Z

@kodesiba Or we put in a PR to extent purrr with a new functionloudly(), that captures results, outputs, warnings and messages in a list, but still puts them in the console/log for everyone.

kodesiba · 2022-03-24T14:06:32Z

@nicholas-masel I think that making our own internal function to do it would be a solid fix for now. we could definitely put in a PR to purrr but not sure what their turnaround time is. I would love to do that at some point as this might actually be an interesting new case for purrr adverbs

I've been looking at the same thing as this actually and I think your solution is great. We can set the option in our zzz.R so it gets loaded at the beginning. Happy to implement this in this branch today.

Merge branch 'dev' of github.com:atorus-research/timber into fix-2_capture_output # Conflicts: # NAMESPACE

kodesiba · 2022-03-24T20:24:21Z

@nicholas-masel this is finally ready for review. The way that purrr does the adverbs looks to mean that we just have to write one function due to how the package gets build. Let me know if you have any other comments or suggestions for this PR

bms63 · 2022-03-25T00:36:17Z

R/utils-execution.R

+#'
+#' @importFrom purrr as_mapper
+#'
+#' @param code all code to be run loudly


Suggested change

#' @param code all code to be run loudly

#' @param code All code to be run loudly

bms63 · 2022-03-25T00:36:33Z

R/utils-execution.R

+#'
+#' @param code all code to be run loudly
+#'
+#' @return wrapped function returns a list with components


Suggested change

#' @return wrapped function returns a list with components

#' @return wrapped Function returns a list with components

bms63 · 2022-03-25T00:37:34Z

R/writer.R

@@ -8,9 +8,6 @@
 #' @return formatted element including prefix


Suggested change

#' @return formatted element including prefix

#' @return Formatted element including prefix

R/zzz.R

bms63 · 2022-03-25T00:41:16Z

tests/testthat/test-interact.R

-   log_config(file = scriptPath)
-
-   run_safely_n_quietly(scriptPath)
+   run_safely_loudly(fp)

   testthat::expect_true(!is.null(getOption("timber.log")[["warnings"]]))


Do we need the directs calls to testthat:: if that package is loaded when you Run the Tests?

Not necessarily, we will need to do a cleaning and include checking consistency with stuff like this. It can be either way we'll just want to pick one at some point.

admiral doesn't have these calls so we should remove them to keep in line with what they are doing. I can make a separate issue for us to address?

yeah, go ahead and log an issue for that as well as one for checking Roxygen docs for consistency (not sure if Admiral had a style guide or anything)

bms63

Hey Nathan. Looks good. I just had a few minor comments. Looking forward to testings this out wants this and the other PR gets merged into dev

kodesiba · 2022-03-25T14:57:34Z

@nicholas-masel this is ready for final review. Adding checks for zzz.R don't do anything since testthat loads timber at the beginning and that stuff isn't really checkable within the testthat framework

nicholas-masel

@kodesiba Nicely done!

Nathan Kosiba added 2 commits March 8, 2022 16:54

initial updates for capturing messages, output, and results

8022396

update example for vignette

7943c22

kodesiba requested review from bms63 and thomasp-b March 8, 2022 19:13

Nathan Kosiba added 2 commits March 8, 2022 19:51

fix unit tests

82d52e3

update file extensions to be consistent

fc7a428

kodesiba requested a review from nicholas-masel March 8, 2022 21:25

nicholas-masel reviewed Mar 9, 2022

View reviewed changes

kodesiba and others added 7 commits March 10, 2022 11:07

Update R/writer.R

6afbf4c

Co-authored-by: nicholas-masel <61123199+nicholas-masel@users.noreply.github.com>

Update tests/testthat/test-writer.R

bf44e9a

Co-authored-by: nicholas-masel <61123199+nicholas-masel@users.noreply.github.com>

Update tests/testthat/ref/safely_quietly_test_file.R

84ad5fd

Co-authored-by: nicholas-masel <61123199+nicholas-masel@users.noreply.github.com>

Update R/writer.R

e13358f

Co-authored-by: nicholas-masel <61123199+nicholas-masel@users.noreply.github.com>

add updates to commited changes to fix failing unit tests

1296416

remove extraneous examples

b93e5d0

update run_safely_quietly() to use proper purrr adverb standards

ae66ac1

Nathan Kosiba added 3 commits March 11, 2022 16:10

Merge branch 'dev' of github.com:atorus-research/timber into fix-2_ca…

3f9536b

…pture_output

Add log_stream for discussion

439cf94

udpate failing pieces of package check

d4c399c

kodesiba changed the base branch from main to dev March 18, 2022 19:04

Merge branch 'dev' into fix-2_capture_output

e4f46a4

kodesiba requested a review from nicholas-masel March 18, 2022 19:08

Nathan Kosiba added 3 commits March 18, 2022 19:49

ffwd to dev

914f518

Merge branch 'dev' of github.com:atorus-research/timber into fix-2_capture_output # Conflicts: # NAMESPACE # R/zzz.R # tests/testthat/test-log.R

ffwd extra stuff

4cb0c05

Merge branch 'dev' of github.com:atorus-research/timber into fix-2_ca…

50addb0

…pture_output

update to include new functions loudly()

544cc91

ffwd to dev

60d0a36

Merge branch 'dev' of github.com:atorus-research/timber into fix-2_capture_output # Conflicts: # NAMESPACE

update log_config test to remove elements moved out of log_config

6f00c49

This was linked to issues Mar 24, 2022

Add stdout logging support #7

Closed

capture message, warnings, and (possibly?) errors in {timber} log #2

Closed

bms63 reviewed Mar 25, 2022

View reviewed changes

R/zzz.R Show resolved Hide resolved

bms63 reviewed Mar 25, 2022

View reviewed changes

bms63 approved these changes Mar 25, 2022

View reviewed changes

nicholas-masel approved these changes Mar 25, 2022

View reviewed changes

kodesiba merged commit 842da57 into dev Mar 25, 2022

bms63 deleted the fix-2_capture_output branch April 1, 2022 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 2 capture output #16

Fix 2 capture output #16

kodesiba commented Mar 8, 2022 •

edited

Loading

codecov bot commented Mar 8, 2022 •

edited

Loading

nicholas-masel left a comment •

edited

Loading

nicholas-masel Mar 9, 2022

nicholas-masel Mar 9, 2022

nicholas-masel Mar 9, 2022

kodesiba commented Mar 11, 2022

nicholas-masel commented Mar 23, 2022

nicholas-masel commented Mar 23, 2022

kodesiba commented Mar 24, 2022

kodesiba commented Mar 24, 2022

bms63 Mar 25, 2022

bms63 Mar 25, 2022

bms63 Mar 25, 2022

bms63 Mar 25, 2022

kodesiba Mar 25, 2022

bms63 Mar 25, 2022

kodesiba Mar 25, 2022

bms63 left a comment

kodesiba commented Mar 25, 2022

nicholas-masel left a comment

	paste0("\nResult:\n",
	paste0(result, collapse = "\n\t"))
	capture.output(print(result$value))

	#' @param code all code to be run loudly
	#' @param code All code to be run loudly

	#' @return wrapped function returns a list with components
	#' @return wrapped Function returns a list with components

		@@ -8,9 +8,6 @@
		#' @return formatted element including prefix

Fix 2 capture output #16

Fix 2 capture output #16

Conversation

kodesiba commented Mar 8, 2022 • edited Loading

codecov bot commented Mar 8, 2022 • edited Loading

Codecov Report

nicholas-masel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kodesiba commented Mar 11, 2022

nicholas-masel commented Mar 23, 2022

nicholas-masel commented Mar 23, 2022

kodesiba commented Mar 24, 2022

kodesiba commented Mar 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bms63 left a comment

Choose a reason for hiding this comment

kodesiba commented Mar 25, 2022

nicholas-masel left a comment

Choose a reason for hiding this comment

kodesiba commented Mar 8, 2022 •

edited

Loading

codecov bot commented Mar 8, 2022 •

edited

Loading

nicholas-masel left a comment •

edited

Loading