ARROW-5488: [R] Workaround when C++ lib not available

[romainfrancois]

This is very much a work in progress. The idea is to replace the code generation that is usually done by Rcpp::compileAttributes() with something custom. This is driven by the data-raw/codegen.R file, which I'll polish. I'm opening this right now for feedback opportunities.

All of the .cpp files are guarded by :

#if defined(ARROW_R_WITH_ARROW)
... 
#endif

And ARROW_R_WITH_ARROW is defined via the configure file, if the library was indeed used.

For example, this function:

// [[arrow::export]]
std::shared_ptr<arrow::Column> ipc___feather___TableReader__GetColumn(
    const std::unique_ptr<arrow::ipc::feather::TableReader>& reader, int i) {
  std::shared_ptr<arrow::Column> column;
  STOP_IF_NOT_OK(reader->GetColumn(i, &column));
  return column;
}

triggers generation of this in the generated.cpp file:

#if defined(ARROW_R_WITH_ARROW)
std::string ipc___feather___TableReader__GetColumnName(const std::unique_ptr<arrow::ipc::feather::TableReader>& reader, int i);
SEXP _arrow_ipc___feather___TableReader__GetColumnName(SEXP reader_sexp, SEXP i_sexp){
  BEGIN_RCPP
  Rcpp::traits::input_parameter<const std::unique_ptr<arrow::ipc::feather::TableReader>&>::type reader(reader_sexp);
Rcpp::traits::input_parameter<int>::type i(i_sexp);Rcpp::Shield<SEXP> rcpp_result_gen(Rcpp::wrap(ipc___feather___TableReader__GetColumnName( reader, i)));
return rcpp_result_gen;
  END_RCPP
}
#else
SEXP _arrow_ipc___feather___TableReader__GetColumnName(SEXP reader_sexp, SEXP i_sexp){
  BEGIN_RCPP
  Rcpp::stop("arrow C++ library not available");
  END_RCPP
}
#endif

So the generated R api SEXPy functions only call the real thing when the c++ library is available, otherwise they just throw an error.

and this in the generated.R file:

ipc___feather___TableReader__GetColumnName <- function(reader, i) {
    .Call(`_arrow_ipc___feather___TableReader__GetColumnName` , reader, i )
}

This also needed some extra care in test functions so that the tests only run if Arrow is available.

@wesm comment from https://issues.apache.org/jira/browse/ARROW-5488 might be more practical and closer to what @jjallaire mentioned about how the RcppParallel package does it with Intel tbb:
https://github.com/RcppCore/RcppParallel/blob/master/R/hooks.R

One possibility is to bundle the Arrow header files with the CRAN package and build against them, but do not include libarrow and libparquet when linking. When the library is loaded, the libraries must be loaded in-process via dlopen before loading the Rcpp extensions. The C++ libraries can be installed then after the fact

[wesm]

So what would occur if:

• User installs the package without the C++ library available
• User then installs the Arrow C++ libraries

Is the installed R package broken until it is reinstalled? If the Arrow C++ headers are bundled privately with the R package then it should be able to build against them, and you can omit -larrow -lparquet at link time. The question is whether you can get library(arrow) to do a dynload of libarrow.so prior to attempting to load the Rcpp extensions

[nealrichardson]

Correct, under this approach, the R package would not find the C++ library if it were installed after the R package were installed. That said, when we discussed this approach before, we proposed adding an arrow::install_arrow() function that the error message would suggest that you run, and this function could, after installing the C++ library, reinstall the R package itself to fix it. So the user experience either with this approach or with the bundling of headers like you suggest would be the same: if you install the R package without Arrow C++ present, when you try to use the package, you'll be told to call arrow::install_arrow(), the result of which will be a functioning C++/R arrow environment.

[romainfrancois]

@wesm I'm going to try the approach of bundling headers in inst/include perhaps only when they're not found otherwise.

I'm just a little worried about code duplication and bloat for the R package, but this is less problematic as if we were bundling all of arrow (*.h and *.cpp).

If we are successful here, then I guess we can do something similar to what @jjallaire did with RcppParallel, i.e. not using @useDynLib as per usual but rather first dynamically load the arrow lib and then load the library of the package, i.e.: https://github.com/RcppCore/RcppParallel/blob/master/R/hooks.R#L5

