CONSISTENCY: %dopar% with doSEQ should evaluate expression in `local()` environment

[HenrikBengtsson]

I'd like to suggest that the doSEQ backend evaluates the %dopar% expression in a local() environment. This will help clarify that "global" assignments should not be made within %dopar% expressions. Currently, the latter is a common misunderstanding and one of the FAQs on foreach.

Details

Currently, we have that %dopar% falls back to using the doSEQ backend if no %dopar% backend is registered. Now, doSEQ evaluates the expression in the parent frame, which means that all assignments end up there, e.g.

> library(foreach)
> rm(a)
> y <- foreach(i=1:2) %dopar% { a <- i; i }
Warning message:
executing %dopar% sequentially: no parallel backend registered 
> a
[1] 2

This has the unfortunate side effect that users believe that doing assignments from within a %dopar% loop should work and as soon as they turn to real parallel backend their code no longer works.

My proposal is to have the doSEQ expression be evaluated in a local environment, effectively achieving something like:

> library(foreach)
> rm(a)
> y <- foreach(i=1:2) %dopar% local({ a <- i; i })
Warning message:
executing %dopar% sequentially: no parallel backend registered 
> a
Error: object 'a' not found

This can probably be implemented with something as simple as adding:

if (local) envir <- new.env(parent=envir)

to the top of doSEQ() + some care of providing a setting/argument local so that doSEQ can still be used for both %dopar% (local=TRUE) and %do% (local=FALSE).

[hongooi73]

Hi Henrik, what you suggest sounds very reasonable. I'm a bit worried about back-compatibility though; foreach has been around for more than a decade and as a brand-new maintainer, I'm not sure what the impact of this change would be.

@richcalaway would you have any thoughts? (Sorry for getting you involved again...)

[HenrikBengtsson]

Running revdepcheck::revdep_check() with local=TRUE enabled would give a good estimate on how big of a problem this is on CRAN and Bioconductor, i.e. how frequent this %dopar% mistake occurs (because that is what I think it is - a bug, mistake, misuse, ...).

In order to change the default, there are different strategies where one slowly deprecate the current behavior while allowing users to still use it via explicit settings/arguments.

[HenrikBengtsson]

I'd like to revisit this one.

First of all, I'd like to argue that any code that breaks if updated from:

y <- foreach(...) %dopar% { expr }

to

y <- foreach(...) %dopar% local({ expr })

is incorrect and buggy. I cannot find a valid argument for why it's not a bug. If the code assumes it should work, I think it has never been run in parallel and assumes sequential processing. Then it should use %do% instead;

y <- foreach(...) %do% { expr }

So, a way forward could be to introduce optional support for this and slowly deprecate the non-local evaluation in %dopar% over time. Something like:

1. Add support for options(foreach.dopar.local = TRUE) which makes %dopar% { expr } on registerDoSEQ() to be evalulated as local({ expr }) where the default is local <- getOption("foreach.dopar.local", FALSE). To make this controllable via env variables, one could let the default be as.logical(Sys.getenv("R_FOREACH_DOPAR_LOCAL", "FALSE")) instead of hardcoded to FALSE. The only function that would require an update is foreach:::doSEQ(). Stopping here, would be backward compatible too.

2. Test foreach with R_FOREACH_DOPAR_LOCAL=TRUE - maybe something will be caught.

3. Set export R_FOREACH_DOPAR_LOCAL=TRUE and run reverse package dependency checks, e.g. revdepcheck::revdep_check(). This will tell you if this is a real problem and how big of an issue it is.

4. Possibly work with maintainers of faulty packages and have them fix the assumptions in their code, e.g. to use %do% instead of %dopar% (which is the only reasonable solution I can see for such faulty assumptions).

5. Tell package developers to test with R_FOREACH_DOPAR_LOCAL=TRUE.

