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

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

Open
HenrikBengtsson opened this issue Sep 8, 2019 · 29 comments

Comments

@HenrikBengtsson
Copy link

@HenrikBengtsson HenrikBengtsson commented Sep 8, 2019

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).

@HenrikBengtsson HenrikBengtsson changed the title CONSISTENCY: doSEQ should evaluate expression in `local()` environment CONSISTENCY: %dopar% with doSEQ should evaluate expression in `local()` environment Sep 8, 2019
@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Sep 10, 2019

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

This comment has been minimized.

Copy link
Author

@HenrikBengtsson HenrikBengtsson commented Sep 10, 2019

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

This comment has been minimized.

Copy link
Author

@HenrikBengtsson HenrikBengtsson commented Dec 31, 2019

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.

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Dec 31, 2019

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

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 7, 2020

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

@HenrikBengtsson

This comment has been minimized.

Copy link
Author

@HenrikBengtsson HenrikBengtsson commented Jan 7, 2020

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.

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 8, 2020

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....

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 8, 2020

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

This comment has been minimized.

Copy link
Author

@HenrikBengtsson HenrikBengtsson commented Jan 8, 2020

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.

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 9, 2020

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.

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 15, 2020

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

@richcalaway

This comment has been minimized.

Copy link
Collaborator

@richcalaway richcalaway commented Jan 15, 2020

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.

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 16, 2020

Merged into Release/1.4.8.

@Hong-Revo Hong-Revo closed this Jan 16, 2020
@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 24, 2020

revdepcheck results:

  • R_FOREACH_LOCAL_DOPAR=FALSE: no additional problems
  • R_FOREACH_LOCAL_DOPAR=TRUE: at least 3 packages have errors: AMARETTO, BiocParallel, pec
@HenrikBengtsson

This comment has been minimized.

Copy link
Author

@HenrikBengtsson HenrikBengtsson commented Jan 24, 2020

