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

ROBUSTNESS: Use defaults for unset ff* options #14

Open
HenrikBengtsson opened this issue May 10, 2022 · 1 comment
Open

ROBUSTNESS: Use defaults for unset ff* options #14

HenrikBengtsson opened this issue May 10, 2022 · 1 comment

Comments

@HenrikBengtsson
Copy link

HenrikBengtsson commented May 10, 2022

Issue

If some functions, own or third-party, resets all R options to what they were prior to loading ff, then some ff functions fail because querying those options result in NULL values. Imagine a function that reset all R options using on.exit() to protect against side effects from some rough function call setting, say, options(warn = 2).

Reproducible example

library(ff)

## ff options
names <- grep("^ff", names(options()), value = TRUE)
opts <- options()[names]

x <- ff(1:3)

## remove ff options
reset <- setNames(vector("list", length = length(opts)), names(opts))
options(reset)

y <- ff(1:3)
#> Error in splitted$path[nopath] <- getOption("fftempdir") : 
#>  replacement has length zero

Troubleshooting

The problem is that ff::fftempfile() assumes that R option fftempdir is always set;

ff/R/fileutil.R

Lines 181 to 187 in 46bc1c5

fftempfile <- function(x){
splitted <- splitPathFile(x)
nopath <- splitted$path=="" & splitted$fsep==""
if (any(nopath))
splitted$path[nopath] <- getOption("fftempdir")
tempPathFile(splitted, extension=getOption("ffextension"))
}

Suggestion

I'd like to argue that code should not make assumptions about R options being set, even for "internal" R options. My suggestions is to add defaults to all getOption(), e.g.

fftempfile <- function(x){
  splitted <- splitPathFile(x)
  nopath <- splitted$path=="" & splitted$fsep==""
  if (any(nopath)) {
    path <- getOption("fftempdir")
    if (is.null(path)) path <- standardPathFile(file.path(tempdir(), "ff"))
    splitted$path[nopath] <- path
  }
  tempPathFile(splitted, extension=getOption("ffextension"))
}

Alternatively, give an informative error message:

path <- getOption("fftempdir", stop("Option 'fftempdir' has been removed. It is essential for the 'ff' package to function. Please restart your R session"))

Session info

> sessionInfo()
R version 4.2.0 (2022-04-22)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 20.04.4 LTS

Matrix products: default
BLAS:   /home/hb/shared/software/CBI/R-4.2.0-gcc9/lib/R/lib/libRblas.so
LAPACK: /home/hb/shared/software/CBI/R-4.2.0-gcc9/lib/R/lib/libRlapack.so

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] ff_4.0.7  bit_4.0.4

loaded via a namespace (and not attached):
[1] compiler_4.2.0
@HenrikBengtsson
Copy link
Author

HenrikBengtsson commented Feb 22, 2023

A friendly nudge about this one. I don't think resetting R options should break a package this much. I think it wouldn't be too hard to recover from it, if it happens.

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

No branches or pull requests

1 participant