Closed brownag closed 3 months ago
I am going to add some reprexes here; starting with this suggestion from @njtierney on #39
The following works interactively, in a reprex, or similar, but will not work if example_fun_fun()$write
is passed to tar_format()
I wonder if we even actually need to modify the function body directly? I'm fairly sure those parts will be evaluated appropriately?
E.g.,
example_funfun <- function(sep_type){
.write_fun <- function(object, file){
# loudly declare the quote type when writing
readr::write_lines(x = object,
file = file,
sep = sep_type)
cat("Hi! The separation marker is: '", sep_type,"'", sep = "")
cat("\n")
cat("We wrote the file out to:\n", file, sep = "")
}
return(
list(
read = readr::read_csv,
write = .write_fun
)
)
}
my_example <- example_funfun(",")
# sep_type isn't evaluated yet
my_example$write
#> function(object, file){
#> # loudly declare the quote type when writing
#> readr::write_lines(x = object,
#> file = file,
#> sep = sep_type)
#> cat("Hi! The separation marker is: '", sep_type,"'", sep = "")
#> cat("\n")
#> cat("We wrote the file out to:\n", file, sep = "")
#> }
#> <environment: 0x12a2669e0>
# but sep_type gets evaluated later when the function is used:
my_example$write(
object = "1,2,3,4",
file = tempfile(fileext = ".csv")
)
#> Hi! The separation marker is: ','
#> We wrote the file out to:
#> /var/folders/9c/k3wqmhhx4qsb3fd66n4prhlw0000gq/T//RtmpUdvv1a/file104a0120aafb7.csv
Created on 2024-03-16 with reprex v2.1.0
Or is there some deeper stuff happening with targets here?
_Originally posted by @njtierney in https://github.com/njtierney/geotargets/pull/39#discussion_r1527125475_
Compare this to running the same process via {targets}
library(targets)
tar_script({
tar_custom_writecsv <- function(name,
command,
pattern = NULL,
sep_type = NULL,
tidy_eval = targets::tar_option_get("tidy_eval"),
packages = targets::tar_option_get("packages"),
library = targets::tar_option_get("library"),
repository = targets::tar_option_get("repository"),
iteration = targets::tar_option_get("iteration"),
error = targets::tar_option_get("error"),
memory = targets::tar_option_get("memory"),
garbage_collection = targets::tar_option_get("garbage_collection"),
deployment = targets::tar_option_get("deployment"),
priority = targets::tar_option_get("priority"),
resources = targets::tar_option_get("resources"),
storage = targets::tar_option_get("storage"),
retrieval = targets::tar_option_get("retrieval"),
cue = targets::tar_option_get("cue")) {
name <- targets::tar_deparse_language(substitute(name))
envir <- targets::tar_option_get("envir")
command <- targets::tar_tidy_eval(
expr = as.expression(substitute(command)),
envir = envir,
tidy_eval = tidy_eval
)
pattern <- targets::tar_tidy_eval(
expr = as.expression(substitute(pattern)),
envir = envir,
tidy_eval = tidy_eval
)
example_funfun <- function(sep_type) {
.write_fun <- function(object, path) {
# loudly declare the quote type when writing
readr::write_lines(x = object,
file = file,
sep = sep_type)
cat("Hi! The separation marker is: '", sep_type,"'", sep = "")
cat("\n")
cat("We wrote the file out to:\n", file, sep = "")
}
return(
list(
read = function(path) readr::read_csv(path),
write = .write_fun
)
)
}
fmt <- targets::tar_format(
read = example_funfun(sep_type)$read,
write = example_funfun(sep_type)$write
)
targets::tar_target_raw(
name = name,
command = command,
pattern = pattern,
packages = packages,
library = library,
format = fmt,
repository = repository,
iteration = iteration,
error = error,
memory = memory,
garbage_collection = garbage_collection,
deployment = deployment,
priority = priority,
resources = resources,
storage = storage,
retrieval = retrieval,
cue = cue
)
}
list(tar_custom_writecsv(test_writecsv, cars, sep_type = "|"))
})
tar_make()
#> ▶ dispatched target test_writecsv
#> ✖ errored target test_writecsv
#> ✖ errored pipeline [0.346 seconds]
#> Error:
#> ! Error running targets::tar_make()
#> Error messages: targets::tar_meta(fields = error, complete_only = TRUE)
#> Debugging guide: https://books.ropensci.org/targets/debugging.html
#> How to ask for help: https://books.ropensci.org/targets/help.html
#> Last error message:
#> _store_ object 'sep_type' not found
#> Last error traceback:
#> No traceback available.
#> Backtrace:
#> ▆
#> 1. └─targets::tar_make()
#> 2. └─targets:::callr_outer(...)
#> 3. ├─targets:::if_any(...)
#> 4. └─targets:::callr_error(traced_condition = out, fun = fun)
#> 5. └─targets::tar_throw_run(message, class = class(traced_condition$condition))
#> 6. └─targets::tar_error(...)
#> 7. └─rlang::abort(message = message, class = class, call = tar_empty_envir)
x <- tar_read(test_writecsv)
#> Error: '_targets/objects/test_writecsv' does not exist in current working directory ('/tmp/RtmpxNHiyN/reprex-691145ccf529-stuck-crane').
x
#> Error in eval(expr, envir, enclos): object 'x' not found
I don't think that will work, or at least I tried it before arriving at the body<- solution I proposed in #11 here. Tried doing it again now because I hoped I had missed something.
What you propose works fine interactively in your global environment, or in a reprex, etc. but the context where those
tar_format()
functions get evaluated you will not have the e.g.sep_type
object defined.Essentially the functions
tar_format()
uses need to be self-contained, not relying on the prior parent frame. You actually wantsep_type
to be evaluated sooner as it is when we modify the function body.If you implement your suggestion for
tar_terra_rast()
you will get something like this:
Loading required namespace: terra
▶ dispatched target test_terra_rast
✖ errored target test_terra_rast
✖ errored pipeline [0.13 seconds]
Error:
! Error running targets::tar_make()
Error messages: targets::tar_meta(fields = error, complete_only = TRUE)
Debugging guide: https://books.ropensci.org/targets/debugging.html
How to ask for help: https://books.ropensci.org/targets/help.html
Last error message:
_store_ object 'filetype' not found
Last error traceback:
No traceback available.
Some other ideas I tried were using purrr::partial()
to compose functions with some arguments pre-specified and couldn't get that working. Also forcing evaluation and injecting values with {rlang} metaprogramming operators also did not appear to work, but that might be worth re-investigating now.
_Originally posted by @brownag in https://github.com/njtierney/geotargets/pull/39#discussion_r1527184740_
I'm curious if you've run into this issue before @wlandau?
In the functions supplied to tar_format()
, it looks like sep_type
is a global variable stored in the closure of the function. These functions are deparsed so they can be stored accurately and compactly in the metadata, so the closure is lost. I would substitute()
the value of sep_type
into the function body instead.
Replacement of a function body does not seem to work properly within the evaluation environment created by combination of {covr}, targets tests with
tar_test()
, and {testthat}. We dynamically modify our internal functions usingbody<-
to customize the functions passed totar_format()
.Interactive runs, local runs, runs of {testthat} locally and remotely will all work fine, but when running tests via {covr} the problem arises.
I did some digging the other day into this issue, and there is a great vignette on how {covr} works behind the scenes: https://cran.r-project.org/web/packages/covr/vignettes/how_it_works.html
From the vignette:
My suspicion is whatever {covr} does to modify the code during its processing causes a situation where no modification to the
tar_format()
read/write function occurs when we callbody<-
.To solve this, I think we need to isolate the bug in a reprex and report it to {covr} maintainers. Probably this is not intended behavior, since other evaluation contexts have no apparent issues. In the meantime we could possibly find a way to "protect" or exclude the specific functions from being modified by {covr}.
I have tried several different alternatives to current approach--including converting function body to string and modifying the string, replacing the whole function; using an intermediate object; storing the function in a different environment and modifying it there--and no method of replacement with
body<-
seems to work.These tests indicate to me it is not something simple like that the {covr} modified function body has more than 2 elements, or that the names of the arguments are missing, or that it is evaluating in the wrong environment. It seems to mealmost like there is a copy of the function made, and it corresponds to the initial function definition, and the body calls do not get applied to the right version of the function, which is what is ultimately passed to
tar_format()
@Aariq had some good thoughts on covr related issues worth following up on:
Originally posted by @Aariq in https://github.com/njtierney/geotargets/issues/31#issuecomment-2000067999