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

Global variables in functions cannot be found #43

Open
joshuaulrich opened this issue Nov 28, 2016 · 4 comments
Open

Global variables in functions cannot be found #43

joshuaulrich opened this issue Nov 28, 2016 · 4 comments

Comments

@joshuaulrich
Copy link

Granted, using global variables in functions isn't the best practice, but sometimes users do it. In this case, doRedis does not behave as do other foreach backends, which caught me off-guard. For example:

# sequential
suppressMessages(require("foreach"))
registerDoSEQ()
f <- function() x
x <- "Hello World"
foreach(1) %dopar% eval(call("f"))
# [[1]]
# [1] "Hello World"

# multicore
suppressMessages(require("doMC"))
registerDoMC()
f <- function() x
x <- "Hello World"
foreach(1) %dopar% eval(call("f"))
# [[1]]
# [1] "Hello World"

# socket
suppressMessages(require("doParallel"))
cl <- makePSOCKcluster(2)
registerDoParallel(cl)
f <- function() x
x <- "Hello World"
clusterExport(cl, c("f", "x"))
foreach(1) %dopar% eval(call("f"))
# [[1]]
# [1] "Hello World"

# redis
suppressMessages(require("doRedis"))
registerDoRedis("jobs")
startLocalWorkers(1, "jobs")
f <- function() x
x <- "Hello World"
setExport(c("f", "x"))
foreach(1) %dopar% eval(call("f"))
# Error in eval(call("f")) : task 1 failed - "object 'x' not found"

I've tracked this down to the evaluation in doRedis:::.evalWrapper(). The expression .doRedisGlobals$expr is evaluated using the .doRedisGlobals$exportenv environment, which contains all the exported variables. But .doRedisGlobals$exportenv is not on the search path, so x it cannot be found when f() is called. That's because f()'s environment is the global environment, and when x cannot be found there, R starts to walk the search path. Note that this is only a problem when the function is invoked via eval.

It seems like you tried to address something like this already. But I don't think that works in this case, because the evaluation environments are not searched when f() is called. Only f()'s enclosing environments are searched. Since the documentation warns that parent.env<- may be removed, perhaps the next best solution is attach(.doRedisGlobals$exportenv)?

If you like, I'm happy to make that change, test using my actual use case, and submit a PR.

@bwlewis
Copy link
Owner

bwlewis commented Nov 30, 2016

A PR would be appreciated. I don't fully understand what's going on yet. The .evalWrapper() function and .workerInit() function in doRedis are extremely close to, for instance, the evalWrapper() and workerInit() functions in doParallel, including the questionable use of replacing the parent environment that you call out. Strange.

@joshuaulrich
Copy link
Author

Well, my doParallel example only works if you call clusterExport first, which assigns the objects to the global environment of the nodes by default. Nodes created by doMC essentially have a copy of the master process, so the objects are in the global environment on the node too.

doRedis::setExport assigns the objects to the .doRedisGlobals$exportenv, not the global environment like the other two. Since .doRedisGlobals$exportenv isn't on the search path, objects in it cannot be found by lexical scope if they're used inside a user-defined function (because the user cannot define the function in .doRedisGlobals$exportenv; it doesn't exist until they call foreach).

I found How R Searches and Finds Stuff useful while looking into this.

@bwlewis
Copy link
Owner

bwlewis commented Nov 30, 2016

That makes sense. It would be nice to follow a purley attach() approach and discard the re-ordering of the search path by re-assigning parent.env if possible.

I think I tried the attach() approach a long time (years) ago and ran into trouble. This is a subtle issue.

@bwlewis
Copy link
Owner

bwlewis commented May 12, 2017 via email

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

2 participants