6. Have foreach:::.onLoad() detect when R CMD check is run and then automatically set options(foreach.dopar.local = TRUE). This is pretty easy to implement, e.g. https://github.com/HenrikBengtsson/R.cache/blob/a9265edbadb422a0fbf01a3eaa7de2c7a9d87cde/R/zzz.R#L7-L14.
   a. The first would be to set this only when !testthat:::on_cran() (= identical(Sys.getenv("NOT_CRAN"), "true")) - this way it would not introduce errors on CRAN or Bioconductor but yet it would trigger the attention of developers who use devtools::check(), which is a fair number these days.
   b. Eventually, do it for all package checks, i.e. do not condition on NOT_CRAN. This would force maintainers to fix these doSEQ mistakes and no new packages will be able to introduce new mistakes. Existing foeach scripts with this mistake will still work.

7. Finally, switch the default to become Sys.getenv("R_FOREACH_DOPAR_LOCAL", "TRUE"). Existing foeach scripts with this mistake will now work.

[hongooi73]

Thanks for the reminder. I'll see if I can get this implemented over the next week or so.

[hongooi73]

Ugh, working with 90s-era tooling....

[HenrikBengtsson]

Awesome - thanks for moving forward on this. Now, reading your added NEWS entry makes me wonder if we think about the problem in the same way - or rather - I think you're trying to address more things in your PR.

To clarify, the way I think of it, is that there are two foreach APIs, one using %do% (always sequential) and one using %dopar% (sequential or parallel). Since %dopar% may/often evaluates in another R process, I figured it should always evaluate in a local() for consistency. Personally, I'm less concerned about %do% right now because it always evaluates sequentially in the current R process, so in that sens it's alright if %do% evaluates in the calling environment as now, but the same is not true for %dopar% - there we must evaluate in a local environment to better emulate the same behavior across backends.

If you decide to make %do% and %dopar% behave the same here, then I think it's time to ask the question what the difference between the two is/should be. Is the difference that the develop should use %do% whenever they want to make sure foreach() runs sequentially whereas with %dopar% the developer allows the user to control evaluation via registerDoSEQ(), registerDoParallel(), registerDoMC(), etc. Basically, my feeling is that updating only the behavior of %dopar% and not %do% is a small leap and should introduce fever conflicts. Doing the same for %do% risks breaking lots more code, code that use foreach() as a for loop without considering its return value, e.g.

a <- 0
foreach(i in 1:10) %do% {
  a <- a + i
}

I can imagine there's lots of code/scripts that use this design pattern. I don't expect developers who use %dopar% to expect that to work (and it is a bug). So, I think it is safer to treat any updates to %do% separately. This is also why I think there should be distinct options/env vars for %do% and %dopar%.

PS. For my own development, I decided on option name format <pkg>.<option> to make it easier to guess in what context an existing option applies.

[hongooi73]

Ok, I misunderstood. I thought this issue was referring to a problem with %do%, rather than %dopar%.

Ironically, I've actually never used foreach; parallel::par*apply has always been good enough. Might need to familiarise myself more with the stuff I'm supposed to maintain....

[hongooi73]

So if I understand correctly, what you want is that (when there is no parallel backend)

a <- 0; foreach(i=1:10) %dopar% { a <- a + 1 }

and

a <- 0; foreach(i=1:10) %do% { a <- a + 1 }

should behave differently?

[HenrikBengtsson]

I wouldn't say that is my objective but, yes, that is the outcome of my proposed update to %dopar% + plus I think it's the safest to leave %do% as is. Because of this, there is a need to distinguish the two cases when setting up doSEQ, which I think comes in in:

> foreach:::getDoPar
> foreach:::registerDoSEQ

So, there's probably a need to have two versions of doSEQ - one that evalulates locally and one that doesn't. That should be doable with via a factory function, in order to not having to maintain two, almost identical versions.

[hongooi73]

7fd184d

I didn't need 2 versions of doSEQ, by piggybacking on the (supposedly) unused data argument. This is very hackish, but is also the solution that probably touches the code least.

[hongooi73]

So, yes, slightly more complicated than I thought....

[richcalaway]

I'd agree that the local() enhancement is worthwhile for dopar constructions, for the reasons Henrik outlined originally. I'd go so far as to say it's actually a bug in the original design. One potential gotcha is that awhile back we had a third-party contribution of the infrastructure to allow user-defined doSEQ functions; I don't know if anyone ever used that, but it's something to keep in mind.

[hongooi73]

Merged into Release/1.4.8.