-
Notifications
You must be signed in to change notification settings - Fork 31
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
Uwot/umap crashes when run twice .... #39
Comments
Sorry you are having trouble. I am unable to reproduce the crash you give, even with If so, what does |
The current master might solve the issue. Please give it a try if possible and let me know. |
Yeah, looks nasty. Confirm that |
Thanks for confirming the issue @LTLA. That's one problem solved, but is it the problem? I suppose with uninitialized memory anything is possible, although it's odd that it works once then fails, and that more than one person reports in satijalab/seurat#2256 that installing RcppParallel from conda-forge solves the issue. Some compiler difference that initializes the memory differently? |
Hmm. The fact that it fails on the second go does suggest it's a memory leak of some sort rather than the The question is whether this is a memory leak in uwot or RcppParallel, given that the problem was "fixed" by reinstalling the latter from conda. Though given how much conda messes with the libraries, it feels like a house of cards to rely on that to solve this kind of problem. It would be nice to see what happens if someone can run Valgrind on a machine where the above code crashes. Might be pretty painful to do on Windows, though. |
Dear @LTLA and @jlmelville , I just installed the latest uwot code, reinstalled RcppParallel and ran the code again....it now works! RcppParallel::defaultNumThreads() gives 8, by the way (as it did before). Thanks for all your help!! |
I'm glad it's working now, but I am mystified as to why. Did you reinstall RcppParallel from CRAN or from conda forge? Edited to add: if |
I reinstalled using CRAN (should have said so in previous comment). I am also mystified, but I am not that well versed in programming/tracing/debugging to be able to find the source of what went wrong... |
I will note that in the following chunk of code: out <- prcomp(as.matrix(iris[,-5]))
library(irlba)
out <- irlba(as.matrix(iris[,-5]), nu=1, nv=1)
library(Rtsne)
out <- Rtsne(as.matrix(iris[,-5]), check_duplicates=FALSE)
library(uwot)
iris_umap <- umap(as.matrix(iris[,-5]), pca = 50, n_threads=1)
# And a second time
iris_umap2 <- umap(iris[,-5], pca = 50, n_threads=1) Only The first message looks something like this:
Definitely cryptic enough to be a parallelization issue. Rtsne also parallelizes but via OpenMP, which is generally more restrictive so it's harder to accidentally put in R API calls. |
Oops. Possibly maybe someone who shall remain nameless (spoiler alert: it's me) is calling the R random number generator from inside a thread? Fixing this isn't conceptually difficult, but requires a fair bit of typing (because it's C++) so might not get finished until later today. |
I forgot to tag the commit with this issue, but what's currently on master should hopefully behave. @LTLA, if you ever install from master, re-running valgrind would be an interesting exercise. |
|
FWIW, I directly ran valgrind, and A new version of |
I am still having issues with this even with the new version on cran. I reinstalled everything and tried again. But same as before on the second run of the example I get a seg fault. |
Operating system? |
It is a linux cluster, which unfortunately is running the 2.6 kernel. However, there doesn't seem to be any major issues with any other R package. Linux n6426 2.6.32-754.14.2.el6.x86_64 #1 SMP Tue May 14 19:35:42 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
Do you have If so, could you try copying the OP's code into some file (e.g.,
and seeing what Those diagnostics would be extremely helpful. |
Also could you run iris_umap <- umap(iris, pca = 50, verbose = TRUE, n_threads = 0) twice in a row, as well as repeating twice with: iris_umap <- umap(iris, pca = 50, verbose = TRUE, n_threads = 1) and see if either makes a difference, providing the output for the second crashing run. Getting a clue to where the second crash occurs would be helpful (although I suspect the damage is already done at some point in the first run). |
Edit: there is an explicit check that the number of components does not exceed the number of columns in the input, so for |
Reinstalling RcppParallel from CRAN after you have installed uwot etc? It seemed to help for me... |
If @theboocock has |
@LTLA , you're completely right!! Apologies for suggesting reinstallation BEFORE checking.... |
Hey all, Reinstalling never does anything for me anyways. Here is the valgrind error. Seems like it is coming from libtbb. Is this post relevant https://software.intel.com/en-us/forums/intel-threading-building-blocks/topic/641654?
|
@theboocock, thank you for running |
Well, it looks like it isn't even hitting uwot's C++ code, so it's hard to blame anything else... An even simpler test would be whether running # valgrind me:
library(RcppParallel)
setThreadOptions(numThreads = 1) If so, that seems like a slam dunk, though the use of conda does complicate matters. |
@LTLA, seeing as we get through one run without a crash, is it possible that |
I was looking at @theboocock's valgrind output above, where umap fails the first time it runs. (The difference from a non- It's also possible that # Put into test1.R with nothing else, and run under valgrind:
library(RcppParallel)
setThreadOptions(numThreads = 1) Or, if the above doesn't trigger the error, then: # Put into test2.R with nothing else, and run under valgrind:
library(uwot)
iris_umap <- umap(as.matrix(iris[,-5]), pca = 50, n_threads=1) |
I wonder if benjjneb/dada2#684 is a related problem? There are some suspicious similarities. |
Triggers the error for me. I am going to try the dada2 solution now., |
@jlmelville Yes! That fixed it. The patch added to rcpp-parallel on conda works for me.
|
This seems like the key piece
|
So the takeaway is that if you're running R under conda, you should be installing RcppParallel via conda as well? Not the most intuitive outcome, but tolerable. Possibly another thing to throw into the |
Yes, I was hoping to work out if there is a lesson learned here. I don't want to mislead anyone. I don't have any experience using |
@aldojongejan, you mentioned that you reinstalled RcppParallel from CRAN to fix the issue. Do you know if you had previously installed from conda? Or if you had a mix of conda-installed and CRAN-installed packages? |
I worked with the developers version of R to get the latest version of Seurat and SingleR working. Guess that that also installed RcppParallel. Then, as a possible fix for my problem, I installed via Conda as suggested here (cole-trapnell-lab/monocle3#186). That didn't solve it for me, only when I later reinstalled RcppParallel from CRAN again (removing the RcppParallel directory from the 'library' folder etc.)... I should have paid more attention and documented the exact steps... |
@aldojongejan, no problem, thank you for opening the issue here in the first place. |
Ha ha, opening the issue was not a real problem ;-) |
uwot 0.1.8 removes the dependency on RcppParallel so hopefully these problems are gone. |
Hopefully this is solved. Closing. |
Dear all,
I can't get umap to run twice in an R-session without crashing. Initially observed using RunUMAP from Seurat 3.1.1, but also the very basic code (see below) would not work..
I have tried to resolve this using the hints in cole-trapnell-lab/monocle3#186 and satijalab/seurat#2256, but without any succes..
Any suggestions?
Session and info:
library(uwot)
iris_umap <- umap(iris, pca = 50)
# And a second time
iris_umap2 <- umap(iris, pca = 50)
# Crash ....
#
# Bioconductor version [1] ‘3.10’
#
# R Under development (unstable) (2019-11-05 r77375)
# Platform: x86_64-w64-mingw32/x64 (64-bit)
# Running under: Windows >= 8 x64 (build 9200)
#
# Matrix products: default
#
# locale:
# [1] LC_COLLATE=English_United States.1252 LC_CTYPE=English_United States.1252 LC_MONETARY=English_United States.1252
# [4] LC_NUMERIC=C LC_TIME=English_United States.1252
#
# attached base packages:
# [1] stats graphics grDevices utils datasets methods base
#
# other attached packages:
# [1] uwot_0.1.5 Matrix_1.2-17
#
# loaded via a namespace (and not attached):
# [1] compiler_4.0.0 tools_4.0.0 yaml_2.2.0 Rcpp_1.0.3 grid_4.0.0 FNN_1.1.3 RcppParallel_4.4.4
# [8] lattice_0.20-38
thanks in advance and with kind regards,
Aldo
The text was updated successfully, but these errors were encountered: