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

Avoid re-running targets if supplied args are the same as default args #705

Closed
richardbayes opened this issue Feb 1, 2019 · 3 comments
Closed

Comments

@richardbayes
Copy link

It would be great if drake didn't re-run targets if a command is changed by supplying an argument which is the same as the default argument. For instance, the following plans call a function with the same arguments, one of which uses the default arg for y and another which overrides the default argument for y with a value which is the same as the default argument. I think it would be nice if drake could determine that this target did not need to be re-run.

Example:

library(drake)
#> Warning: package 'drake' was built under R version 3.5.1
f <- function(x, y = 2) x + y
my_plan <- drake_plan(my_target = f(1))
#> Warning: package 'bindrcpp' was built under R version 3.5.1
make(my_plan)
#> target my_target
# modify plan which doesn't actually change function call
my_plan <- drake_plan(my_target = f(1, 2))
# can drake be smart enough not to re-run target? That would be nice!
make(my_plan)
#> target my_target

Created on 2019-02-01 by the reprex package (v0.2.1)

This feature would have been nice for a recent application where I used map_plan(args, my_func) and used some default arguments of my_func (i.e. args did not have a column for these default args). Later, I needed to run my_func for a few more scenarios which required having different values for a default argument value of my_func. Thus, I added an extra column to args where the majority of the extra columns matched the default arg, and the new scenarios had values different from the default values. All the targets were re-run although only the new scenarios needed to be updated.

An obvious workaround is to do something like map_plan(args2, my_func) for the new targets and add it to overall plan, but that will start looking messy for large and evolving projects where it is not known beforehand every combination of arguments which may need to be explored.

drake is great! Keep up the awesome work!

@wlandau wlandau self-assigned this Feb 1, 2019
@wlandau wlandau removed their assignment Feb 1, 2019
@wlandau
Copy link
Member

wlandau commented Feb 1, 2019

That's a great point. I agree, it would be nice to avoid rebuilding targets if all you did was set an argument to its default value.

I would like to come at this from a best practices angle. It is possible to sanitize your commands up front so they always include all the arguments.

# https://rdrr.io/cran/stackoverflow/src/R/match.call.default.R
match_call_defaults <- function(
  definition = sys.function(sys.parent()),
  call = sys.call(sys.parent()),
  expand.dots = TRUE,
  envir = parent.frame(2L)
) {
  call <- match.call(definition, call, expand.dots, envir)
  formals <- formals(definition)
  if(expand.dots && "..." %in% names(formals)) {
    formals[["..."]] <- NULL
  }
  for(i in setdiff(names(formals), names(call))) {
    call[i] <- list(formals[[i]])
  }
  match.call(definition, call, TRUE, envir)
}

f <- function(x, y = 2, z = 3) x + y

# We don't pass z...
my_call <- quote(f(10, 11))

# ... but the language object now has the default value.
match_call_defaults(f, my_call)
#> f(x = 10, y = 11, z = 3)

Created on 2019-02-01 by the reprex package (v0.2.1.9000)

Until #700 is solved, you may need to use parse() and deparse() to go between language objects and character strings.

Unfortunately, I do not plan to build this into drake directly because

  • Commands are arbitrary code chunks and not always individual function calls.
  • Matching all the calls in a command would require recursing through language objects, which is unavoidably slow. Not something I would want to do unless there was a clear benefit to all use cases.
  • drake is actually moving away from map_plan(). A MUCH better interface is coming out with version 7.0.0.

@wlandau wlandau closed this as completed Feb 1, 2019
@wlandau
Copy link
Member

wlandau commented Feb 1, 2019

I have tagged this issue as a frequently asked question, so it will automatically be listed in the FAQ next time there is a new commit to the master branch of https://github.com/ropenscilabs/drake-manual.

@wlandau
Copy link
Member

wlandau commented Feb 4, 2019

Related: #706

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants