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

New imported function does not trigger downstream builds. #548

Closed
tiernanmartin opened this issue Oct 13, 2018 · 5 comments
Closed

New imported function does not trigger downstream builds. #548

tiernanmartin opened this issue Oct 13, 2018 · 5 comments

Comments

@tiernanmartin
Copy link
Contributor

tiernanmartin commented Oct 13, 2018

Hi Will,

Sorry for dropping off the map a bit. Despite being overwhelmed by a tsunami of new work I am still very interested in developing a set of best practices for drake plans as R packages.

On that note, I believe that the recent improvements to dependency detection (#522) have broken the workflow that I have been developing. I want to store my command functions in a package and make() the plan in a different environment, but this seems to result in a dependency graph that doesn't include the functions in the package.

Is there a workaround that you can recommend? [edit: I now realize that I can explicitly namespace every function from my package but this is cumbersome]

Is there a way to declare a particular package -- or set of packages -- as part of a plan's dependency graph?

Thanks!

@wlandau
Copy link
Member

wlandau commented Oct 13, 2018

Sorry about that. Have you tried expose_imports() on your package? I think that's actually a better solution than namespacing every single function because then you get nested functions too.

@tiernanmartin
Copy link
Contributor Author

Thanks for the quick suggestion!

Unfortunately I haven't been able to get expose_imports() to work.

I tried to follow the example included in the documentation but I got the following results (psuedo code):

# after editing the `make_target1()` function and reinstalling `mypackage`

library(mypackage)

expose_imports("mypackage")

make(plan(t1 = make_target1()))
## All targets are already up to date.

Alternatively, I also tried passing cache to make() but that resulted in all of the functions in my package being outdated and remade (psuedo code):

library(mypackage)

expose_imports("mypackage")

cache <- storr::storr_environment()

make(plan(t1 = make_target1()), cache = cache)
## target target1
## target target2
## target target3
## ... (all targets are re-made -- regardless of whether they were edited or not)

Am I misunderstanding how expose_imports() is intended to be used?

@wlandau
Copy link
Member

wlandau commented Oct 13, 2018

You are doing everything right. The problem comes from a bug in the implementation of #507. I will push a fix soon.

Details

The preprocessing is now memoized in order to reduce overhead, but I messed up. It turns out that adding a new import no longer triggers a fresh analysis of targets for dependencies. You can run the following and see. Notice the missing "analyze 5 targets" message in the second call to drake_config().

library(drake)
library(drakepkg)
library(ggplot2)
config <- drake_config(plan_example, verbose = 4)
expose_imports(drakepkg)
config <- drake_config(plan_example, verbose = 4)
vis_drake_graph(config)

For those of you looking on, the above code assumes you have installed Tiernan's drakepkg and your working directory is drakepkg/inst.

@wlandau
Copy link
Member

wlandau commented Oct 13, 2018

Now patched in dev. Please let me know if this issue persists.

The patch contains a test that fails before and succeeds after so this will not happen again.

test_with_dir("add a new import", {
plan <- drake_plan(a = as.integer(sqrt(4)))
cache <- storr::storr_environment()
config <- make(plan, cache = cache, session_info = FALSE)
expect_equal(justbuilt(config), "a")
expect_equal(readd(a, cache = cache), 2L)
sqrt <- function(x){
x + 1L
}
config <- make(plan, cache = cache, session_info = FALSE)
expect_equal(justbuilt(config), "a")
expect_equal(readd(a, cache = cache), 5L)
})

@tiernanmartin
Copy link
Contributor Author

That fixed it 👍

Thanks!

@wlandau wlandau changed the title Missing dependencies in packaged drake plans New imported function does not trigger downstream builds. Oct 14, 2018
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