wlandau / crew

A distributed worker launcher
https://wlandau.github.io/crew/
Other
123 stars 4 forks source link

Changes to enable L'Ecuyer-CMRG Streams #115

Closed shikokuchuo closed 1 year ago

shikokuchuo commented 1 year ago

Prework

Related GitHub issues and pull requests

Summary

Posting this as I was testing the new function to make sure it works as designed.

This is a minimal patch, but a good starting point.

Needs to be fleshed out - docs, tests, dependencies (will need to rely on the latest dev build of mirai and CRAN nanonext 0.10.0) - hope can leave that in your good hands.

shikokuchuo commented 1 year ago

Ah probably a bit more needs to change - the seed and algorithm defaults mask the underlying .Random.seed, I had to set them to NULL to get at it.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (b0066d2) 100.00% compared to head (2841e4e) 100.00%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #115 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 22 22 Lines 1300 1302 +2 ========================================= + Hits 1300 1302 +2 ``` | [Files Changed](https://app.codecov.io/gh/wlandau/crew/pull/115?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Will+Landau) | Coverage Δ | | |---|---|---| | [R/crew\_launcher.R](https://app.codecov.io/gh/wlandau/crew/pull/115?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Will+Landau#diff-Ui9jcmV3X2xhdW5jaGVyLlI=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

wlandau commented 1 year ago

Thanks for the PR, @shikokuchuo. I will take it and adjust push() and crew_eval() to accommodate.

Ah probably a bit more needs to change - the seed and algorithm defaults mask the underlying .Random.seed, I had to set them to NULL to get at it.

Sorry, I'm not sure I understand. Are you referring to the defaults in push()? I plan to make the default algorithm in push() equal to "mirai", which will avoid setting either inside the task (crew_eval()).

wlandau commented 1 year ago

And come to think of it, I should probably restore the seed and algorithm if someone sets non-defaults, in order to restore the widely-spaced L'Ecuyer streams.

shikokuchuo commented 1 year ago

Thanks for the PR, @shikokuchuo. I will take it and adjust push() and crew_eval() to accommodate.

Ah probably a bit more needs to change - the seed and algorithm defaults mask the underlying .Random.seed, I had to set them to NULL to get at it.

Sorry, I'm not sure I understand. Are you referring to the defaults in push()? I plan to make the default algorithm in push() equal to "mirai", which will avoid setting either inside the task (crew_eval()).

Yes, exactly what I meant. And yes algorithm = "mirai" is a nice name for the default.

wlandau commented 1 year ago

Should be implemented as of 0ed6b0cf053ee5a3d392d764bb12bbf36a889775. I decided to make algorithm = NULL the default since is.null() is faster than string comparison.

shikokuchuo commented 1 year ago

Great - seems to work. It's feature freeze for mirai as of 0.9.1.9025 until the CRAN release. So do get all your testing in!

wlandau commented 1 year ago

I just ran through all my tests, and almost everything looks good. Everything is definitely still compatible, and I would go ahead with the next mirai release as soon as CRAN allows.

I did encounter a few errors initially, but that may be because my Ubuntu machine couldn't handle the load. It has 4 physical cores and 16 GB memory. I built it back in early 2017. I usually run urlchecker, lintr, testthat, covr, devtools::check(), revdepcheck, and goodpractice all at the same time in different terminal windows.

Running devtools::test():

Error (test-crew_controller_local.R:516:3): map() does not need an started controller
<crew_error/crew/rlang_error/error/condition>
Error: invalid daemons: structure(5L, class = "errorValue")
 The mirai dispatcher is not running. Please call the start() method of the controller (e.g. your_controller$start() before using methods like push(), collect(), and scale(). If you already did, then the dispatcher may have exited too early.

Running covr::package_coverage():

 Error (test-crew_controller_group.R:192:3): crew_controller_group() collect
Error in `launch_and_sync_daemon(sock = sock, urld, parse_dots(...), url, 
    n, urlc, tls = tls)`: initial sync with dispatcher timed out after 5s

I also hit a couple segfaults while all this was happening. But again, my computer may have been struggling to keep up.

shikokuchuo commented 1 year ago

I did encounter a few errors initially... I usually run urlchecker, lintr, testthat, covr, devtools::check(), revdepcheck, and goodpractice all at the same time in different terminal windows.

At the same time - it is probably for this reason - running all those at the same time will likely lead to issues such as contention on locks etc. There are some settings when the NNG library is built which take into account the core count of the machine.