klmr / box

Write reusable, composable and modular R code
https://klmr.me/box/
MIT License
833 stars 47 forks source link

Module not found when running on shiny 1.6.0 #237

Closed grst closed 2 years ago

grst commented 2 years ago

Sorry, it's me again with another edge case!

I have this minimal shiny app loading a module located in the same directory:

app.R

# works! 
source("./module.R")

# doesn't work!
box::use(
    ./module
)

ui = fluidPage()

server = function(input, output) {}

shinyApp(ui=ui, server=server)

When I run the app from the command-line, like this, everything works as expected (I navigated to http://localhost:3838 and see an empty page)

Rscript -e "shiny::runApp('app', port=3838)"

However, when I place the app in the apps directory of a shiny server installation and open the app in the browser, it fails with the following error message:

Error in box::use(./module) : 
  Unable to load module “./module”; not found in “/opt/shiny-server/R”
(inside “find_in_path(spec, calling_mod_path(caller))”)
Calls: runApp ... tryCatchList -> tryCatchOne -> <Anonymous> -> rethrow -> throw
Execution halted

Evidently, it can't find the module.R. Interestingly, sourcing the file with a relative path appears to work without issues.

Full repex

(using the rocker/shiny docker container)

#!/bin/bash

mkdir -p app

cat <<EOF > app/app.R
# works! 
source("./module.R")

# doesn't work!
box::use(
    ./module
)

ui = fluidPage()

server = function(input, output) {}

shinyApp(ui=ui, server=server)
EOF

cat <<EOF > app/module.R
foo = "bar"
EOF

cat <<EOF > Dockerfile
FROM rocker/shiny:4.1.1

RUN install2.r --error remotes && Rscript -e "remotes::install_github('klmr/box@dev')"
EOF

docker build . -t test_box_shiny 
docker run -v $(pwd)/app:/srv/shiny-server/box_test -p 3838:3838 test_box_shiny

Once the container is running, navigate to http://localhost:3838/box_test/

mrismailt commented 2 years ago

Try box::use(../module)

klmr commented 2 years ago

Thanks for the great reprex!

The reason this fails is that, in Shiny server, the result of shiny::isRunning() is different from when we’re running a Shiny app ourselves via runApp or similar: at the point where you’re calling box::use, the value of shiny::isRunning() is FALSE. And sure enough, when moving the box::use call into the server function, it succeeds (but of course this changes the semantics, and therefore won’t always work).

I’m honestly a bit at a loss at this behaviour of Shiny Server, and I’m wondering whether it might be a bug. As far as I know, ‘box’ might be the only package that’s using isRunning (the function was included specifically at my request). I’ll need to escalate this to the Shiny team, unfortuantely.

klmr commented 2 years ago

Ah, I completely misdiagnosed the issue …: isRunning is FALSE in both cases, and this makes sense (unfortunately for us). The actual issue is that Shiny Server invokes a new R script to start the app, /opt/shiny-server/R/SockJSAdapter.R, which makes ‘box’ think that this is the calling module.

klmr commented 2 years ago

More complications: the behaviour of isRunning actually changed between Shiny v1.5.0 and v1.6.0, specifically with commit rstudio/shiny@b1e5dd1d1d5435cb038909a3b910af2bb2013a06.

klmr commented 2 years ago

Could you please try the candidate hotfix by downloading the .tar.gz attached in the linked PR, or by executing

install.packages(
    'https://github.com/klmr/box/files/7117820/box_1.0.2.9000-shiny-hotfix.tar.gz',
    repos = NULL, type = 'source'
)

I think this fixes it (it does in my local testing, but confirmation from your specific use-case would be nice, since the fix is kind of a hack). Unfortunately the fix is also somewhat inefficient since it needs to inspect the call stack for each import when Shiny is loaded. Ideally the previous value of shiny::isRunning will be restored in the next revision of the Shiny package.

grst commented 2 years ago

This works now, also with my actual app :tada:

Yet, maybe a naive question: how comes you need to rely on shiny-specific internals like isRunning to correctly resolve the relative paths? This begs the question how robust this mechanism is in other cases... And source() can do the same without any hacks.

klmr commented 2 years ago

And source() can do the same without any hacks.

No, source can’t do that. source always resolves paths relative to the current working directory. By contrast, box::use resolves paths relative to the calling module, which isn’t always located in the working directory.

source just happens to (sometimes!) work in Shiny, because Shiny (for better or worse) sets the working directory to the app’s application at startup. But consider the following very simple case of a nested module dependency which no longer works with source but which does work with box::use:

shiny-test
├── app.r
└── utils
    ├── bar.r
    └── foo.r

With

This’ll fail because source can’t find bar.r; the author of foo.r needs to know how foo.r is invoked, and hard-code the path to bar.r accordingly; or the user of foo.r (i.e. app.r) needs to use source(…, chdir = TRUE), which potentially breaks other things. Either way destroys the reusability of foo.r, which is why ‘box’ was created in the first place.


And that, in a nutshell, is why ‘box’ needs to figure out whether it’s being called from inside Shiny (as well as a few other special cases): because it needs to figure out how to find relative imports outside of a module. Resolving them relative to the current working directory is generally not sufficient. Consider a script which is invoked as

$ pwd
/some/path
$ Rscript some/other/path/to/script.r

now the script itself isn’t inside the working directory (which is /some/path). If I now have box::use(./foo) inside that script then what I want to load is obviously some/other/path/to/foo.r, not /some/path/foo.r.

In fact, the reason why Shiny (as well as e.g. ‘knitr’) sets the working directory in the first place is probably to make source work, and all other bad consequences of setting the working directory be damned.

grst commented 2 years ago

Ah, I see now! Thanks for the extensive explanation!

Feel free to close the issue from my side.