check_future_plan(): Variable `worker` not always defined

[HenrikBengtsson]

Running revdep checks on parallelly and future, I got:

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure (test_future.R:8:3): check_future_plan() works ──────────────────────
`check_future_plan()` threw an unexpected error.
Message: object 'worker' not found
Class:   simpleError/error/condition
Backtrace:
    ▆
 1. ├─testthat::expect_error(check_future_plan(), NA) at test_future.R:8:2
 2. │ └─testthat:::expect_condition_matching(...)
 3. │   └─testthat:::quasi_capture(...)
 4. │     ├─testthat .capture(...)
 5. │     │ └─base::withCallingHandlers(...)
 6. │     └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
 7. └─greta:::check_future_plan(

This is because worker is not guaranteed to be defined in:

https://github.com/greta-dev/greta/blob/4f61e0bd28cf564d1843645c0f528124295cd602/R/checkers.R#L552-L559

[HenrikBengtsson]

FYI, you can reproduce this by emulating running on a single-core machine when checking, e.g.

R_PARALLELLY_AVAILABLECORES_SYSTEM=1 R CMD check greta_0.4.2.tar.gz

This will cause plan(cluster) and plan(multisession) to fall back to sequential processing, which means that the Future object f created will inherit from sequential.

[njtierney]

Hi @HenrikBengtsson sorry for not replying to this sooner!

This will cause plan(cluster) and plan(multisession) to fall back to sequential processing, which means that the Future object f created will inherit from sequential.

This sounds like not a terrible result, if it ends up being sequential - I'm just trying to wrap my head around whether I should try and change the test, or change something in

https://github.com/greta-dev/greta/blob/4f61e0bd28cf564d1843645c0f528124295cd602/R/checkers.R#L552-L559

[HenrikBengtsson]

... or change something in

If inherits(workers, "cluster") is FALSE, variable worker is not set, so you'll get an error when is.null(worker$host) is called. I guess:

 if (inherits(workers, "cluster")) { 
   worker <- workers[[1]]  
   if (!is.null(worker$host)) { 
     localhosts <- c("localhost", "127.0.0.1", Sys.info()[["nodename"]]) 
     plan_is$local <- worker$host %in% localhosts 
   }
 } 

might work.

Though, I would start by making sure you can reproduce my original error, and then make sure it works after the fix.

[njtierney]

Hi @HenrikBengtsson - sorry for the delay on my end, I'm going to try and tackle this and #513 this week.

[HenrikBengtsson]

No worries and no rush

[njtierney]

OK, so I get the same error:

── Failure (test_future.R:8:3): check_future_plan() works ──────────────────────
`check_future_plan()` threw an unexpected error.
Message: object 'worker' not found
Class:   simpleError/error/condition
Backtrace:
    ▆
 1. ├─testthat::expect_error(check_future_plan(), NA) at test_future.R:8:2
 2. │ └─testthat:::expect_condition_matching(...)
 3. │   └─testthat:::quasi_capture(...)
 4. │     ├─testthat .capture(...)
 5. │     │ └─base::withCallingHandlers(...)
 6. │     └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
 7. └─greta:::check_future_plan()

As well as several other related errors, going to change the code as you've suggested above :)

[njtierney]

It seems that resolving this in https://github.com/greta-dev/greta/pull/536 would also resolve #513, does that sound right to you, @HenrikBengtsson ?

[HenrikBengtsson]

I think they're different. Fix one at the time. This one is trivial to fix, the other one probably needs some troubleshooting. But fix this one first.

[njtierney]

OK cool, I'm pretty sure that was all wrapped up in #536 - I'll take a closer look at #513 today