.onLoad <- function(libname, pkgname) {
   
   # load tbb and tbbmalloc on supported platforms   
   tbb <- tbbLibPath()
   if (!is.null(tbb)) {
      if (!file.exists(tbb)) {
         warning(paste("TBB library", tbb, "not found."))
      } else {
         dllInfo <<- dyn.load(tbb, local = FALSE, now = TRUE)
      }
   }
   tbbMalloc <- tbbLibPath("malloc")
   if (!is.null(tbbMalloc)) {
      if (!file.exists(tbbMalloc)) {
         warning(paste("TBB malloc library", tbbMalloc, "not found."))
      } else {
         mallocDllInfo <<- dyn.load(tbbMalloc, local = FALSE, now = TRUE)
      }
   }
   
   # load the package library
   library.dynam("RcppParallel", pkgname, libname)
   
   # set default thread options
   numThreads <- "auto"
   numThreadsEnv <- Sys.getenv("RCPP_PARALLEL_NUM_THREADS", unset = NA)
   if (!is.na(numThreadsEnv))
      setThreadOptions(numThreads = as.integer(numThreadsEnv))
   else
      setThreadOptions(numThreads = "auto")
}

[jjallaire]

I easily might be missing something here, so take my comments w/ a grain of salt if my mental picture is incomplete.

I'm wondering a couple of things about including the headers:

1. I realize that we can optionally omit omit -larrow -lparquet when we know Arrow isn't installed, but wouldn't our C++ library still have unresolved symbols and as a result yield build errors? (or perhaps there is a way to suppress those)

2. I would guess that we strongly prefer to compile against the headers from the same version of Arrow that we are linking against (otherwise there can be skew/incompatibility). I guess we could only build against those headers in the "arrow not installed" scenario, but in that case I'm not sure what it's buying us.

[wesm]

@jjallaire

On #1, on Linux I believe if the symbols that the Rcpp library are available in process (e.g. having been loaded with dlopen earlier) then they'll be resolved correctly (this will only work on Linux, though)? I might be wrong about this.

On #2, yes if the user has some other version of the shared libraries installed then ABI mismatch will most likely cause symbol resolution failure more often than a flat out crash. The shared libraries have ABI tags (e.g. libarrow.so.13) so we can check for this if it becomes a problem

[jjallaire]

@wesm My worry though is that when we are building/testing on CRAN, we won't have loaded the symbols w/ dlopen (since Arrow won't be on the build servers), so there will be a link failure. My hope is that when on systems w/o Arrow (inevitable on CRAN) we still build, link, and run tests "successfully". In this mode the package is effectively a shim that tells you to call install_arrow() after which point you'll be good to go (which is basically the experience we have now for Spark and TF).

w/r/t ABI issues, I'm wondering if we are always better off building against the headers that match the library (i.e. I'm not sure what including the headers in the package buys us). Forgive me for being dense if it's implicit in some previous discussion!

[nealrichardson]

A few notes. It might also be good to add a travis job to check the package without the arrow C++ present.

[nealrichardson]

When does data-raw/codegen.R get called? Can you please update the README with developer instructions (and if necessary/appropriate, add Suggests dependencies to DESCRIPTION)?

[romainfrancois]

Sure. Will do that when I'm sure this is solid.

[romainfrancois]

I guess I need to edit the ./configure error here:

# Customize the error
if [ $? -ne 0 ]; then
  echo "------------------------- ANTICONF ERROR ---------------------------"
  echo "Configuration failed because $PKG_CONFIG_NAME was not found. Try installing:"
  echo " * brew: $PKG_BREW_NAME (Mac OSX)"
  echo "If $PKG_CONFIG_NAME is already installed, check that 'pkg-config' is in your"
  echo "PATH and PKG_CONFIG_PATH contains a $PKG_CONFIG_NAME.pc file. If pkg-config"
  echo "is unavailable you can set INCLUDE_DIR and LIB_DIR manually via:"
  echo "R CMD INSTALL --configure-vars='INCLUDE_DIR=... LIB_DIR=...'"
  echo "--------------------------------------------------------------------"

  PKG_LIBS=""
  PKG_CFLAGS=""
else
  PKG_CFLAGS="$PKG_CFLAGS -DARROW_R_WITH_ARROW -DARROW_R_WITH_PARQUET"
fi

so that it says a shim package has been compiled and the user should use arrow::install_arrow() to install lib arrow and reinstall the package.

I'll probably do install_arrow() in another PR though.

The files R/generated.R and src/generated.cpp are generated by the data-raw/codegen.R file, so now it is a manual thing to source this file every time something changes in a .cpp file. Is there a hook in pkgbuild to trigger code generation with custom code instead of Rcpp::compileAttributes() ? @jimhester @jjallaire (I'm pretty sure there isn't, but could there be)

[jjallaire]

There isn't a hook like this that I'm aware of (I guess configure could do it if could successfully detect changes). There is this function from the utils package that lets you easily do before/after snapshots of a directory to detect changes: https://stat.ethz.ch/R-manual/R-devel/library/utils/html/changedFiles.html

[jimhester]

You could regenerate it unconditionally in configure, it should be fast, so there is probably no harm in doing it every time configure is called, assuming the output is deterministic across platforms for the same input file.

[jjallaire]

+1 on doing it unconditionally. Assuming it has similar overhead to Rcpp::compileAttributes it won't be noticeable.

…

On Thu, Jun 6, 2019 at 9:53 AM Jim Hester ***@***.***> wrote: You could regenerate it unconditionally in configure, it should be fast, so there is probably no harm in doing it every time configure is called, assuming the output is deterministic across platforms for the same input file. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4471?email_source=notifications&email_token=AAAZPR4G3FWFDKATTQQ5K5LPZEJGBA5CNFSM4HS44S4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXC466I#issuecomment-499502969>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAZPR2SU5J5CYFDEGXMKQTPZEJGBANCNFSM4HS44S4A> .

[romainfrancois]

❯ system.time(source('~/git/apache/arrow/r/data-raw/codegen.R'))
   user  system elapsed 
  0.656   0.036   0.694 

The only thing is that it uses a bunch of packages

# pak::pkg_install("romainfrancois/decor")
library(decor)

library(dplyr)
library(stringr)
library(purrr)
library(glue)

[jjallaire]

So I guess we don't want those packages to become dependencies of the arrow package. I'd say just have a special environment variable that triggers the use of codegen.R within configure so those packages don't have to be declared in Suggests?

…

On Thu, Jun 6, 2019 at 10:14 AM Romain François ***@***.***> wrote: ❯ system.time(source('~/git/apache/arrow/r/data-raw/codegen.R')) user system elapsed 0.656 0.036 0.694 The only thing is that it uses a bunch of packages # pak::pkg_install("romainfrancois/decor") library(decor) library(dplyr) library(stringr) library(purrr) library(glue) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4471?email_source=notifications&email_token=AAAZPR3WY5TF6QGT46773MDPZELU3A5CNFSM4HS44S4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXC7HFY#issuecomment-499512215>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAZPRYZRDMUQFKTO7WYVGLPZELU3ANCNFSM4HS44S4A> .

[nealrichardson]

Also, data_raw/ is in .Rbuildignore so IIUC we couldn't call data_raw/codegen.R in configure because the file won't be present in the built package (unless we were to move it elsewhere).

[romainfrancois]

@nealrichardson I believe this is ready for another review now. I will open another JIRA/PR for the install_arrow() function.

Code generation is triggered on each ./configure if the ARROW_R_DEV environment variable is set to TRUE. Only on systems that use configure (i.e. not windows). I think that's fine because it's a development time thing, i.e. the generated files are pushed.

The code generation is somewhat craft right now, I'm hoping to make progress with my hobby decor package later.

[wesm]

@romainfrancois since we're talking about interactions with GPL2 build tools can you confirm that this patch includes no reused code from RcppCore/Rcpp?

[romainfrancois]

Yes. It generates similar code (and Rcpp is still being used in the package), but this patch does not borrow code from Rcpp.

[wesm]

OK, thanks. Note that it's fine to utilize GPL tools as part of a build system (for example: GNU autotools), but it's the code reuse / copying where things become problematic

[nealrichardson]

A few 💅 ✍️ comments, but assuming CHECK is passing everywhere, looks like we're really close here.

To @jjallaire 's point about the examples, that's interesting, I hadn't thought about the prospect that readers of the help pages might see the examples and then think they need to wrap everything in if(arrow_available()) {...}, so it would be better to make them all \dontrun. Have we seen that happen for other packages?

A counterargument would be that if we did keep the arrow_available() check in the examples and let them run, not only would we avoid any potential CRAN litigation about \dontrun, we would also get examples with rendered output in the pkgdown site, which would be nice.

