Azure / AzureStor

R interface to Azure storage accounts
Other
64 stars 20 forks source link

Intermittent error spawning processes for storage_multiupload() #98

Closed hadley closed 3 years ago

hadley commented 3 years ago
Error: Error: 10 simultaneous processes spawned
Backtrace:
  1. └─pins::pin_upload(board, c(path1, path2), random_pin_name())
  2.   ├─pins::pin_store(board, name, paths, meta, ...)
  3.   └─pins:::pin_store.pins_board_azure(board, name, paths, meta, ...)
  4.     ├─AzureStor::storage_multiupload(...)
  5.     └─AzureStor:::storage_multiupload.blob_container(...)
  6.       └─AzureStor::multiupload_blob(container, ...)
  7.         └─AzureStor:::multiupload_internal(...)
  8.           └─AzureRMR::init_pool(max_concurrent_transfers)
  9.             └─parallel::makeCluster(size, ...)
 10.               └─parallel::makePSOCKcluster(names = spec, ...)
 11.                 └─parallel:::.check_ncores(length(names))

(e.g. https://github.com/rstudio/pins/pull/483/checks?check_run_id=3364204767)

It seems to be happening reliably on GHA, but I don't see the problem locally. Any ideas?

hadley commented 3 years ago

Oooh the problem only occurs in R CMD check, possibly because R CMD check limits the number of processes that you're allowed to create?

hadley commented 3 years ago

Yeah, this is parallel:::.check_ncores():

function (nc) 
{
    chk <- tolower(Sys.getenv("_R_CHECK_LIMIT_CORES_", ""))
    if (nzchar(chk) && (chk != "false") && nc > 2L) {
        msg <- sprintf("%d simultaneous processes spawned", nc)
        if (chk == "warn") 
            warning(msg, call. = FALSE, immediate. = TRUE)
        else stop(msg, call. = TRUE)
    }
}

So maybe init_pool() could do min(size, as.numeric(Sys.getenv("_R_CHECK_LIMIT_CORES_", Inf)) or similar.

hongooi73 commented 3 years ago

Right, this is why I skip testing the pool on CRAN: https://github.com/Azure/AzureRMR/blob/master/tests/testthat/test00_pool.R 😉

storage_multidownload/upload allows setting the maximum number of transfers, so you could insert a check there. I see that you've put the check in azure_cores; a more flexible solution might be to allow specifying the no. of transfers in pin_store. That way you can move the check entirely into the testing code.

hadley commented 3 years ago

Yeah, that's a good idea.