Nice. I think that's good news that only three packages are affected. Though, BiocParallel is a bit concerning given that it, in turn, provides a parallel API but hopefully it's minor. Would it be possible for you to share your {revdepcheck} results (e.g. https://github.com/HenrikBengtsson/future/tree/master/revdep) - I can imagine it'll be useful also in the future.

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 24, 2020

Actually, I'm just rerunning the check now since I discovered that I ran out of space on the main filesystem. Some of those packages have 2GB of additional dependencies 😲

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 25, 2020

Looks like those results are good.

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 26, 2020

No wait, I spoke too soon. The results look annoyingly non-deterministic; more like random glitches in the testing environment than actual problems in the code. Now the FALSE check reports that doFuture (!) fails, and the TRUE check reports that MineICA fails.

I've uploaded the results. Will try running the test again with some tweaks.

@Hong-Revo Hong-Revo reopened this Jan 26, 2020
@HenrikBengtsson

This comment has been minimized.

Copy link
Author

@HenrikBengtsson HenrikBengtsson commented Jan 26, 2020

The doFuture error is a false positive and a glitch. It happens to me too. It's an issue with how parallel makeCluster() works and it happens at random. If you rerun it, it'll most likely go away. You can rerun a single package using revdep_add(packages = "doFuture").

So, it seems like you caught one package with the local() bug. I wouldn't be surprised if there's a foreach() %dopar% { ... } without its value assigned to a variable. Just a guess.

If that's the only package, this is really good news.

@HenrikBengtsson

This comment has been minimized.

Copy link
Author

@HenrikBengtsson HenrikBengtsson commented Jan 26, 2020

BTW, in a similar vein as this env var, one could image having an env var controlling what the default adaptor should be. This way you set the default to doParallel(cl = makeCluster(2)) instead of doSEQ(). That will really test the correctness of all those packages. FYI, I do this via various hacks using doFuture for some common packages and I found that lots of bugs when it comes to global variables. These are rescued by how future finds globals but that won't happen with doParallel (until issue #2 is done). This is just a note for now, but it illustrates how one can validate and strengthen the quality of downstream packages by these tricks. I do these extra checks sometimes with future's revdep pkgs to make sure inners of futures and my design stand the test. It also helps me understand common misunderstanding by other developers. So, now with revdepcheck set up, there's lots of powerful things you can achieve.

@HenrikBengtsson

This comment has been minimized.

Copy link
Author

@HenrikBengtsson HenrikBengtsson commented Jan 26, 2020

Found the bug in MineICA. It attempts to update centrotypes from within the foreach call, cf. https://gitea.com/hb/MineICA/src/branch/master/R/runAn.R#L83-L129

(Turns out I know the author)

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 27, 2020

Rerunning the check (again) uncovers a couple more problems: CounterFactual and turboEM. revdep_add is kind of annoying since you have to run a full check at least once. I've opened an issue about this.

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 27, 2020

Counterfactual and TurboEM appear to be more false positives. DepecheR appears as a failure for the FALSE check, but not for the TRUE check; this is probably another false positive as well.

@HenrikBengtsson

This comment has been minimized.

Copy link
Author

@HenrikBengtsson HenrikBengtsson commented Jan 27, 2020

FYI, I've got https://github.com/HenrikBengtsson/future/blob/develop/revdep/run.R with lots of bells and whistles that I've needed for my revdepcheck. Have a look at it, since you might reinventing some of those wheels.

revdep_add is kind of annoying since you have to run a full check at least once.

Yes, I haven't tried it, but as a workaround you might be able to interrupt a big run, then use revdep_rm() and revdep_add() to run a specific packages.

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Jan 31, 2020

Looks like the update should be ready to go. Unless anyone has objections, I'll submit this to CRAN next Friday.

@HenrikBengtsson

This comment has been minimized.

Copy link
Author

@HenrikBengtsson HenrikBengtsson commented Feb 1, 2020

Thanks @Hong-Revo for doing this and pushing this fwd. I and @bwlewis met up at rstudio::conf the other day and we got to talk a lot about the philosophy behind foreach and what things can/should be tidied up and clarified for the users/devs. It was the first time we met in person and I was happy to learn that @bwlewis has a deep understanding of foreach's design philosophy and what Steve wanted it be.

@bwlewis, would mind commenting on the current strategy for achieving local() evaluation for %dopar% (but not %do%) conditionally on a few things. It looks good to me.

The strategy behind this update/release is:

  1. Keep everything backward compatible for now, but introduce an R option and env var for changing the default to local() so devs, users, and revdep can test.

  2. Release to CRAN.

  3. Reach out to broken revdep packages and have them update.

  4. Change the default to use local().

  5. Eventually, remove this new R option and env var from the package again.

The gist of the code updates are in:

  1. The default behavior is set in .onLoad():

.onLoad <- function(libname, pkgname) {
local <- as.logical(Sys.getenv("R_FOREACH_DOPAR_LOCAL", "FALSE"))
local <- getOption("foreachDoparLocal", local)
options(foreachDoparLocal=local)
invisible(NULL)
}

  1. doSEQ() uses local() conditional on the R option and a new internal .foreach_do flag that is only set in %do%:

if(is.null(parent.env(environment())$.foreach_do) &&
getOption("foreachDoparLocal"))
envir <- new.env(parent=envir)

environment(e$fun)$.foreach_do <- TRUE

@HenrikBengtsson

This comment has been minimized.

Copy link
Author

@HenrikBengtsson HenrikBengtsson commented Feb 11, 2020

I've emailed the maintain of MineICA and asked her to fix this in Bioc devel and release.

@Hong-Revo, it's not clear to me if there are other packages on CRAN and Bioconductor with this problem. Did you run revdep checks on all CRAN and Bioconductor packages?
The https://github.com/RevolutionAnalytics/foreach/tree/Release/foreach-1.4.8/revdep results only lists those four packages that you focused on at the end.

If there was only one package with this problem, which would surprise me, I think we could flip the switch and have R_FOREACH_LOCAL_DOPAR=TRUE be the default during R CMD check If something was missed, then this should trigger errors on Bioconductor and CRAN.

@Hong-Revo

This comment has been minimized.

Copy link
Collaborator

@Hong-Revo Hong-Revo commented Feb 12, 2020

Thanks for contacting the MineICA person. I was going to wait until the next release before contacting the revdep maintainers, but now works too. I've actually run a full revdep check a few times now, and nothing else has appeared that wasn't clearly a false positive. I should probably push those results.

I think the reason for the low positive rate is because this problem is something that would be caught fairly quickly once a package is in use. It's probably more likely to occur in open code, during prototyping. Also, I don't know how many of those revdeps will actually be testing the foreach portion, and if they do, how many use a sequential backend.

@HenrikBengtsson

This comment has been minimized.

Copy link
Author

@HenrikBengtsson HenrikBengtsson commented Feb 12, 2020

That's great news.

I think it would be useful to expose the full revdep results. It would also show to the world how many CRAN + Bioc pkgs rely on foreach. Over time you'll also get a record how this grows.

If only that one pkg, I think one could flip the switch for R CMD check via .onLoad() and wait for it to propagate on CRAN and Bioc to see what happens. If we made no mistakes, there should still be only that one package.

True, there are probably a fair bit of packages that don't have the test coverage for this bug to be detected. Those will only be exposed when end-users run into problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.