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

MRC-538: remove use of context/queuer #9

Merged
merged 12 commits into from Oct 14, 2019
Merged

MRC-538: remove use of context/queuer #9

merged 12 commits into from Oct 14, 2019

Conversation

richfitz
Copy link
Member

@richfitz richfitz commented Oct 3, 2019

This PR removes use of the context and queuer packages from rrq. Doing this will allow us to do two things:

  • submit the package to CRAN without worrying about a long dependency chain
  • re-implement the bulk submission (lapply - like interface) in a way that fits better with the new environment creation

The bulk of the new code here was directly copied in from context / queuer with simplifications to remove unused code branches. The code in expression.R will change when we do bulk submission, and includes a couple of options that are not (yet) exposed to enqueue. The progress bar bits are all a bit tedious, but nice for interactive use.

@richfitz richfitz requested a review from r-ash October 3, 2019 07:36
@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #9    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          14     16     +2     
  Lines         849    955   +106     
======================================
+ Hits          849    955   +106
Impacted Files Coverage Δ
R/rrq_controller.R 100% <100%> (ø) ⬆️
R/expression.R 100% <100%> (ø)
R/worker_run.R 100% <100%> (ø) ⬆️
R/utils.R 100% <100%> (ø) ⬆️
R/worker_messages.R 100% <100%> (ø) ⬆️
R/worker_spawn.R 100% <100%> (ø) ⬆️
R/time.R 100% <100%> (ø)
R/worker.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 83f4811...066fc98. Read the comment docs.

This reduces the worker overhead by reducing the number of total
roundtrips taken; we now have

- BLPOP while waiting for job ($poll)
- one pipeline while retrieving job (worker_run_task_start)
- one pipeline while returning job (worker_run_task_cleanup)
Copy link
Contributor

@r-ash r-ash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great

@@ -0,0 +1,63 @@
## Adapted from queuer
time_checker <- function(timeout) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a neat little function

@r-ash r-ash merged commit e30d45d into master Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants