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

add arg 'sanitize_targets' to 'build_drake_graph()' #342

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ChrisMuir
Member

ChrisMuir commented Mar 24, 2018

Summary

When function drake_config() is called, the targets get sanitized at the top of the function with targets <- sanitize_targets(plan, targets). If arg graph = NULL, then the targets end up getting sanitized a second time at the top of function build_drake_graph(). To avoid running the targets through sanitize_targets() twice, I've added arg sanitize_targets to function build_drake_graph() (default value is TRUE). Then if build_drake_graph() is called within drake_config(), it's called with sanitize_target = FALSE.

The new arg operates the same way as existing arg sanitize_path.

GitHub issues fixed

  • No associated issue.

Checklist

  • I have read drake's code of conduct, and I agree to follow its rules.
  • I have read the guidelines for contributing.
  • I have listed any substantial changes in the development news.
  • I have added testthat unit tests to tests/testthat to confirm that any new features or functionality work correctly.
  • I have tested this pull request locally with devtools::check()
  • This pull request is ready for review.
  • I think this pull request is ready to merge.

All tests passed locally after making these edits:

devtools::test()
Loading drake
Use suppressPackageStartupMessages() to eliminate package startup messages like this one.
Testing drake| OK F W S | Context| 36       | local_parLapply_1: basic [19.1 s]
√ | 118       | local_parLapply_1: cache [7.4 s]
√ |  8       | local_parLapply_1: command changes [5.7 s]
√ | 35       | local_parLapply_1: console [0.8 s]
√ | 45       | local_parLapply_1: custom caches [2.0 s]
√ |  7       | local_parLapply_1: dbi cache [3.4 s]
√ | 41       | local_parLapply_1: dependencies [2.9 s]
√ | 59       | local_parLapply_1: deprecation [2.2 s]
√ | 32       | local_parLapply_1: edge cases [9.8 s]
√ |  6       | local_parLapply_1: envir [2.1 s]
√ | 32       | local_parLapply_1: examples [2.2 s]
√ |  3       | local_parLapply_1: expose imports [2.1 s]
√ | 39       | local_parLapply_1: full build [3.3 s]
√ | 13       | local_parLapply_1: future [16.2 s]
√ | 36       | local_parLapply_1: generate [3.4 s]
√ | 41       | local_parLapply_1: graph [18.9 s]
√ | 19       | local_parLapply_1: hash [1.6 s]
√ |  9       | local_parLapply_1: import file [3.7 s]
√ | 10       | local_parLapply_1: import object [7.2 s]
√ |  6       | local_parLapply_1: intermediate file [3.0 s]
√ | 51       | local_parLapply_1: knitr [1.6 s]
√ | 19       | local_parLapply_1: lazy load [7.1 s]
√ | 46       | local_parLapply_1: Makefile [5.3 s]
√ | 52       | local_parLapply_1: memory cache [4.5 s]
√ | 32       | local_parLapply_1: migrate [13.0 s]
√ |  9       | local_parLapply_1: namespaced [1.0 s]
√ | 94       | local_parLapply_1: other features [5.8 s]
√ | 36       | local_parLapply_1: parallel [7.6 s]
√ | 29       | local_parLapply_1: queue [0.2 s]
√ | 26       | local_parLapply_1: reproducible random numbers [2.8 s]
√ | 14       | local_parLapply_1: retries and timeouts [4.2 s]
√ | 12       | local_parLapply_1: strings| 58       | local_parLapply_1: testing [0.9 s]
√ |  3       | local_parLapply_1: tidy eval [0.5 s]
√ | 49       | local_parLapply_1: time [16.3 s]
√ | 41       | local_parLapply_1: triggers [22.1 s]
√ | 83       | local_parLapply_1: workflow plan [3.4 s]

== Results =====================================================================
Duration: 214.1 s

OK:       1249
Failed:   0
Warnings: 0
Skipped:  0
@lintr-bot

This comment has been minimized.

lintr-bot commented Mar 24, 2018

R/graph.R:30:24: style: Trailing whitespace is superfluous.

sanitize_plan = TRUE, 
                       ^
@codecov-io

This comment has been minimized.

codecov-io commented Mar 24, 2018

Codecov Report

Merging #342 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #342   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          66     66           
  Lines        5232   5234    +2     
=====================================
+ Hits         5232   5234    +2
Impacted Files Coverage Δ
R/config.R 100% <100%> (ø) ⬆️
R/graph.R 100% <100%> (ø) ⬆️

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 262e0d8...1774e15. Read the comment docs.

@wlandau

This comment has been minimized.

Collaborator

wlandau commented Mar 25, 2018

Thanks for the PR. I really appreciate these kinds of tune-ups.

In this case, though, I am not sure about the direction. In the short term, I think we could lump all sanitation into a single argument, either sanitize (a small API change, extremely unlikely to break any workflows) or just the existing sanitize_plan.

As it turns out, the sanitize_plan argument to build_drake_graph() did not actually do much when I implemented it. I think it was a poor design choice on my part. It complicated the API and still left redundant calls to sanitize_plan().

library(drake)
load_basic_example()
debug(drake:::sanitize_plan)
make(my_plan)
## Browse[2]> c
## Browse[2]> c
## Browse[2]> c
## Browse[2]> c

I think we might consider adding a special attribute to plans we sanitize so we do not re-sanitize them. We could remove that attribute at the beginning of drake_config() to make sure we always sanitize each plan once. Targets are sanitized less frequently, so we may not need to do anything similar for sanitize_targets() besides this PR.

@ChrisMuir

This comment has been minimized.

Member

ChrisMuir commented Mar 25, 2018

Ah, this all makes sense. Yeah I also considered the idea of adding a "sanitized" attribute to the plan objects, set to FALSE upon creation, and then flip it to TRUE after the first pass through the sanitize functions. That would handle the issue nicely, but I don't know the code base well enough to know whether adding new attributes would have other implications or consequences.

Feel free to close this if you'd like, thanks for the feedback Will!

wlandau added a commit that referenced this pull request May 5, 2018

@wlandau

This comment has been minimized.

Collaborator

wlandau commented May 5, 2018

It's great to have this for reference, but I think I'll close. At some point, I'll open an issue about repeated calls to sanitize_plan() and other minor inefficiencies throughout drake.

@wlandau wlandau closed this May 5, 2018

@ChrisMuir

This comment has been minimized.

Member

ChrisMuir commented May 6, 2018

👍 No problem Will!

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