search

jlmelville / uwot

An R package implementing the UMAP dimensionality reduction method.
https://jlmelville.github.io/uwot/
GNU General Public License v3.0
314 stars 31 forks source link

What C++ version should CXX_STD have? #107

Open jlmelville opened 1 year ago

jlmelville commented 1 year ago

Makevars has had:

CXX_STD = CXX11

for ages, but

https://cran.r-project.org/doc/manuals/r-devel/R-exts.html#Using-C_002b_002b-code says:

from R 4.0.0 a C++ compiler will be selected only if it conforms to the 2011 standard (‘C++11’) [...] The default standard for compiling R packages was changed to C++17 in R 4.3.0 if supported (and for rather old compilers, C++14 or even C++11 would be used as the default). [...] for maximal portability a package should specify the standard it requires [...] On the other hand, specifying C++1147 when the code is valid under C++14 or C++17 reduces future portability.

Seems like messing this up will be a great way to get booted off CRAN. I have no idea what the best choice is now. C++14?

LTLA commented 1 year ago

Is someone complaining about it? Seems fine to leave it as C++11. Must be loads of other packages doing that as well.

Alternative is to just not specify anything, and allow R to choose a C++11-compatible compiler. Almost everything should be compatible across C++ versions, unless you're fiddling with some of the edge cases (e.g., std::shuffle()).

jlmelville commented 1 year ago

Is someone complaining about it?

No I just saw some packages being changed to deal with this recently which made me take a look at the current R guidelines. I would remove the specification but:

If a package does have a src/Makevars[.win] file then also setting the make variable ‘CXX_STD’ there is recommended, as it allows R CMD SHLIB to work correctly in the package’s src directory.

Probably I'll just pretend I never saw any of this and leave things as they are.

jlmelville commented 11 months ago

Returning to this, for the RcppHNSW package, rhub::check_with_sanitizers() fails to build unless CXX_STD was set in the Makevars (the sheer amount of compilation time required to build uwot under these circumstances means it's unlikely I can confirm it would do the same). RcppAnnoy's Makevars explicitly sets this so I am leaning towards adding CXX_STD = CXX17 at some point.

jeremydavisturak commented 4 months ago

I'm not sure if this is relevant, but I'm (suddenly) having errors compiling uwot in a Docker container using

FROM r-base:4.0.5
RUN R -q -e "withCallingHandlers(install.packages('uwot'),warning = function(w) stop(w)) "

#0 9.057 r_uwot.cpp: In member function ‘std::unique_ptr<uwot::Optimizer> UmapFactory::create_optimizer(Rcpp::List)’:
#0 9.057 r_uwot.cpp:137:19: error: ‘make_unique’ is not a member of ‘std’
#0 9.057   137 |       return std::make_unique<uwot::Adam>(alpha, beta1, beta2, eps,
#0 9.057       |                   ^~~~~~~~~~~
#0 9.057 r_uwot.cpp:137:19: note: ‘std::make_unique’ is only available from C++14 onwards
jlmelville commented 4 months ago

Thanks for the report @jeremydavisturak. I submitted a new version of uwot to CRAN a few days ago so the timing is most suspicious.

Are you building directly from the master branch on this repo @jeremydavisturak or is the source coming from somewhere else? If I pushed a change to specify C++14 (ideally on a branch) would that be something that you could test?

jeremydavisturak commented 4 months ago

@jlmelville I'm doing an install straight from CRAN (install.packages), but I can try devtools:: install_github ; just let me know the branch or ref to point to

jeremydavisturak commented 4 months ago

I did a very small ad-hoc binary search:

commit 4e184028fec69a8e0eeff3154e6c0ebdd16363aa installed for me, but 72447a6f97972c4a343b942f5e008c9f3a8e32df did not

jeremydavisturak commented 4 months ago

This dockerfile can be used to test (my goal was to install Seurat, and uwot is a dependency). The apt-get lines were needed for installed devtools ...

FROM r-base:4.0.5
RUN apt-get update && apt-get install -y libcurl4-openssl-dev libssl-dev libxml2-dev libbz2-dev liblzma-dev libharfbuzz-dev libfribidi-dev
RUN apt-get update && apt-get install -y libfontconfig1-dev  libfreetype6-dev libpng-dev libtiff5-dev libjpeg-dev libgit2-dev

RUN echo 'options(repos = c(CRAN = "https://cloud.r-project.org"))' >>"${R_HOME}/etc/Rprofile.site"

## install devtools
RUN Rscript -e 'withCallingHandlers(install.packages(c("devtools")),warning = function(w) stop(w))';

# test devtools install
RUN Rscript -e 'require("devtools"); devtools::has_tests()'

## Install uwot from a specific commit
RUN R -q -e "devtools::install_github('jlmelville/uwot', ref='4e184028fec69a8e0eeff3154e6c0ebdd16363aa')"

# Install remotes (may be renundant)
RUN R -q -e "withCallingHandlers(install.packages(c('remotes')),warning = function(w) stop(w))"

# Install Seurat
RUN R -q -e "remotes::install_version(package = 'SeuratObject', version = package_version('4.1.4'), update='never')" && \
    R -q -e "remotes::install_version(package = 'Seurat', version = package_version('4.4.0'), update='never')" \
RUN R -q -e "library(Seurat)"
jlmelville commented 4 months ago

Am I right in thinking that your base image is:

FROM r-base:4.0.5

That might be part of the problem as that image is 3 years old. Any chance you can try building with a newer R? The only testing I've done (and the only CRAN checks) are for the development version of R, the current version of R (4.3.3) and the previous minor version (4.2.3) which was released over a year ago.

jlmelville commented 4 months ago

@jeremydavisturak I tried running the Dockerfile you provided with docker build -t myimage . but SeuratObject doesn't build due to an incompatibility with Matrix versions:

Error in loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]]) :
  namespace ‘Matrix’ 1.3-2 is being loaded, but >= 1.6.1 is required
