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

Notify parallel (or other pkg) about SIGCHLD #237

Merged
merged 5 commits into from Feb 5, 2020
Merged

Conversation

@gaborcsardi
Copy link
Member

gaborcsardi commented Feb 5, 2020

This is now opt in, and the PROCESSX_NOTIFY_OLD_SIGCHLD
env var has to be set (to any value), at the time of
loading of the processx package.

Fixes #236.

This is now opt in, and the PROCESSX_NOTIFY_OLD_SIGCHLD
env var has to be set (to any value), at the time of
_loading_ of the processx package.

Fixes #236.
parallel is a bit different on R 3.4.x and
earlier.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 5, 2020

Codecov Report

Merging #237 into master will increase coverage by 2.08%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
+ Coverage   72.12%   74.21%   +2.08%     
==========================================
  Files          31       31              
  Lines        2526     2536      +10     
==========================================
+ Hits         1822     1882      +60     
+ Misses        704      654      -50
Impacted Files Coverage Δ
src/unix/processx.c 73.84% <100%> (+6.63%) ⬆️
src/unix/sigchld.c 85.41% <80%> (+0.41%) ⬆️
R/process.R 81.21% <0%> (+3.03%) ⬆️
src/client.c 79.62% <0%> (+33.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65d47b8...c2afe03. Read the comment docs.

gaborcsardi added 3 commits Feb 5, 2020
It works differently on macOS and Linux
[ci skip]
@gaborcsardi gaborcsardi merged commit eb99979 into master Feb 5, 2020
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@gaborcsardi gaborcsardi deleted the fix/parallel=sigchld branch Feb 5, 2020
@gaborcsardi

This comment has been minimized.

Copy link
Member Author

gaborcsardi commented Feb 5, 2020

The Appveyor failure is unrelated.

@wlandau

This comment has been minimized.

Copy link

wlandau commented Feb 6, 2020

I'm really happy to see a workaround for #236, but I am having trouble using it with 8baaa7e. I included a reprex below, and I get the same results if I start a fresh R session and do not use reprex. Results are the same for Mac OS and RHEL 7.

fun <- function() parallel::mclapply(1:2, function(x) x)
env <- c(callr::rcmd_safe_env(), PROCESSX_NOTIFY_OLD_SIGCHLD = "true")
tmp <- callr::r(fun, env = env, show = TRUE)
#> Error while shutting down parallel: unable to terminate some child processes

Created on 2020-02-06 by the reprex package (v0.3.0)

Session info
devtools::session_info()
#> ─ Session info ───────────────────────────────────────────────────────────────
#>  setting  value                       
#>  version  R version 3.6.1 (2019-07-05)
#>  os       macOS Mojave 10.14.6        
#>  system   x86_64, darwin15.6.0        
#>  ui       X11                         
#>  language (EN)                        
#>  collate  en_US.UTF-8                 
#>  ctype    en_US.UTF-8                 
#>  tz       America/Indiana/Indianapolis
#>  date     2020-02-06                  
#> 
#> ─ Packages ───────────────────────────────────────────────────────────────────
#>  package     * version    date       lib source                              
#>  assertthat    0.2.1      2019-03-21 [1] CRAN (R 3.6.0)                      
#>  backports     1.1.5      2019-10-02 [1] CRAN (R 3.6.1)                      
#>  callr         3.4.1.9000 2020-02-06 [1] Github (r-lib/callr@0e484ff)        
#>  cli           2.0.1      2020-01-08 [1] CRAN (R 3.6.0)                      
#>  crayon        1.3.4      2017-09-16 [1] CRAN (R 3.6.0)                      
#>  desc          1.2.0      2019-08-19 [1] Github (r-lib/desc@c860e7b)         
#>  devtools      2.2.1      2019-09-24 [1] CRAN (R 3.6.0)                      
#>  digest        0.6.23.2   2020-01-03 [1] Github (eddelbuettel/digest@199be2e)
#>  ellipsis      0.3.0      2019-09-20 [1] CRAN (R 3.6.0)                      
#>  evaluate      0.14       2019-05-28 [1] CRAN (R 3.6.0)                      
#>  fansi         0.4.1      2020-01-08 [1] CRAN (R 3.6.0)                      
#>  fs            1.3.1      2019-05-06 [1] CRAN (R 3.6.0)                      
#>  glue          1.3.1      2019-03-12 [1] CRAN (R 3.6.0)                      
#>  highr         0.8        2019-03-20 [1] CRAN (R 3.6.0)                      
#>  htmltools     0.4.0      2019-10-04 [1] CRAN (R 3.6.0)                      
#>  knitr         1.27       2020-01-16 [1] CRAN (R 3.6.0)                      
#>  magrittr      1.5        2014-11-22 [1] CRAN (R 3.6.0)                      
#>  memoise       1.1.0      2017-04-21 [1] CRAN (R 3.6.0)                      
#>  pkgbuild      1.0.6      2019-10-09 [1] CRAN (R 3.6.0)                      
#>  pkgload       1.0.2      2018-10-29 [1] CRAN (R 3.6.0)                      
#>  prettyunits   1.1.1      2020-01-24 [1] CRAN (R 3.6.1)                      
#>  processx      3.4.1.9001 2020-02-06 [1] Github (r-lib/processx@8baaa7e)     
#>  ps            1.3.0      2018-12-21 [1] CRAN (R 3.6.0)                      
#>  R6            2.4.1      2019-11-12 [1] CRAN (R 3.6.1)                      
#>  Rcpp          1.0.3      2019-11-08 [1] CRAN (R 3.6.0)                      
#>  remotes       2.1.0      2019-06-24 [1] CRAN (R 3.6.0)                      
#>  rlang         0.4.4      2020-01-28 [1] CRAN (R 3.6.0)                      
#>  rmarkdown     2.1        2020-01-20 [1] CRAN (R 3.6.1)                      
#>  rprojroot     1.3-2      2018-01-03 [1] CRAN (R 3.6.0)                      
#>  sessioninfo   1.1.1      2018-11-05 [1] CRAN (R 3.6.0)                      
#>  stringi       1.4.5      2020-01-11 [1] CRAN (R 3.6.0)                      
#>  stringr       1.4.0      2019-02-10 [1] CRAN (R 3.6.0)                      
#>  testthat      2.3.1      2019-12-01 [1] CRAN (R 3.6.0)                      
#>  usethis       1.5.1.9000 2019-08-12 [1] Github (r-lib/usethis@b241420)      
#>  withr         2.1.2      2018-03-15 [1] CRAN (R 3.6.0)                      
#>  xfun          0.12       2020-01-13 [1] CRAN (R 3.6.0)                      
#>  yaml          2.2.1      2020-02-01 [1] CRAN (R 3.6.1)                      
#> 
#> [1] /Library/Frameworks/R.framework/Versions/3.6/Resources/library
@gaborcsardi

This comment has been minimized.

Copy link
Member Author

gaborcsardi commented Feb 6, 2020

@wlandau Can you please open an issue for this, so we don't forget? Thanks.

@gaborcsardi

This comment has been minimized.

Copy link
Member Author

gaborcsardi commented Feb 6, 2020

@wlandau Also, that seems to be a different issue, because callr does not load processx (or any other packages) in the subprocess:

fun <- function() { parallel::mclapply(1:2, function(x) x); print(loadedNamespaces()) }
❯ env <- c(callr::rcmd_safe_env(), PROCESSX_NOTIFY_OLD_SIGCHLD = "true")
❯ tmp <- callr::r(fun, env = env, show = TRUE)
[1] "compiler"  "graphics"  "parallel"  "utils"     "grDevices" "stats"
[7] "datasets"  "methods"   "base"
Error while shutting down parallel: unable to terminate some child processes

It does load a special "client library", but that does not mess with SIGCHLD at all. So this is I guess a callr issue.

In fact, using processx once in the subprocess seems to work around the issue:

fun <- function() { processx::run("true"); parallel::mclapply(1:2, function(x) x) }
❯ tmp <- callr::r(fun, env = env, show = TRUE)
❯ 
@wlandau

This comment has been minimized.

Copy link

wlandau commented Feb 6, 2020

Just opened a new callr issue.

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

Successfully merging this pull request may close these issues.

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