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

Migration to batchtools #64

Closed
mfansler opened this issue Aug 2, 2017 · 12 comments
Closed

Migration to batchtools #64

mfansler opened this issue Aug 2, 2017 · 12 comments
Assignees

Comments

@mfansler
Copy link

mfansler commented Aug 2, 2017

Are there any plans to migrate away from BatchJobs to its successor, batchtools?

If not, would a contributed PR be accepted? For instance, one could write a BatchToolsParam class paralleling the BatchJobsParam one, and then eventually mark the BatchJobsParam as deprecated (e.g., throws a warning when using) once the BatchToolsParam has been verified to be stable across different platforms.

@mtmorgan
Copy link
Collaborator

mtmorgan commented Aug 2, 2017

A pull request would be great incentive for this, yes.

@DarwinAwardWinner
Copy link

Based on the existence of future.batchtools, I suggest it might be better to write a backend for future and get batchtools support for free. The downside is that such a solution might not support the more advanced features of batchtools, so it depends on what batchtools features BiocParallel needs.

@DarwinAwardWinner
Copy link

Actually, I just noticed that @HenrikBengtsson appears to have already done so: https://github.com/HenrikBengtsson/BiocParallel.FutureParam. So maybe BiocParallel already supports batchtools through the future package.

@mfansler
Copy link
Author

mfansler commented Aug 4, 2017

@DarwinAwardWinner Nice! I hadn't seen that library previously. Rather new, but looks promising.

I still think it would be useful to integrate such functionality directly into BiocParallel rather than having an additional dependency.

@mllg
Copy link
Collaborator

mllg commented Aug 4, 2017

I don't have time to migrate this myself in the next 2-3 month, but I could export functions and complement missing helpers in batchtools if someone wants to start working on a PR.

@HenrikBengtsson
Copy link
Contributor

HenrikBengtsson commented Aug 11, 2017

I'm willing to contribute the code of BiocParallel.FutureParam to the BiocParallel package. That way BiocParallel would support all backends that implement the Future API, including future.batchtools (and hence batchtools). All the end user would do is register(FutureParam) and select what future backend to use via future::plan(). One can of course come up with a register() call that does both in one go, but to keep it simple, I haven't done so for now.

BTW, in lieu of FutureParam, a working solution to utilize (future.)batchtools in BiocParallel is to go via doFuture and DoParParam, e.g.

library("BiocParallel")
register(DoparParam(), default = TRUE)
doFuture::registerDoFuture()

library("future.batchtools")
plan(batchtools_local)

y <- bplapply(X = 1:3, FUN = function(x) list(x = x, pid = Sys.getpid()))
str(y)
## List of 3
##  $ :List of 2
##   ..$ x  : int 1
##   ..$ pid: int 26553
##  $ :List of 2
##   ..$ x  : int 2
##   ..$ pid: int 26554
##  $ :List of 2
##   ..$ x  : int 3
##   ..$ pid: int 26555

PS. This solution illustrates the goal of the future package; to bridge parallel frontends with parallel backends by providing a backend Future API that parallel backends implement and a frontend Future API that higher-level APIs implement.

@DarwinAwardWinner
Copy link

I guess the question is, is there any special functionality provided by batchtools that BiocParallel needs/wants to access and which is not exposed through the Future API? If not, then I would consider this pretty much solved.

@mtmorgan
Copy link
Collaborator

It seemed like the future api was also committing to a particular programming style (of unevaluated expressions)?

@HenrikBengtsson
Copy link
Contributor

HenrikBengtsson commented Aug 11, 2017

There's

f <- future({ expr })

and

f <- futureCall(FUN, args)

Is that what you're looking for, or did you mean something else? The other two key components of the frontend Future API are

resolved(f)

to check if future is resolved or not, and

v <- value(f)

to collect the value (or signal error if it occurred). The idea is that those are the building blocks for higher level needs. There's also resolved(fs) and values(fs) which operate on a set of futures, e.g. a list of futures.

Certain features such as stopping ("killing") futures are not part of the frontend Future API. This might be added in the future, cf. futureverse/future#93 (long story).

UPDATE 2017-11-16: The civis package implements a CivisFuture for their services and added their own cancel() method in lieu of an official one for the Future API. So, even if I haven't figured out exactly how this should look like in the Future API, it can certainly be added under some contrains/assumptions.

@HenrikBengtsson
Copy link
Contributor

Coming back to this one. Not sure if my most recent reply helped or caused confusion.

From the end user's point of view, futures just provide another backend that the user can register for BiocParallel, i.e. register(FutureParam). Everything else will work the same. It should be 100% compatible with the BiocParallel API.

For the actual internals of the FutureParam backend, there are a few alternatives, but my existing working proposal shows how it can be done. There might be features in BiocParallel that I'm not aware of and that's missing, but the package tests against the existing BiocParallel API that I'm aware of.

Let me know if you want me to do a PR of what I've got.

@nturaga nturaga self-assigned this Feb 23, 2018
@nturaga
Copy link
Contributor

nturaga commented Mar 22, 2018

Hi,

There is now a BatchtoolsParam-class implemented with some basic functionality. It is on pull request #71 , which was just merged with master.

Please take a look.

Best,

Nitesh

@nturaga nturaga closed this as completed Mar 22, 2018
@nturaga nturaga reopened this Mar 22, 2018
@nturaga
Copy link
Contributor

nturaga commented May 6, 2018

Hi, the latest extension has been merged. Please take a look at #72.

@mfansler If you feel like this addresses your issue. Please close it.

@mfansler mfansler closed this as completed Feb 4, 2021
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

No branches or pull requests

6 participants