Calls: <Anonymous> ... namespaceImportFrom -> asNamespace -> loadNamespace
Execution halted
ERROR: lazy loading failed for package ‘SeuratObject’
* removing ‘/usr/local/lib/R/site-library/SeuratObject’
Warning message:
In i.p(...) :
  installation of package ‘/tmp/Rtmp48PlOG/remotes717f0bf84/SeuratObject’ had non-zero exit status
jlmelville commented 4 months ago

Also @jeremydavisturak now I have looked closer at this: if you are working with a specific old version of R, a specific old version of Seurat and a specific old version of SeuratObject, why not also do the same for uwot? I test new versions of uwot with the current version of its reverse dependencies and the current and old release of R, but I can't guarantee things will work with arbitrary old versions.

fenguoerbian commented 4 months ago

I'm not sure if this is relevant, but I'm (suddenly) having errors compiling uwot in a Docker container using

FROM r-base:4.0.5
RUN R -q -e "withCallingHandlers(install.packages('uwot'),warning = function(w) stop(w)) "
#0 9.057 r_uwot.cpp: In member function ‘std::unique_ptr<uwot::Optimizer> UmapFactory::create_optimizer(Rcpp::List)’:
#0 9.057 r_uwot.cpp:137:19: error: ‘make_unique’ is not a member of ‘std’
#0 9.057   137 |       return std::make_unique<uwot::Adam>(alpha, beta1, beta2, eps,
#0 9.057       |                   ^~~~~~~~~~~
#0 9.057 r_uwot.cpp:137:19: note: ‘std::make_unique’ is only available from C++14 onwards

I'm also getting this error by installing source code from CRAN. I've also encounted this in some other packages, like this and this

jlmelville commented 4 months ago

@fenguoerbian what version of R are you using? From R 4.3 onwards, my understanding is that C++17 is assumed to be the standard. Adding SystemRequirements: C++14 will create a NOTE that I would have to justify with CRAN (and I won't be able to).

I build from source on Windows, Linux and Mac, and also as part of CI on Github Actions and again with rhub as part of preparation for CRAN submissions. None of these platforms fail to compile.

If you need this, can you add CXX_STD=CXX14 to your ~/.R/Makevars file?

fenguoerbian commented 4 months ago

@jlmelville I'm using R 4.2.2 on windows. If that's the case I'll just upgrade to 4.3 at some point. Also I believe adding CXX_STD=CXX14 to my local makevars file would work, thanks a lot.

DawnSLin89 commented 3 months ago

Hi @jlmelville I tried to install uwot a few times (on Mac), in different R version (4.4, 4.3, 4.2), from CRAN using install.packages() or devtools:: install_github.

Each time I got the same error messages (see below). The thing is, when I installed some other packages such as sctransform, c++17 was used. So I dont understand why c++11 was used when I tried to install uwot...

Thanks.

jlmelville commented 3 months ago

@DawnSLin89 that's confusing to me. Both the Mac github actions and my personal Mac build uwot without issue, but they pick up clang, not gcc:

* used C++ compiler: ‘Apple clang version 15.0.0 (clang-1500.0.40.1)’
* used SDK: ‘MacOSX14.4.sdk’

I assume you have all the XCode development tools installed? If not, maybe try that.

jeremydavisturak commented 2 months ago

@jlmelville FYI for the old Seurat version we're trying to support, I did get to work (but it's now breaking due to something in the OS for R 4.0.5 ...)

RUN R -q -e "devtools::install_github('jlmelville/uwot', ref='4e184028fec69a8e0eeff3154e6c0ebdd16363aa')"