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

BUG: makeClusterPSOCK() produces error if 'parallelly' is not available on worker #23

Closed
HenrikBengtsson opened this issue Oct 22, 2020 · 2 comments
Labels
bug Something isn't working
Milestone

Comments

@HenrikBengtsson
Copy link
Owner

Issue

makeClusterPSOCK() produces an error on 'could not find function "getOptionOrEnvVar"' if parallelly is not available on R worker.

Reproducible example

Let's hide the parallelly package from the worker by setting a dummy R_LIBS_USER value;

Sys.setenv(R_LIBS_USER=tempdir())
cl <- parallelly::makeClusterPSOCK(1)
## Error in checkForRemoteErrors(lapply(cl, recvResult)) : 
##   one node produced an error: could not find function "getOptionOrEnvVar"

with

> traceback()
5: stop("one node produced an error: ", firstmsg, domain = NA)
4: checkForRemoteErrors(lapply(cl, recvResult))
3: clusterCall(cl[ii], fun = session_info)
2: add_cluster_session_info(cl[ii])
1: parallelly::makeClusterPSOCK(1)

Troubleshooting

This is because of the

clusterCall(cl[ii], fun = session_info)

call (cf. Issue #22), where session_info(), which is part of the parallelly package, looks like:

> parallelly:::session_info
function (pkgs = getOptionOrEnvVar("future.makeNodePSOCK.sessionInfo.pkgs", FALSE)) 
{
   ...
}

The pkgs argument is needed but since it is not specifying in the above clusterCall() it falls back to the default value, which requires (non-exported) getOptionOrEnvVar() that is part of the parallelly package.

Now, if the parallelly package would have been available on the worker, it would have worked because R will try to load the parallelly package when it recieves the parallelly:::session_info() object. To see that it actually fails finding the package, we can do:

Sys.setenv(R_LIBS_USER=tempdir())
cl <- parallelly::makeClusterPSOCK(1, outfile="")
## starting worker pid=10563 on localhost:11361 at 19:39:14.323
## Warning: namespace ‘parallelly’ is not available and has been replaced
## by .GlobalEnv when processing object ‘<unknown>’
## Error in checkForRemoteErrors(lapply(cl, recvResult)) : 
## one node produced an error: could not find function "getOptionOrEnvVar"

Since that's only a warning, we don't see it unless we use outfile="" - and even with that we might not see it on all operating systems.

BTW, this also explains why parallelly is loaded on a fresh worker.

Solution

Specify argument pkgs in the call, i.e.

pkgs <- getOptionOrEnvVar("future.makeNodePSOCK.sessionInfo.pkgs", FALSE)
clusterCall(cl[ii], fun = session_info, pkgs = pkgs)

so that it is not needed on the workers.

This bug was identified because the above error showed up kind of randomly when revdep checking the develop version of future that rely on parallelly for setting up clusters, e.g. HenrikBengtsson/future@a8e3b1a but also HenrikBengtsson/future@02fe610. Not sure why rerunning made the latter disappear but I suspect the revdep checks somehow picks up what current version of future I have installed on my account.

@HenrikBengtsson HenrikBengtsson added the bug Something isn't working label Oct 22, 2020
@HenrikBengtsson HenrikBengtsson added this to the Next release milestone Oct 22, 2020
@HenrikBengtsson
Copy link
Owner Author

Fixed

@HenrikBengtsson
Copy link
Owner Author

Workaround

Until the next version of parallelly is on CRAN, we can workaround this bug by tweaking rscript_startup;

Sys.setenv(R_LIBS_USER = tempdir())
rscript_startup <- "getOptionOrEnvVar <- function(...) FALSE"
cl <- parallelly::makeClusterPSOCK(1L, rscript_startup = rscript_startup)

or at bit fancier to respect those options and env vars:

Sys.setenv(R_LIBS_USER = tempdir())
pkgs <- parallelly:::getOptionOrEnvVar("future.makeNodePSOCK.sessionInfo.pkgs", FALSE)
rscript_startup <- paste("getOptionOrEnvVar <- function(...)", pkgs)
cl <- parallelly::makeClusterPSOCK(1L, rscript_startup = rscript_startup)

HenrikBengtsson added a commit to HenrikBengtsson/future that referenced this issue Oct 22, 2020
HenrikBengtsson added a commit to HenrikBengtsson/future that referenced this issue Oct 22, 2020
clrpackages pushed a commit to clearlinux-pkgs/R-future that referenced this issue Nov 24, 2020
… 1.20.1

Henrik Bengtsson (109):
      Bump develop version [ci skip]
      GitHub Actions: Test with strict R_FUTURE_RNG_ONMISUSE=error
      BUG FIX: The Mandelbrot demo would produce random numbers without declaring so
      typos
      Add 'make spelling', which spell checks NEWS and all vignettes
      'local = FALSE' is deprecated [#382]
      Now all .Deprecated() and .Defunct() specify the 'package' argument
      HELP: Give example how to set options and env vars in R [ci skip]
      print() for Future would also print any attributes of its environment
      Remove more references to 'multiprocess'
      Deprecated 'multiprocess' [#420]
      It's ok for transparent to use local=FALSE [#382]
      TYPO: Renamed environment variable 'R_FUTURE_MAKENODEPSOCK_tries' to 'R_FUTURE_MAKENODEPSOCK_TRIES' [ci skip]
      CLEANUP: Moved as.cluster(), autoStopCluster(), makeClusterMPI(), makeClusterPSOCK(), and makeNodePSOCK() to 'parallelly'
      CLEANUP: Moved supportsMulticore() to 'parallelly'
      CLEANUP: Moved availableCores() and availableWorkers() to 'parallelly'
      CLEANUP: Moved several options/env vars to the 'parallelly' package
      CLEANUP: Removed internal code used for validating connection; importing from 'parallelly' package instead [ci skip]
      values() is to be deprecated; use value() instead [#426]
      Deprecate values() [#426]
      REVDEP: 139 CRAN & Bioconductor pkgs after deprecating values() [ci skip]
      BUG FIX: The error message produced by nbrOfWorkers() was incomplete
      Now tweak() passes also arguments not explicitly available on the tweaked future function
      New tweak() did not handle arguments with NULL values [ci skip]
      REVDEP: 140 CRAN & Bioconductor pkgs - new tweak() breaks furrr test; testthat specific?!? [ci skip]
      TESTS: Assert that plan(tweak(...)) does not pickup package dependencies from globals causing it to load those packages on workers
      Now tweak() passes also arguments not explicitly available on the tweaked future function
      New tweak() did not handle arguments with NULL values [ci skip]
      FIX: New tweak() would pick up and re-export package dependendies from the calling frames [ci skip]
      REVDEP: 190 CRAN & Bioconductor pkgs; fixing tweak envir problem ... but was another one introducd, cf. simhelpers [ci skip]
      REVDEP: Warning on simhelpers was a hiccup and a false positive [ci skip]
      NEWS: Mention new tweak()
      plan() and tweak() protect more arguments from being adjusted
      FIX: New tweak() required bquote() of R (>= 4.0.0)
      plan()/tweak(): assert also that 'globals' is not tweaked
      Add "hidden" future argument 'split' across the board [#424]
      TESTS: Add test for 'split=TRUE' [#424]
      tweak()/plan(): Add more arguments to the "forbidden" list
      REVDEP: 140 CRAN & Bioconductor pkgs after preventing 'forbidden' arguments + support for 'split = TRUE' [#424] [ci skip]
      Add attribute 'untweakable' for 'future' strategy classes; this allows backends to define *additional* untweakable arguments [ci skip]
      CLEANUP: Drop stray code for defunction Future format v1.7
      BUG FIX: Use conditions = NULL to let conditions alone; this replaces conditions = character(0L) which doesn't work since future 1.19.1 [#427]
      Roxygen: inherit from 'future', which should document all arguments needed
      TESTS: Tighten up test of futureOf(NULL)
      TESTS: Avoid retries of makeClusterPSOCK() exception tests
      Explicitly list 'label' as an argument to future()
      cleanup; drop lots of explicit arguments
      REVDEP: 140 CRAN & Bioconductor pkgs - after cleaning out lots of arguments from future strategy functions [ci skip]
      CODE: Harmonizing Future constructor functions to also use 'substitute = TRUE'
      CLEANUP: With 'substitute = TRUE' as the default throughout, the argument can be dropped from more future strategy functions [ci skip]
      CLEANUP: Remove all code relying on defunct element 'value' [ci skip]
      More arg cleanup, especially around constant futures [ci skip]
      CLEANUP: Pass more arguments via '...' for cluster() and remote()
      Now it's possible to pass any makeClusterPSOCK() argument via plan()
      CLEANUP: multisession() no longer sets up the cluster; that task is now passed down to ClusterFuture, which already can do it [ci skip]
      NEWS: clarify [ci skip]
      CLEANUP: Removing defunct, hidden argument 'evaluator'.
      Now plan("next") asserts that it returns a function
      PERFORMANCE: stopifnot() -> stop_if_not()
      CLEANUP: No need to test makeClusterPSOCK() here; it's done in 'parallelly' [ci skip]
      DOCS: Update according to new 'parallelly' package [ci skip]
      REVDEP: 140 CRAN & Bioconductor pkgs - after cleaning out even more arguments and code [ci skip]
      REVDEP: 140 CRAN & Bioconductor pkgs - after re-exporting from parallelly => furrr needs to be updated; photosynthesis is being investigated [ci skip]
      HELP: Clarify further why setting the plan() in a function without undoing is a bad idea [ci skip]
      REVDEP: 140 CRAN & Bioconductor pkgs - rerunning "fixed" photosynthesis [ci skip]
      HELP: Using \describe{} for all option sections [ci skip]
      HELP: Using \describe{} for all option sections [ci skip]
      REVDEP: 140 CRAN & Bioconductor pkgs - with furrr 0.2.1, which confirms a bug in 'parallelly' on getOptionOrEnvVar not found [ci skip]
      Workaround bug HenrikBengtsson/parallelly#23 in parallelly 1.20.0
      Add 'earlySignal' to the set of tweakable future arguments [ci skip]
      Patch parallelly 1.20.0 when package is loaded
      Needed to hide valid patch from 'R CMD check' [ci skip]
      REVDEP: 140 CRAN & Bioconductor pkgs - on-load patching of parallelly 1.20.0 works [ci skip]
      BUG FIX: Near-live signalling was not supported for useXDR=FALSE PSOCK clusters
      NEWS: restructure entries [ci skip]
      fixes #436 [ci skip]
      TESTS: 'parallelly' and dependency 'tools' are also loaded on cluster workers
      Now multisession workers inherit the package library path from the main R session [#435]
      NEWS: clarify [ci skip]
      REVDEP: 140 CRAN & Bioconductor pkgs - inherits .libPaths() [ci skip]
      Add a 'Best Practices for Package Developers' vignette [#375]
      Add a 'How the Future Framework is Validated' vignette
      NEWS: tweaks [ci skip]
      Cleaned up the expression generated by makeExpression() a tad
      PERFORMANCE: Avoid utils::packageVersion() because it hits the file system and is slow
      PERFORMANCE: Apply the covr-workaround once when the PSOCK cluster is started instead for every cluster future
      Undo 'covr' workaround; not worth it
      REVDEP: 141 CRAN & Bioconductor pkgs [ci skip]
      tweak [ci skip]
      makeExpression(): more cleanup tweaks
      REVDEP: 141 CRAN & Bioconductor pkgs [ci skip]
      CLI options --parallel=n now sets a 'multisession' plan; was 'multiprocess' [#420]
      Now multicore futures relay immediateCondition:s in a near-live fashion [#419]
      CLEANUP: readImmediateConditions() gained argument 'signal = TRUE' (#419)
      clarify [ci skip]
      Use an explicit argument 'signal = TRUE' [ci skip]
      REVDEP: 141 CRAN & Bioconductor pkgs - with multicore live relaying [ci skip]
      Bump 'globals' requirement
      Add a secret, temporary option and env var for silencing deprecation warnings [#420]
      Allow for comma-separated values in future.deprecated.ignore/R_FUTURE_DEPRECATED_IGNORE [#420] [ci skip]
      TESTS: Relax one assertion on the state of a cluster future; it might be that it already is 'running' if it's really quick
      AppVeyor CI: Install also 'parallelly'
      REVDEP: 140 CRAN & Bioconductor pkgs [ci skip]
      future 1.20.0
      CLEANUP: parallelly 1.20.0 patch no longer needed
      NEWS: typo
      BUG FIX: future::plan("multisession") would produce an error if the covr package was loaded
      Travis CI: Run 'covr' under script
      future 1.20.1

Version: 1.20.1 [2020-10-30]

BUG FIXES:

 * future::plan("multisession") would produce 'Error in if (debug)
   mdebug("covr::package_coverage() workaround ...") : argument is not
   interpretable as logical' if and only if the 'covr' package was loaded.

Version: 1.20.0 [2020-10-30]

SIGNIFICANT CHANGES:

 * Strategy 'multiprocess' is deprecated in favor of either 'multisession' or
   'multicore', depending on operating system and R setup.  The plan() function

(NEWS truncated at 15 lines)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant