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

Fixing sim_fixed_n() parallel results return #252

Merged
merged 4 commits into from
May 28, 2024
Merged

Conversation

cmansch
Copy link
Collaborator

@cmansch cmansch commented May 28, 2024

This update addresses two open issues: #250 and #251.

In order to return the results from each %dofuture% loop, the last object must return some value. The setDF() call previously used resulted in the results not being passed back from the loop when run with a parallel backend. This error was hidden in the results as the loop used the .errorhandling = "pass" instead of "stop".

The parallel vignette was also updated to indicate that sim_gs_n() supports the same backend implementation as sim_fixed_n().

cmansch and others added 3 commits May 28, 2024 13:38
…r a parallel implementation. Also updating the `.errorhandling` option to `"stop"` to ensure that issues with parallel implementation are flagged as they are written and not hidden in the results.
@cmansch cmansch self-assigned this May 28, 2024
@cmansch cmansch marked this pull request as ready for review May 28, 2024 18:35
@jdblischak
Copy link
Collaborator

I get an error when running it locally. Am I the only one?

git2r::commits(n = 1)
## [[1]]
## [443a59c] 2024-05-28: Updating the parallel vignette to indicate that `sim_gs_n()` also supports the `future` backends.
devtools::load_all()
library("future")
plan("multisession", workers = 2)
sim_fixed_n(n_sim = 10)
## Using 2 cores with backend multisession
## Error in doAnalysis(d, rho_gamma, n_stratum) :
##   could not find function "doAnalysis"
sessionInfo()
## R version 4.3.3 (2024-02-29 ucrt)
## Platform: x86_64-w64-mingw32/x64 (64-bit)
## Running under: Windows 11 x64 (build 22631)
## 
## Matrix products: default
## 
## 
## locale:
##   [1] LC_COLLATE=English_United States.utf8  LC_CTYPE=English_United States.utf8   
## [3] LC_MONETARY=English_United States.utf8 LC_NUMERIC=C                          
## [5] LC_TIME=English_United States.utf8    
## 
## time zone: America/New_York
## tzcode source: internal
## 
## attached base packages:
##   [1] stats     graphics  grDevices utils     datasets  methods   base     
## 
## other attached packages:
##   [1] future_1.33.2    simtrial_0.4.1.3 testthat_3.2.1.1
## 
## loaded via a namespace (and not attached):
##   [1] lattice_0.22-6      stringi_1.8.4       listenv_0.9.1       digest_0.6.35      
## [5] magrittr_2.0.3      grid_4.3.3          iterators_1.0.14    mvtnorm_1.2-4      
## [9] pkgload_1.3.4       fastmap_1.1.1       Matrix_1.6-5        foreach_1.5.2      
## [13] rprojroot_2.0.4     pkgbuild_1.4.4      sessioninfo_1.2.2   brio_1.1.5         
## [17] survival_3.6-4      urlchecker_1.0.1    promises_1.3.0      purrr_1.0.2        
## [21] codetools_0.2-20    cli_3.6.2           shiny_1.8.1.1       rlang_1.1.3        
## [25] parallelly_1.37.1   future.apply_1.11.2 splines_4.3.3       ellipsis_0.3.2     
## [29] remotes_2.5.0       withr_3.0.0         cachem_1.0.8        devtools_2.4.5     
## [33] tools_4.3.3         parallel_4.3.3      memoise_2.0.1       doFuture_1.0.1     
## [37] httpuv_1.6.15       globals_0.16.3      vctrs_0.6.5         R6_2.5.1           
## [41] mime_0.12           lifecycle_1.0.4     git2r_0.33.0        stringr_1.5.1      
## [45] fs_1.6.4            htmlwidgets_1.6.4   usethis_2.2.3       miniUI_0.1.1.1     
## [49] desc_1.4.3          later_1.3.2         glue_1.7.0          data.table_1.15.4  
## [53] profvis_0.3.8       Rcpp_1.0.12         rstudioapi_0.16.0   xtable_1.8-4       
## [57] htmltools_0.5.8.1   compiler_4.3.3

@jdblischak
Copy link
Collaborator

Hmmm...the problem appears to be with devtools::load_all()

git2r::commits(n = 1)
## [[1]]
## [443a59c] 2024-05-28: Updating the parallel vignette to indicate that `sim_gs_n()` also supports the `future` backends.
library("simtrial")
library("future")
plan("multisession", workers = 2)
sim_fixed_n(n_sim = 10)
# works

@jdblischak
Copy link
Collaborator

I think I figured it out. In short, you can't use devtools::load_all() on Windows to test parallel code that uses "multisession". You can only use devtools::load_all() on unix-alike with "multiprocess" or "multicore. See this Issue for all the details futureverse/future#206

@jdblischak
Copy link
Collaborator

jdblischak commented May 28, 2024

One minor nit: since this fixes a known bug that is affecting end users, it would be useful to bump the version number in DESCRIPTION to 0.4.1.4 so that users can confirm they have the fix installed

@cmansch
Copy link
Collaborator Author

cmansch commented May 28, 2024

One minor nit: since this fixes a known bug that is affecting end users, it would be use to bump the version number in DESCRIPTION to 0.4.1.4 so that users can confirm they have the fix installed

Added a commit to address the version bump!

Copy link
Collaborator

@LittleBeannie LittleBeannie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for timely address it, @cmansch !

@LittleBeannie LittleBeannie merged commit 75297a1 into Merck:main May 28, 2024
7 checks passed
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

Successfully merging this pull request may close these issues.

Example 4 of sim_fixed_n does not work Update sim_fixed_n() and relevant vignette
3 participants