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

Pre-work leading up to difficult parallelism issues #241

Merged
merged 4 commits into from
Feb 6, 2018

Conversation

wlandau
Copy link
Member

@wlandau wlandau commented Feb 6, 2018

This PR fine-tunes the building and caching infrastructure, and it should make #227, #233, #237, and #239 much easier to solve. I also think it solves #236 to the full extent possible before direct implementation begins for #237. Details:

  • Restructure build_target() so that it does not need access to the cache. Add a unit test to track.
  • Do some polishing on drake_build() and store_target(). The logic is clearer, more readable, and more modular. It will help us to write a version of drake_build() that lets the master process do the caching (Manual scheduling #227, Add future support for the Makefile backend. #237).
  • Put all the staged parallelism functions in staged_parallelism.R. Similarly put other functions in the files where they belong.

This PR polishes some super sensitive internals, so regular unit testing is not good enough. I will do the pre-CRAN long-running tests on all backends and then test on an old work project before I merge this tomorrow.

@codecov-io
Copy link

codecov-io commented Feb 6, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #241   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          61     63    +2     
  Lines        4352   4360    +8     
=====================================
+ Hits         4352   4360    +8
Impacted Files Coverage Δ
R/parallel.R 100% <ø> (ø) ⬆️
R/random.R 100% <ø> (ø) ⬆️
R/parLapply.R 100% <100%> (ø) ⬆️
R/dependencies.R 100% <100%> (ø) ⬆️
R/staged_parallelism.R 100% <100%> (ø)
R/mclapply.R 100% <100%> (ø) ⬆️
R/future_lapply.R 100% <100%> (ø) ⬆️
R/utils.R 100% <100%> (ø)
R/envir.R 100% <100%> (ø) ⬆️
R/commands.R 100% <100%> (ø)
... and 6 more

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 1b00fe2...27f6c30. Read the comment docs.

@wlandau
Copy link
Member Author

wlandau commented Feb 6, 2018

The longer, more thorough checks look good too. Merging.

@wlandau wlandau merged commit a6fd190 into master Feb 6, 2018
@wlandau wlandau deleted the i236-plus-cleanup branch February 6, 2018 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants