Skip to content
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

R installations possibly broken #919

Closed
martenson opened this issue Oct 14, 2015 · 17 comments
Closed

R installations possibly broken #919

martenson opened this issue Oct 14, 2015 · 17 comments
Labels

Comments

@martenson
Copy link
Member

details here: #893 (comment)

ping @yhoogstrate @mvdbeek @bgruening @jmchilton

@bgruening
Copy link
Member

working on this ...
Potential fix here: https://github.com/galaxyproject/galaxy/compare/dev...bgruening:R_return?expand=1
Just want to search for more test computers with different R versions and OS ...
R is not really portable and error handling seems to be very tricky :(

@nsoranzo
Copy link
Member

@bgruening @yhoogstrate Your fix is wrong, it will return 0 also if Rscript fails for a random reason. The original problem is with the syntax of the error parameter of tryCatch. Can you test if this commit fixes the problem for you:

nsoranzo@73ca4c7

It works correctly for me on the command line.

@bgruening
Copy link
Member

@nsoranzo for some reason the tryCatch is not working for all users.
Please see the latest discussion here:
bgruening#8

Including a new idea using Python to catch the error.

@nsoranzo
Copy link
Member

I guess tryCatch was not working because of the problem fixed in nsoranzo/galaxy@73ca4c7 . I think my solution is less hacky and does not rely on grepping the Rscript stdout. I'm doing more testing now.

@bgruening
Copy link
Member

@nsoranzo even the old version was working for me in one R version :)
If you think this works for every R version including R2 than I'm happy to take this one.

@bgruening
Copy link
Member

This is what I get:

root@4e0ee2d18014:/galaxy-central# R --version
R version 3.0.2 (2013-09-25) -- "Frisbee Sailing"
Copyright (C) 2013 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under the terms of the
GNU General Public License versions 2 or 3.
For more information about these matters see
http://www.gnu.org/licenses/.

root@4e0ee2d18014:/galaxy-central# Rscript -e "tryCatch({ install.packages(c('BiocGenerics_0.14.0.tar.gz-'), lib = '/galaxy-central/tool_deps/deseq2/1.8.2/iuc/package_deseq2_1_8_2/00b14423d00f', repos=NULL, dependencies=FALSE) }, error = function(e) { print(e); quit(status = 1) } )"
Warning: invalid package ‘BiocGenerics_0.14.0.tar.gz-’
Error: ERROR: no packages specified
Warning message:
In install.packages(c("BiocGenerics_0.14.0.tar.gz-"), lib = "/galaxy-central/tool_deps/deseq2/1.8.2/iuc/package_deseq2_1_8_2/00b14423d00f",  :
  installation of package ‘BiocGenerics_0.14.0.tar.gz-’ had non-zero exit status
root@4e0ee2d18014:/galaxy-central# echo $?
0

@yhoogstrate
Copy link
Member

$ R --version
R version 3.2.0 (2015-04-16) -- "Full of Ingredients"
Copyright (C) 2015 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under the terms of the
GNU General Public License versions 2 or 3.
For more information about these matters see
http://www.gnu.org/licenses/.

Test 1: correct package, incorrect destination:

$ Rscript -e "tryCatch({ install.packages(c('BiocGenerics_0.14.0.tar.gz'), lib = '/dev/null', repos=NULL, dependencies=FALSE) }, error = function(e) { print(e); quit(status = 1) } )"
Warning in install.packages(c("BiocGenerics_0.14.0.tar.gz"), lib = "/dev/null",  :
  'lib = "/dev/null"' is not writable
<simpleError in install.packages(c("BiocGenerics_0.14.0.tar.gz"), lib = "/dev/null",     repos = NULL, dependencies = FALSE): unable to install packages>
$ echo $?
1

Test 2: incorrect package, correct destination:

$ Rscript -e "tryCatch({ install.packages(c('/dev/null'), lib = '/tmp/deseq2/inst', repos=NULL, dependencies=FALSE) }, error = function(e) { print(e); quit(status = 1) } )"
ERROR: cannot extract package from ‘/dev/null’
Warning message:
In install.packages(c("/dev/null"), lib = "/tmp/deseq2/inst", repos = NULL,  :
  installation of package ‘/dev/null’ had non-zero exit status
$ echo $?
0

Edit
Test 3: corrupt .tar.gz file, correct destination:

$ Rscript -e "tryCatch({ install.packages(c('Bio2.tar.gz'), lib = '/tmp/deseq2/inst2', repos=NULL, dependencies=FALSE) }, error = function(e) { print(e); quit(status = 1) } )"
Error in untar2(tarfile, files, list, exdir, restore_times) : 
  incomplete block on file
Warning message:
In install.packages(c("Bio2.tar.gz"), lib = "/tmp/deseq2/inst2",  :
  installation of package ‘Bio2.tar.gz’ had non-zero exit status
$ echo $?
0

@nsoranzo
Copy link
Member

Yep, it looks like an invalid package name raise a warning instead of an error. Should we exit with status = 1 also for warnings?

@bgruening
Copy link
Member

@nsoranzo I haven't done this because I fear that during compilations there are a lot of warnings.

@nsoranzo
Copy link
Member

Should be fixed by nsoranzo@f666b05

@yhoogstrate
Copy link
Member

@nsoranzo if ( grep('had non-zero exit status', as.character(w)) Pure ownage. Shame on us @bgruening 😆

The three tests cases I wrote in the comment above work currectly using nsoranzo@f666b05

@bgruening
Copy link
Member

@nsoranzo your last one works for all of my examples. You probably want to include a few more strings to grep for, as shown by @yhoogstrate. Not sure if all are errors or a few of them are warnings.
Also we should consider that we need to add more filters later if R changes the error messages.

Please decide yourself if you want to have this one-liner or if we want to create a Python template without tryCatch(). The grep functions seems to be stable since 2.11 so this could be a valid solution.

@nsoranzo
Copy link
Member

@yhoogstrate How can I reproduce unable to install packages and Execution halted errors/warnings?

@yhoogstrate
Copy link
Member

@nsoranzo

$ Rscript -e "install.packages(c('BiocGenerics_0.14.0.tar.gz'),lib='/dev/null', repos=NULL, dependencies=FALSE)" 
Warning in install.packages(c("BiocGenerics_0.14.0.tar.gz"), lib = "/dev/null",  :
  'lib = "/dev/null"' is not writable
Error in install.packages(c("BiocGenerics_0.14.0.tar.gz"), lib = "/dev/null",  : 
  unable to install packages
Execution halted

This makes me realize that your grep should not be able to capture this warning, but it still returns an error. I think we should look a bit closer into it, unless I made a mistake in copy pasting:

$ Rscript -e "tryCatch( { install.packages(c('BiocGenerics_0.14.0.tar.gz'), lib = '/dev/null', repos = NULL, dependencies = FALSE) }, error = function(e) { print('--Error--'); quit(status = 1) }, warning = function(w) { if ( grep('ZZZZZZZZZZZZZZZZZZZZZZZZZZZZz', as.character(w)) ) { print('--Warning--'); quit(status = 1) } } )"
Error in if (grep("ZZZZZZZZZZZZZZZZZZZZZZZZZZZZz", as.character(w))) { : 
  argument is of length zero
Calls: tryCatch -> tryCatchList -> tryCatchOne -> <Anonymous>
Execution halted

@nsoranzo
Copy link
Member

@yhoogstrate The problem is that I should have used grepl instead of grep (which returns an empty vector if there is no match). It should be finally fixed with nsoranzo@c9294a6 , where I also added all warnings found in the install.packages() source code.

@yhoogstrate
Copy link
Member

@nsoranzo Thanks, it works here. I think you should make a PR and in the meantime we should test the packages within:

package_dexseq_1_14
package_deseq_2_8_2
package_r_ggplot2_0_9_3
package_r_edger_3_11_0

@martenson
Copy link
Member Author

Awesome, thank you guys!

mvdbeek added a commit to mvdbeek/galaxy that referenced this issue Feb 20, 2021
this is still not ideal, as the entire response will be loaded into
memory. This is a problem with streaming responses.
encode/starlette#1012 (comment)
is actually enlightening here:

> "This means this class will either load the entirety of streaming requests into memory (this issue) and run the background before returning the response (galaxyproject#919 ), or if we fix those problems, that it will then encourage users to leave resources in a pending or open state, an arguably worse result. In short, it's problematic."

Which is exactly what we'd be doing with a middleware, keeping resources
open for longer than necessary, which we need to avoid if we ever want
to run background / async tasks.  I think the solution here what we
already had, path operation dependencies.
mvdbeek added a commit to mvdbeek/galaxy that referenced this issue Feb 20, 2021
this is still not ideal, as the entire response will be loaded into
memory. This is a problem with streaming responses.
encode/starlette#1012 (comment)
is actually enlightening here:

> "This means this class will either load the entirety of streaming requests into memory (this issue) and run the background before returning the response (galaxyproject#919 ), or if we fix those problems, that it will then encourage users to leave resources in a pending or open state, an arguably worse result. In short, it's problematic."

Which is exactly what we'd be doing with a middleware, keeping resources
open for longer than necessary, which we need to avoid if we ever want
to run background / async tasks.  I think the solution here what we
already had, path operation dependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants