Bug 17373 - parallel::mclapply doesn't use lapply for mc.cores == 1 & !mc.preschedule
Summary: parallel::mclapply doesn't use lapply for mc.cores == 1 & !mc.preschedule
Status: CLOSED FIXED
Alias: None
Product: R
Classification: Unclassified
Component: Misc (show other bugs)
Version: R-devel (trunk)
Hardware: All unix-other
: P5 minor
Assignee: R-core
URL:
Depends on:
Blocks:
 
Reported: 2018-01-10 11:50 UTC by Joan Maspons
Modified: 2018-01-12 15:11 UTC (History)
2 users (show)

See Also:


Attachments
Patch (952 bytes, patch)
2018-01-10 11:50 UTC, Joan Maspons
Details | Diff
updated patch (ensuring length(X) is correct/consistent) (2.49 KB, patch)
2018-01-11 09:49 UTC, Martin Maechler
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joan Maspons 2018-01-10 11:50:20 UTC
Created attachment 2313 [details]
Patch

mclapply doesn't follow documentation when !mc.preschedule and mc.cores == 1.

Details in the manual says: for mc.cores == 1 it simply calls lapply

This patch fixes the problem avoiding the overhead of forking
Comment 1 Martin Maechler 2018-01-11 09:49:51 UTC
Created attachment 2314 [details]
updated patch (ensuring  length(X) is correct/consistent)
Comment 2 Martin Maechler 2018-01-11 09:51:53 UTC
Thank you, Joan!

I agree that there's a problem here, mclapply() not calling lapply() in this case, even though the docs claim so.

However, your patch
does not check if mc.preschedule is true or false ... well because it moves  the two lines from the  'mc.preschedule == TRUE'  case.

On the other hand, we have later (then the if() check in your coase),

    ## Follow lapply
    if(!is.vector(X) || is.object(X)) X <- as.list(X)
    if(!is.null(affinity.list) && length(affinity.list) < length(X))
        stop("affinity.list and X must have the same length")

and I think that should be moved up very close to the beginning, notably because later  length(X)  is used  and the above    X <- as.list(X)  may have changed the length(X) .. thought not in typical cases.

At the moment, I'm a bit wary about changing this myself, because I cannot claim to be able considering all the possible use cases and also because I'm not aware of how much of this functionality is tested by R's own checks.

I will attach an updated patch -- having moved the above "Follow lapply" lines further up.
Comment 3 Joan Maspons 2018-01-11 11:05:59 UTC
Hello.

It's not necessary to check mc.preschedule because, regardless of the value, it should call lapply when mc.cores == 1. The call to lapply it's moved before the mc.preschedule conditional blocks so it works for both cases.

There is no need to move the ## Follow lapply chunk because the same checks are performed inside the lapply function. It's only necessary if we don't call lapply.

We don't modify X, I don't see why length(X) should change.
Comment 4 Martin Maechler 2018-01-11 16:09:05 UTC
(In reply to Joan Maspons from comment #3)
> Hello.
> 
> It's not necessary to check mc.preschedule because, regardless of the value,
> it should call lapply when mc.cores == 1. The call to lapply it's moved
> before the mc.preschedule conditional blocks so it works for both cases.
> 
> There is no need to move the ## Follow lapply chunk because the same checks
> are performed inside the lapply function. It's only necessary if we don't
> call lapply.
> 
> We don't modify X, I don't see why length(X) should change.

Yes, indeed checking is mc.preschedule is unneeded. That part of my comment may have been misleading.

But yes, we do conditionally modify X, as I said, you're wrong there.
No big deal.
Comment 5 Joan Maspons 2018-01-11 16:13:48 UTC
(In reply to Martin Maechler from comment #4)

> But yes, we do conditionally modify X, as I said, you're wrong there.
> No big deal.

My point is that if we finally call lapply the line

if(!is.vector(X) || is.object(X)) X <- as.list(X)
Comment 6 Joan Maspons 2018-01-11 16:20:33 UTC
Sorry, last comment get posted before finnishing it.

(In reply to Martin Maechler from comment #4)

> But yes, we do conditionally modify X, as I said, you're wrong there.
> No big deal.

My point is that if we finally call lapply, the line

  if(!is.vector(X) || is.object(X)) X <- as.list(X)

is run 2 times (in mclapply and in lapply). 

There is no need to move this line before running lapply. It's only necessary if we skip all the exit points that call lapply and we really enter the parallel execution of the function.

But yes, not a big deal
Comment 7 Martin Maechler 2018-01-12 15:11:38 UTC
The correct patch has now been committed to R-devel 


(hint:  X <- as.list(X) does change X, and *can* change length(X) and hence *must* come early)