rticulate / import

An Import Mechanism For R
https://import.rticulate.org
Other
222 stars 14 forks source link

`import::from()` not working inside package function #71

Open brshallo opened 2 years ago

brshallo commented 2 years ago

I'm getting the following error when running import::from() from the debugger after loading a personal package.

import::from("purrr", "map", .into = "explictpackage:purrr", .character_only = TRUE)
#> Error in as.environment(where) : 
#>   no item called "explictpackage:purrr" on the search list

Strangely I don't get any error when running that line from the debugger after doing something like debugonce("mean"); mean(1:10). But I had to revert back to using the approach mentioned in #53 for attaching functions here: https://github.com/brshallo/funspotr/blob/issue-5/R/spot-funs.R#L104

torfason commented 2 years ago

Well, that's a bummer! I hope that this was not a live error while presenting, and that it did not prevent you from submitting funspotter to CRAN.

Not sure what the solution here is though:

brshallo commented 2 years ago

Hah, no, nothing like that. I held off on submitting funspotr for now because was re-writing the API for the talk which I've only just finished. (Though I will want to update this part before before submitting as w/o it I'm stuck with a pesky "note".)

I'll check that out, may have been a typo but yeah wouldn't think that would matter. No I don't have any expectation about colons. In {funspotr} I just use explicitpackage: for my internal labeling to identify packages in a file that are used like pkg::fun() rather than library(pkg); fun() but there's nothing about the colon or syntax in particular.

I was actually noticing the issue initially when using import::from() within callr::r() which first made me suspicious that there may be something weird happening when dealing with nested R processes but I'll try and open a minimal reprex here next week and also make sure that I'm not just doing something wrong in my set-up.

torfason commented 2 years ago

Sounds like a plan!

Note that main (currently at v1.3.0.9003) has a somewhat deep internal change compared to the CRAN release. If you add a reprex, it would be great if you could note whether it happens with the CRAN release, the main branch, or both.

torfason commented 11 months ago

I did not hear more about this. The v.1.3.1 release is now out, and I'm doing a bit of a cleanup. It has seemed to me that in general, the best way forward in complex situations regarding environment is to use import::here(). If that is not working to resolve this case, or if there is a neat solution that you could propose, feel free to reopen the issue.

brshallo commented 11 months ago

The problem with import::here() is I don't have the flexibility that comes with the .into arg.

This is the function where I'm using import::from() and the intended output:

attach_pkg_fun <- function(pkg_fun){
  pkg_nm <- pkg_fun$pkg
  fun_nm <- pkg_fun$fun
  pkg_nm_exp <- paste0("explicitpackage:", pkg_nm)

  import::from(pkg_nm, fun_nm, .into = pkg_nm_exp, .character_only = TRUE)
}

pkg_fun <- list(pkg = "purrr", fun = "map")
attach_pkg_fun(pkg_fun)

search()
#>  [1] ".GlobalEnv"            "explicitpackage:purrr" "package:stats"        
#>  [4] "package:graphics"      "package:grDevices"     "package:utils"        
#>  [7] "package:datasets"      "package:methods"       "Autoloads"            
#> [10] "tools:callr"           "package:base"

The above output is working as intended (when the function is in a standalone script) as you see "explicitpackage:purrr" has been added to the second position on the search path.

However if I try calling this from a (dev version of a) package, you'll see I get an error and the search path is not edited.

pkg_fun <- list(pkg = "purrr", fun = "map")
funspotr:::attach_pkg_fun(pkg_fun)
#> Error in as.environment(where): no item called "explicitpackage:purrr" on the search list

search()
#>  [1] ".GlobalEnv"        "package:stats"     "package:graphics" 
#>  [4] "package:grDevices" "package:utils"     "package:datasets" 
#>  [7] "package:methods"   "Autoloads"         "tools:callr"      
#> [10] "package:base"

This seems like some kind of environment issue that comes from it being in a package but haven't isolated it.

torfason commented 11 months ago

OK, happy to leave this open. One question, is there a possibility import get around this using environment pointers rather than strings. As in:

> x = import::into(new.env(), "%<>%", .from = magrittr)
> import::into(.GlobalEnv, "%>%", "%$%", .from = magrittr)

I realize this would not solve anything directly, what I was thinking was either:

a. Use base R to get a pointer to the named environment (assign it to a variable), and then use this method to import into it. b. First import into a temporary environment and then use some base R commands to copy to the correct place.

brshallo commented 11 months ago

Putting things in .GlobalEnv won't work for my use case as I need it to identify the function as coming from that package.

I did try swapping import::from() for import::into() just to see if changing syntax would magically fix things but didn't seem to.

torfason commented 11 months ago

Sorry, I was not suggesting to use the actual .GlobalEnv, just demonstrating this syntax. My question would be, does the following approach work for you:

my_package_local <- new.env()
import::from(dplyr, select, .into = my_package_local)
attach(my_package_local, name = "my_package") 
search()
#>  [1] ".GlobalEnv"        "my_package"        "package:stats"    
#>  [4] "package:graphics"  "package:grDevices" "package:utils"    
#>  [7] "package:datasets"  "package:methods"   "Autoloads"        
#> [10] "tools:callr"       "package:base"
ls(pos = "my_package")
#> [1] "select"

Knowing the answer would help diagnose whether this is import doing something strange, or whether it is external to that.

brshallo commented 11 months ago

Yes, that works. That's the approach I've been taking brshallo/funspotr/spot_funs/L119. I'd like to get rid of attach() though so that I no longer get a NOTE when running devtools::check() (even though my use of attach isn't actually dangerous as I do it from a separate R instance).

I think this likely points to some edge case issue with {import} though as well.

torfason commented 11 months ago

OK, that definitely points to an edge case in import even though it does not give us any blueprint for fixing this. So this will stay open for now.

FWIW, this is how import gets rid of the NOTE:

  make_attach <- attach # Make R CMD check happy.
  if (use_into && !into_exists)
    make_attach(NULL, 2L, name = .into)