search

Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

transomics2cytoscape #1513

Closed kozo2 closed 4 years ago

kozo2 commented 4 years ago

Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor

Confirm the following by editing each check box to '[x]'

I am familiar with the essential aspects of Bioconductor software management, including:

For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.

bioc-issue-bot commented 4 years ago

Hi @kozo2

Thanks for submitting your package. We are taking a quick look at it and you will hear back from us soon.

The DESCRIPTION file for this package is:

Package: transomics2cytoscape
Title: Create 3D transomics network view with Cytoscape and Cy3D
Version: 0.99.0
Authors@R: c(
    person ("Kozo", "Nishida", email = "kozo.nishida@gmail.com",
        role = c("aut", "cre"), comment = c(ORCID = "0000-0001-8501-7319"))
    )
Description: transomics2cytoscape import multiple KEGG pathways and 
    combine those to one 3D space and visualize it with Cy3D Cytoscape app.
License: Artistic-2.0
Imports: RCy3, KEGGREST, dplyr
Suggests: testthat, roxygen2, knitr, BiocStyle, rmarkdown
Encoding: UTF-8
LazyData: true
biocViews: Network, Software, Pathways, DataImport, KEGG
VignetteBuilder: knitr
RoxygenNote: 7.1.0

Add SSH keys to your GitHub account. SSH keys will are used to control access to accepted Bioconductor packages. See these instructions to add SSH keys to your GitHub account.

bioc-issue-bot commented 4 years ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your repository will NOT trigger a new build.

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

kozo2 commented 4 years ago

Hi @mtmorgan . This package needs to install Cytoscape (and Java11) in the build environment. Is it possible for me to do that? If I can't do that, can I use the --no-check-vignettes option for this BiocCheck ?

mtmorgan commented 4 years ago

Hi @kozo2

In principle, we could manually install Cytoscape if it were added to the SystemRequirements: field of the DESCRIPTION file, and if there were a file INSTALL that provided detailed instructions on how to install it on each of our platforms (Linux, Mac, Windows). The 'Headless' version is appropriate for the builders.

But how do other packages like RCy3 that seem like they would have Cytoscape dependencies manage? Also, just reaching out to @paul-shannon in case he remembers past experiences...

paul-shannon commented 4 years ago

@kozo2 and @mtmorgan,

Dan Tenebaum managed what I think he called "a cytoscape farm" for several years to support RCytoscape then RCy3 testing. I vaguely recall that as the overall bioc build system evolved, the burden of the farm rose, and we all agreed to give it up. @dtenenba will remember the details.

kozo2 commented 4 years ago

Thanks @mtmorgan I added the SystemRequirements: field And I added a file INSTALL that provided detailed instructions on how to install it on each of our platforms (Linux, Mac, Windows).

@mtmorgan @paul-shannon As far as I know, the 'Headless' version is no longer in development. We need at least two terminals (on each of your platforms) to use Cytoscape as a REST server. (And RCy3 is a client package for the REST server.) One is for the Cytoscape process and the other for the R process. One terminal has to run Cytoscape process and keep the process running. (This cannot be done in the background.)

mtmorgan commented 4 years ago

I'm concerned that this will be a very fragile situation. It sounds similar to what Dan had, and if I recall it required regular maintenance as well as familiarity with the details of Cytoscape across platforms; it also ran into connectivity and other problems associated with long-running processes on multi-tenant systems.

In the end and for this particular case I think it is better to provide a vignette with code chunks that are built on your system but not the build system

knitr::opts_chunk$set(eval = identical(Sys.getenv("KOZOS_SYSTEM"), "TRUE"))

I think it would be very valuable to explore the use of testthat::with_mock() to write unit tests that do not rely on the server, but allow assessment of the correctness of your code.

paul-shannon commented 4 years ago

Fragile - I agree.

One other possibility (maybe practical, maybe not) is for the Cytoscape Consortium to run a Cytoscape instance with the CyREST interface at a known host:port - just for bioc build testing. RCy3 is the client, talking to this remote, always available server, which I imagine would be running headless but otherwise fully functional, and completely separate from Bioconductor.

n- Paul

On Jun 15, 2020, at 3:16 AM, Martin Morgan notifications@github.com wrote:

I'm concerned that this will be a very fragile situation. It sounds similar to what Dan had, and it required regular maintenance as well as familiarity with the details of Cytoscape across platforms; it also ran into connectivity and other problems associated with long-running processes on multi-tenant systems.

In the end and for this particular case I think it is better to provide a vignette with code chunks that are built on your system but not the build system

knitr::opts_chunk$set(eval = identical(Sys.getenv("KOZOS_SYSTEM"), "TRUE"))

I think it would be very valuable to explore the use of testthat::with_mock() to write unit tests that do not rely on the server, but allow assessment of the correctness of your code.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

kozo2 commented 4 years ago

@mtmorgan I provided a code chunk not to be built on the Bioconductor system. https://github.com/ecell/transomics2cytoscape/blob/master/vignettes/transomics2cytoscape.Rmd#L14-L18

I didn't see why testthat::with_mock() could be used for writing unit tests that do not rely on the CyREST server. Anyway I added some unit tests to https://github.com/ecell/transomics2cytoscape/tree/master/tests/testthat (But these unit tests rely on the CyREST server.)

kozo2 commented 4 years ago

@paul-shannon I'll send your message to Cytoscape team (when I get a chance). But It might be difficult to realize. (One of the reasons is that CyREST does not support remote file IO.)

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

d292ef5 Bump version

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

ee13d37 Disable tests as the Bioconductor build system doe...

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

cd4110e Disable test in man Rd file

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

11c7597 Add newline at end of test-*.R

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

0712755 Add explanation to Desctiption field

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

kozo2 commented 4 years ago

@mtmorgan

These ERROR and NOTE cannot be resolved because Cytoscape is required to run the man Rd file. What should I do?

* Checking man page documentation...
    * ERROR: At least 80% of man pages documenting exported objects
      must have runnable examples. The following pages do not:
      create3Dcyjs.Rd
    * NOTE: Usage of dontrun{} / donttest{} found in man page examples.
      100% of man pages use one of these cases.
      Found in the following files:
        create3Dcyjs.Rd
mtmorgan commented 4 years ago

ignore the error; I'll review the package

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

5e34cd2 Add 2 sentences to Description

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

3ab43fa Add functionality to control the Cy3D renderer fro...

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

c8747c2 Bump version

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

ef54358 Bump version

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

72c5af7 Change input file and output specifications (#9) ...

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/transomics2cytoscape to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

kozo2 commented 4 years ago

@mtmorgan I'm sorry. I changed the specifications of the package since you replied me. Would it be possible for you to review the package again?

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

957583c Update the Version in DESCRIPTION

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/transomics2cytoscape to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

mtmorgan commented 4 years ago

If the package is stable and you perform a version bump, I will review it.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

6ea297c Refactor and stabilize code (#11) * Update tests ...

kozo2 commented 4 years ago

@mtmorgan I stopped updating. Now the package is stable (and I performed a version bump). I would be grateful if you could review it.

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/transomics2cytoscape to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

mtmorgan commented 4 years ago

3 August, 2020

Please address these very brief notes and include a comment indicating your response.

DESCRIPTION, NAMESPACE

vignettes

R

tests

kozo2 commented 4 years ago

@mtmorgan Thank you for your review.

I updated the code as much as I can. https://github.com/ecell/transomics2cytoscape/compare/comment-3Aug?expand=1 If it's OK with this update, I would like to merge comment-3Aug to master.

please update installation instructions to indicate BiocManager::install("transomics2cytoscape").

I replaced devtools::install_github to BiocManager::install

please do not write to the current working director "." at line 123; use tempdir() or dir.create(tempfile()) instead (also in the man page).

I couldn't find a way to follow this advise.

I think the sif path created by tempdir is not fixed. On the other hand, in the networkLayers.tsv (an argument of create3Dnetwork function), it is necessary to write the path of the sif. (In the example data case, it is galFiltered.sif in the current directory).

layer1  rno04910    600
layer2  galFiltered.sif 400
layer3  rno00010    200
layer4  rno00010    1

I think we have no way to know the path created by tempdir before writing this tsv. If we can use the current directory, we don't need to write the full path.

What are your thoughts on that?

transomics2cytoscape.R: 77 message(paste(...)) is usually not necessary, since message() calls paste0() internally. Just message("... ", "...")

I removed all the paste()s in the message()s.

the unit tests are commented out; it is better to remove 'dead' code.

I moved the test code from tests to inst/unitTests (and removed the comment marks in them). I imitated the way of RCy3 https://github.com/cytoscape/RCy3/blob/master/inst/unitTests/test_RCy3.R

mtmorgan commented 4 years ago

tempfile() exists for the duration of the R session, and would be an appropriate value for a vignette or example; of course the user could write "." or any other path? Something like

diff --git a/R/transomics2cytoscape.R b/R/transomics2cytoscape.R
index 22e1eea..ceb719a 100644
--- a/R/transomics2cytoscape.R
+++ b/R/transomics2cytoscape.R
@@ -12,23 +12,28 @@
 ##' representing transomic interaction edges between the network layers.
 ##' @param stylexml A XML file path for Cytoscape style file to be applied to
 ##' the 3D network.
+##' @param workdir Path to the directory used to create output.
 ##' @return A SUID of the 3D network. 
 ##' @author Kozo Nishida
 ##' @import dplyr
 ##' @export
 ##' @examples \donttest{
+##' workdir <- tempfile(); dir.create(workdir)
 ##' sif <- system.file("extdata","galFiltered.sif",package="RCy3")
-##' file.copy(sif, ".")
+##' file.copy(sif, workdir)
 ##' networkLayers <- system.file("extdata", "networkLayers.tsv",
 ##'     package = "transomics2cytoscape")
 ##' transomicEdges <- system.file("extdata", "transomicEdges.tsv",
 ##'     package = "transomics2cytoscape")
 ##' stylexml <- system.file("extdata", "transomics.xml",
 ##'     package = "transomics2cytoscape")
-##' create3Dnetwork(networkLayers, transomicEdges, stylexml)
+##' create3Dnetwork(networkLayers, transomicEdges, stylexml, workdir)
 ##' }

-create3Dnetwork <- function(networkLayers, transomicEdges, stylexml) {
+create3Dnetwork <- function(networkLayers, transomicEdges, stylexml, workdir) {
+    owd <- setwd(workdir)
+    on.exit(setwd(owd))
+
     tryCatch({
         RCy3::cytoscapePing()
     }, error = function(e) {
kozo2 commented 4 years ago

@mtmorgan Thank you for your advice.

I replaced "." to tempdir() with your idea.

https://github.com/ecell/transomics2cytoscape/compare/comment-3Aug?expand=1 If it's OK with this update, I would like to merge comment-3Aug to master.

mtmorgan commented 4 years ago

sounds good!

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

fee927e Follow mtmorgan's advise 3 August (#12) * Use Bio...

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/transomics2cytoscape to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

95b8d3b Update README (#13) * Update README.md

kozo2 commented 4 years ago

Sorry, I forgot to update README. Please forgive me for updating twice.