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

Remove superfluous parallel backends #648

Merged
merged 34 commits into from
Jan 3, 2019
Merged

Remove superfluous parallel backends #648

merged 34 commits into from
Jan 3, 2019

Conversation

wlandau
Copy link
Member

@wlandau wlandau commented Jan 3, 2019

Summary

  • Remove all backends accept "clustermq", "future", "hasty", and "future_lapply_staged".
  • @kendonB, "future_lapply_staged" parallelism is our secret. It's still there, but it is not officially supported or even in the documentation. I still intend to remove it ASAP, but probably not for 7.0.0. I simplified it down to 68 lines of code, which is not too terrible in the short term (R/future_lapply_staged.R).
  • Build all the imports before all the targets. This greatly simplifies the internals.
  • The parallelism and jobs arguments to make() are now officially scalars (cc @bpbond). Use the jobs_preprocess argument to parallelize the imports and other elements of the preprocessing.
  • Use automatic staged parallelism for the imports, based on parLapply() on Windows and mclapply() on more programmer-friendly systems.

Related GitHub issues and pull requests

Checklist

  • I have read drake's code of conduct, and I agree to follow its rules.
  • 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.

@codecov-io
Copy link

codecov-io commented Jan 3, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #648    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          80     73     -7     
  Lines        6223   5611   -612     
======================================
- Hits         6223   5611   -612
Impacted Files Coverage Δ
R/triggers.R 100% <ø> (ø) ⬆️
R/priority_queue.R 100% <ø> (ø) ⬆️
R/test_scenarios.R 100% <ø> (ø) ⬆️
R/handlers.R 100% <ø> (ø) ⬆️
R/utils.R 100% <ø> (ø) ⬆️
R/drake_debug.R 100% <ø> (ø) ⬆️
R/imports.R 100% <100%> (ø)
R/build_times.R 100% <100%> (ø) ⬆️
R/check.R 100% <100%> (ø) ⬆️
R/deprecate.R 100% <100%> (ø) ⬆️
... and 15 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 ea06a91...3e0250c. Read the comment docs.

@wlandau wlandau merged commit 889357c into master Jan 3, 2019
@bpbond
Copy link
Contributor

bpbond commented Jan 3, 2019

Seems very in keeping with move to lighter, tighter, faster package.

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.

5 participants