[nealrichardson]

This script and the ARROW_R_DEV env var should be documented at least minimally for developers. Somewhere around here in the readme it should say that anyone working on the C++ code in the R package should export ARROW_R_DEV=TRUE and will also need to remotes::install_github("romainfrancois/decor").

It might then be sufficient to have a brief comment here at the top of this file (below the 🐀 license) just explaining that this is a supplement to the usual Rcpp magic and wraps all functions in a check for the existence of the Arrow library, or something.

[romainfrancois]

I added some words

[nealrichardson]

Minor point but is "codegen" the best name for this?

[romainfrancois]

🤷‍♂

[nealrichardson]

I guess Rcpp::compileAttributes() isn't all that vivid either ;)

[nealrichardson]

This function could use some more substantive docs, like to say that package users generally wouldn't need to call this, to address the concern that JJ raised about the examples.

[jjallaire]

Yes, we use \dontrun extensively for TensorFlow, Spark, and reticulate. We do however have example functions for any code that doesn't have dependencies (in this case e.g. arrow_available()) so that we don't trip the "no running examples" flag.

pkgdown is actually setup to deal with this pretty gracefully (example code is enclosed in a subtle not run treatment, e.g. see https://keras.rstudio.com/reference/keras_model.html)

[nealrichardson]

Right, pkgdown handles dontrun gracefully, but the downside is that you don't get to see the example results, like with e.g. https://dplyr.tidyverse.org/reference/select.html#examples. Provided that we build the pkgdown static site in an environment where libarrow is installed (which we will), if we used the arrow_available() check in the examples, we'd get the example output showing. For pedagogical purposes, I'm not sure which is the better tradeoff (i.e. see real output but possibly be mislead that you always have to check if arrow is available, vs. no output but also no extraneous code).

[nealrichardson]

Looks like pkgdown has a run_dont_run argument (https://github.com/r-lib/pkgdown/blob/89af5341ef2a0eedf07770d3e98f5094dacb9029/R/build-reference.R#L86), so we can have the cleaner \dontrun and still get example output on the pkgdown site.

[romainfrancois]

Sure. I'd like to document this function in a subsequent pull request, maybe together with install_arrow().

[jjallaire]

FWIW don't think it's worth polluting the examples with `arrow_available()`. It's both inelegant and probably misleading as users might add this to their code.

…

On Mon, Jun 10, 2019 at 5:41 PM Neal Richardson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In r/R/zzz.R <#4471 (comment)>: > @@ -18,3 +18,11 @@ #' @useDynLib arrow, .registration = TRUE #' @importFrom Rcpp sourceCpp NULL + +#' Is the C++ Arrow library available +#' +#' @export Right, pkgdown handles dontrun gracefully, but the downside is that you don't get to see the example results, like with e.g. https://dplyr.tidyverse.org/reference/select.html#examples. Provided that we build the pkgdown static site in an environment where libarrow is installed (which we will), if we used the arrow_available() check in the examples, we'd get the example output showing. For pedagogical purposes, I'm not sure which is the better tradeoff (i.e. see real output but possibly be mislead that you always have to check if arrow is available, vs. no output but also no extraneous code). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4471?email_source=notifications&email_token=AAAZPR5WAV2WISIIIZCSIPDPZ3C73A5CNFSM4HS44S4KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3DAYLI#discussion_r292203237>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAZPR7EIS6EN5NOSPMR7STPZ3C73ANCNFSM4HS44S4A> .

[nealrichardson]

@romainfrancois just came across this script that probably needs updating with your change: https://github.com/apache/arrow/blob/master/ci/docker_build_r.sh#L26-L27

[romainfrancois]

@jjallaire thanks, I did remove them based on your previous comment.

[romainfrancois]

Thanks @nealrichardson I did not know about these docker files, I don't really speak docker, I guess I should. @jameslamb @xhochy I updated them to the best of my abilities given other changes in this pull request, it should be fine, but you might want to check.

[romainfrancois]

As long as redoing code generation, perhaps it would be useful to spit out several files in a directory instead of the giant arrowExports.cpp to limit merge conflicts ...

[nealrichardson]

LGTM; I say merge this and we can follow with any cleanups after.

[nealrichardson]

I guess Rcpp::compileAttributes() isn't all that vivid either ;)