Allow driver-specific environment variables #126
Conversation
|
|
||
|
|
||
| DEFAULT_ENVVARS <- hipercow::hipercow_envvars( # nolint | ||
| "CMDSTAN" = "I:/cmdstan", |
There was a problem hiding this comment.
Does this work when there are multiple installations within I:/cmdstan? In testing, I did this -
envvars <- c(hipercow_envvars("CMDSTAN" = "I:/cmdstan/cmdstan-2.35.0-rc3"),
hipercow_envvars("CMDSTANR_USE_RTOOLS" = "TRUE"))
There was a problem hiding this comment.
Yes, this works, though the mechanism in stan is byzantine. More details in the stan PR :)
tests/testthat/test-envvars.R
Outdated
| envvars_combine(NULL, | ||
| hipercow_envvars(ENV1 = "AA", ENV5 = "EE", ENV6 = "FF")), | ||
| hipercow_envvars(ENV2 = "b", ENV3 = "c", | ||
| ENV1 = "AA", ENV5 = "EE", ENV6 = "FF")) |
There was a problem hiding this comment.
Can I also remove a default environment variable by overwriting it with a NULL value? Eg, what if I have some very vindictive code that fails mysteriously if it detects the CMDSTAN env var is set...
There was a problem hiding this comment.
The convention that withr uses is that a value of NA_character_ removes an envvar. This probably needs adding into c.hipercow_envvars
There was a problem hiding this comment.
added support for this now
| envvars_combine <- function(driver_envvars, task_envvars) { | ||
| c(getOption("hipercow.default_envvars", DEFAULT_ENVVARS), | ||
| driver_envvars, | ||
| task_envvars) |
There was a problem hiding this comment.
Do we want to do
c(DEFAULT_ENVVARS,
driver_envvars,
getOption("hipercow.default_envvars"),
task_envvars)instead?
That's the behaviour suggested by the docs you've added.
On the other hand the DEFAULT_ENVVARS made some of the unit testing a bit annoying because they show up everywhere, so being able to disable them entirely was handy.
There was a problem hiding this comment.
That was what I had, then changed the behaviour back to your previous one because the testing was awkward...
There was a problem hiding this comment.
OK, I've gone this way I documented, despite the test pain; I think it's better
| @@ -70,6 +72,7 @@ c.hipercow_envvars <- function(...) { | |||
| } | |||
| ret <- rlang::inject(rbind(!!!inputs)) | |||
| ret <- ret[!duplicated(ret$name, fromLast = TRUE), ] | |||
There was a problem hiding this comment.
Should I have added drop = FALSE here as well?
There was a problem hiding this comment.
I've been doing a bunch with matrices recently where these are really important. I think that for data.frame's mine is superfluous actually - the drop here only would have an effect if we were subsetting columns (in which case with a single column will return the vector and not a data.frame of one column
Co-authored-by: Wes Hinsley <[email protected]>
Co-authored-by: Wes Hinsley <[email protected]>
Co-authored-by: Wes Hinsley <[email protected]>
Co-authored-by: Paul Liétar <[email protected]>
This will make the stan PR easier to understand. Allows a driver to provide its own default environment variables, which overlay over the hipercow defaults introduced in #125