Params should be reference classes

[DarwinAwardWinner]

Looking at how bpstart and bpstop work, it looks like one must do:

param <- bpstart(param)

If one simply does bpstart(param), then param will still be set to an object representing a stopped cluster, and the reference to the started cluster will be lost (possibly leaking resources by leaving a bunch of new processes running but inaccessible).

I think we could make bpstart and bpstop work without assignment by storing the reference to the backend inside an environment in the object, since environments are pass-by-reference.

What do you think?

[mtmorgan]

I agree with the problem. I think BiocParallelParam (and derived) should be reference classes, so a little bit more extensive change.

[DarwinAwardWinner]

Here's a trivial start of implementing a param class hierarchy as reference classes. I mostly did this to figure out how to write reference classes. Seems pretty straightforward once you know the conventions for initializers and callSuper and such.

BiocParallelParamRef <- setRefClass("BiocParallelParamRef",
                                    contains="VIRTUAL",
                                    fields=list(.controlled="logical", workers="integer"),
                                    methods=list(initialize=function(.controlled=TRUE, workers=0, ...) callSuper(.controlled=.controlled, workers=as.integer(workers), ...)))

SerialParamRef <- setRefClass("SerialParamRef",
                              contains="BiocParallelParamRef")

# Only difference is it reports 2 workers
DummyParallelParamRef <- setRefClass("DummyParallelParamRef",
                                     contains="BiocParallelParamRef",
                                     methods=list(initialize=function(...) callSuper(.controlled=TRUE, workers=2, ...)))

x <- DummyParallelParamRef$new()
print (x$workers)

[DarwinAwardWinner]

I'm now experimenting with reference classes here: https://github.com/DarwinAwardWinner/BiocParallel/blob/refclass/R/BiocParallelParam-refclass.R

Anyone with knowledge of how reference classes are actually supposed to be written should feel free to critique me.

[DarwinAwardWinner]

I've just done some more work toward implementing ref classes here: https://github.com/DarwinAwardWinner/BiocParallel/tree/refclass

However, I'm having trouble getting the package to load with my modifications, so I can't test it. The problem is that in the .onLoad function the calls to both SnowParam() and MulticoreParam() fail with:

  error: attempt to apply non-function

However, I put in print statements like print(SnowParam); print(class(SnowParam)) and everything seems to indicate that they are functions. If I can get past this issue then I can start testing my reference-class based implementation.

[DarwinAwardWinner]

Well, I just replaced the onLoad body with register(SerialParam()) which gets me past that hump, and I'm happy to report that I appear to have a working implementation of reference-based parallel param classes!

If someone could help me debug the onLoad problem, we can incorporate this soon.

[mtmorgan]

You can debug by adding browser() to .onLoad and installing with

R CMD INSTALL --no-test-load BiocParallel

Then when you say library(BiocParallel) you end up in browser, ready to fail! I find that if I create a simple class setRefClass("A") somewhere in the code, and then try in the browser in .onLoad to, e.g., getRefClass("A") I see the error message you mention, so it seems like a bug in the methods package.

[DarwinAwardWinner]

So, I've done some testing, and the onLoad error only occurs for me when R is run with --vanilla. (R CMD check runs with --vanilla, which is why it triggers the error as well.)

[DarwinAwardWinner]

After further testing, I have determined that the package can only be loaded after loading the "scales" package. I have no idea why.

[mtmorgan]

On 05/14/2013 05:11 PM, Ryan Thompson wrote:

After further testing, I have determined that the package can only be loaded
after loading the "scales" package. I have no idea why.

Something is not being initialized correctly in the methods package; I think
I've had intermittent success with a simple package with only

A <- setRefClass("A")
.onLoad <- function(...) A()

Try experimenting with a simpler rather than more complicated (by adding scales)
package...? Also it is possible to use options(error=recover) to figure out
what is going wrong, though the code is quite complicated...

—
Reply to this email directly or view it on GitHub
#5 (comment).

Computational Biology / Fred Hutchinson Cancer Research Center
1100 Fairview Ave. N.
PO Box 19024 Seattle, WA 98109

Location: Arnold Building M1 B861
Phone: (206) 667-2793

[DarwinAwardWinner]

Well, I got things to work in R 3.0.0: 6087e62

[DarwinAwardWinner]

My fix also works in R 2.15.3.

[mtmorgan]

this

https://github.com/DarwinAwardWinner/BiocParallel/blob/6087e62513153e3a86fc971582efa688da569184/R/zzz.R

is a work-around that should be addressed by narrowing to a simple reproducible example that can be reported to the R-devel mailing list.

[DarwinAwardWinner]

Yes indeed, I will do that when I have the chance. Since the bug only seems to occur in the onLoad function of a package, should I just create a minimal package that triggers the bug and send a download link to the R-devel list?

[DarwinAwardWinner]

Hmm. It seems that I am not constructing reference objects correctly. I thought you had to do ClassName$new(), when in fact you apparently just have to do ClassName(). As far as I can tell, both forms are equivalent except that the former triggers this bug and the latter doesn't.

So basically I am only seeing the bug because I was doing it wrong. Is it still worth reporting?

[DarwinAwardWinner]

Actually never mind. I'm getting inconsistent results, so now I don't know what's going on. I will investigate further, but for now, disregard my previous comment.

[DarwinAwardWinner]

https://bugs.r-project.org/bugzilla3/show_bug.cgi?id=15322

[DarwinAwardWinner]

Looks like the solution is to use setLoadAction instead of defining a .onLoad function: https://bugs.r-project.org/bugzilla3/show_bug.cgi?id=15322#c1

I'll try this out and see if it fixes things.

[DarwinAwardWinner]

Ok, I switched it over: c9a5c03

Everything seems to work now.

[mtmorgan]

On 05/27/2013 05:36 PM, Ryan Thompson wrote:

Ok, I switched it over: c9a5c03
c9a5c03

Everything seems to work now.

OK great, thanks for pursuing this; I'll merge this in the next two days. Martin

—
Reply to this email directly or view it on GitHub
#5 (comment).

Computational Biology / Fred Hutchinson Cancer Research Center
1100 Fairview Ave. N.
PO Box 19024 Seattle, WA 98109

Location: Arnold Building M1 B861
Phone: (206) 667-2793

[DarwinAwardWinner]

Ok, in the mean time, I will work on eliminating any warnings related to missing documentation and other cleanup. I'll post here when I think it's ready to merge.

[DarwinAwardWinner]

Ok, I'd say it's ready to merge. With this and my other pull request, there are no more warnings from R CMD check.

[mtmorgan]

Should have closed this with the pull request; thanks.