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

Cytoscape #66

Merged
merged 49 commits into from
Sep 22, 2023
Merged

Cytoscape #66

merged 49 commits into from
Sep 22, 2023

Conversation

ajshedivy
Copy link
Collaborator

@ajshedivy ajshedivy commented Jun 27, 2022

  • add py4cytoscape container
  • add new cytoscape visualization rule

Let's focus on creating a cytoscape rule that can grab all of the generated pathways. After that I can continue work on creating the cytoscape session using docker. Right now, local support of cytoscape is integrated, make sure to start cytoscape, then run the workflow.

Links

Snakefile Outdated Show resolved Hide resolved
@agitter
Copy link
Collaborator

agitter commented Jun 28, 2022

Thanks for working on this. Cytoscape visualization will be a great addition. I'll work on input for the Snakefile rule to gather all of the pathway files.

The design here can be streamlined compared to what we set up for TPS because Cytoscape will always run inside the container. All we need on the SPRAS side is an interface to pass the list of files (and any annotations) to the container. We should also try to avoid conda inside the container and install the required packages with pip. That will give us a chance of creating a container that works with Singularity.

Several files in this pull request will be deleted later so we should squash and merge when it's time.

@agitter
Copy link
Collaborator

agitter commented Jun 29, 2022

We need to decide which pathways should be visualized in the Cytoscape session. Should it be the pathways that correspond to the parameters in the current config file (current behavior). Or all pathways discovered in the output directory, including some that may be left over from previous runs with parameters that were removed from the config file?

Copy link
Collaborator

@agitter agitter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm able to run the workflow now locally with Docker and generate the Cytoscape session file. It's very cool to have it working. I did get what looks like several error messages from the cyrest client that I haven't looked at closely.

All of the Docker-related files can be moved to docker-wrappers/Cytoscape from src/analysis/py4cy. Before we merge, please add a readme describing this image. We'll also want to switch to the reedcompbio DockerHub organization.

Can we delete the two empty __init__.py files?

PRRunner.py Outdated Show resolved Hide resolved
config/config_cyto.yaml Outdated Show resolved Hide resolved
config/egfr.yaml Outdated Show resolved Hide resolved
src/analysis/cytoscape/cytoscape.py Outdated Show resolved Hide resolved
src/analysis/cytoscape/cytoscape.py Outdated Show resolved Hide resolved
src/analysis/cytoscape/py4cy/cytoscape_util.py Outdated Show resolved Hide resolved
src/analysis/cytoscape/py4cy/cytoscape_util.py Outdated Show resolved Hide resolved
src/analysis/cytoscape/py4cy/Dockerfile Outdated Show resolved Hide resolved
src/parse_config.py Outdated Show resolved Hide resolved
src/util.py Outdated Show resolved Hide resolved
@agitter
Copy link
Collaborator

agitter commented Jul 9, 2022

The workflow runs to completion when the Cytoscape step is skipped, which is good because earlier it looked like it might be failing at one of the PathLinker runs. I now suspect the issue is #22 and that the output .cys file is owned by root. Switching to the run_container framework will confirm that.

agitter and others added 6 commits August 12, 2022 09:17
@agitter
Copy link
Collaborator

agitter commented Aug 25, 2022

I pushed some updates to these Cytoscape files today. Some were minor code cleanup and reorganization changes. I converted the container wrapper to use the run_container utility function as a step toward Singularity testing. However, the volume mapping needed for this breaks the pathway names that are shown in the Cytoscape session. Instead of names like output/data0-pathlinker-params-VQL7BDZ/pathway.txt now we have names like /spras/NLZ6AAF/pathway.txt. That may mean the original filename needs to be passed to cytoscape_util.py as another command line argument. We're already calling rename_network so we can pass it a different name.

I was able to simplify the Docker call somewhat, which makes me more optimistic about Singularity support. The CMD isn't needed in the Dockerfile, nor is the port in the Docker run call.

There are a few other open TODOs:

  • Add a readme with an explanation and attribution
  • Test Singularity
  • Fix the filename issue described above
  • Move the Docker image to the reedcompbio organization
  • Add the Docker image to the tests
  • Add test cases

@agitter
Copy link
Collaborator

agitter commented Aug 25, 2022

I pushed reedcompbio/py4cytoscape to DockerHub so we can test and maintain that version.

Update: the tests now pass with this version of the Docker image and the run_container wrapper.

@ajshedivy
Copy link
Collaborator Author

I was able to get the workflow to run with the new docker image and the changes to the cytoscape backend components.

@agitter
Copy link
Collaborator

agitter commented Jun 10, 2023

I'm now manually creating the Cytoscape vmoptions file and adding it to the image. I ran the vmoptions generation script inside an interactive container session and modified the memory settings and set the home directory to point to the writeable /spras working directory. These Cytoscape documents catalog relevant installation and configuration directories that are used during startup, several of which need to be writeable.

This post states that the Unable to update instance pid is not serious and can be ignored. However, I'm still getting the errors that follow, which are fatal. No space left on device could have something to do with how Singularity manages disk or temporary files inside the container. I'm still using Singularity's --writable-tmpfs argument for my interactive testing.

@agitter
Copy link
Collaborator

agitter commented Jun 29, 2023

I posted to the Cytoscape Google group about the Singularity problems I'm having and received a response from one of the developers: https://groups.google.com/g/cytoscape-helpdesk/c/NNC1Bj8fcPA/m/4HOlPmvtAQAJ

That gives me some hope for continuing to debug this. I'll do more interactive Singularity testing based on the feedback.

@agitter
Copy link
Collaborator

agitter commented Sep 2, 2023

Through interactive testing in Docker and Singularity and support from a developer through the Google group, I figured out the Singularity "No space left on device" error. I also believe I have a workaround.

The error was due to Cytoscape not using the -Duser.home setting in its vmoptions file for all of the configuration files it downloads. Some configuration bundles are downloaded to a configuration directory that is hard-coded as a subdirectory of $HOME. The containers run as root, so $HOME=/root which is okay in Docker and not in Singularity.

I was eventually able to get Singularity to recognize a different home directory. Now Cytoscape runs interactively with Singularity in my converted Docker image. I also fixed a Cytoscape bug, which I believe is new, described here using their recommended workaround.

Despite that, the workflow still hung at the Cytoscape rule in Singularity.

There are still some final changes to make after the workflow runs:

  • Consider the latest py4cytoscape package that supports configurable log directories, although it may be more work than expected to use those now that we need to set the home directory with an environment variable. Our container framework only supports a single environment variable per call for now.
  • Clean up output files and log files
  • Write one Cytoscape session file per dataset

@agitter
Copy link
Collaborator

agitter commented Sep 8, 2023

This works in Singularity now! I also refactored it to produce one Cytoscape session per dataset instead of combining all pathways into a single dataset. I write the temporary Cytoscape logs to a dataset-specific output directory and then remove it after Cytoscape finishes, so there shouldn't be any problems with parallelism.

I'd like to look over all the code once more and then will ask someone else to test that it works before merging.

@agitter
Copy link
Collaborator

agitter commented Sep 15, 2023

I added test cases to run Cytoscape in Docker and Singularity. If those pass, I'll tag the image as v1 and this should be ready to merge.

@ntalluri could you also please try testing the workflow on your machine in Docker to confirm you get session files and no errors before I merge?

@agitter
Copy link
Collaborator

agitter commented Sep 21, 2023

The test_cytoscape_singularity test that fails in GitHub actions passes in the Apptainer environment I have access to locally:

$ pytest -k test_cytoscape_singularity
====================================================================================== test session starts =======================================================================================
platform linux -- Python 3.8.16, pytest-7.1.3, pluggy-1.0.0
rootdir: /ua/gitter/spras
collected 67 items / 66 deselected / 1 selected

test/analysis/test_cytoscape.py .                                                                                                                                                          [100%]

======================================================================================== warnings summary ========================================================================================
../miniconda3/envs/spras/lib/python3.8/site-packages/future/standard_library/__init__.py:65
  /ua/gitter/miniconda3/envs/spras/lib/python3.8/site-packages/future/standard_library/__init__.py:65: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
==================================================================== 1 passed, 66 deselected, 1 warning in 162.83s (0:02:42) =====================================================================

My test server has Apptainer 1.2.2 whereas GitHub Actions is using 1.1.3.

$ apptainer --version
apptainer version 1.2.2-1.el7

The 1.2.0 release introduced some important behavioral changes so I'll update what we run in GitHub Actions to see if that is causing the test failure.

@agitter
Copy link
Collaborator

agitter commented Sep 22, 2023

All tests pass (after marking the Cytoscape Singularity test xfail) and I tagged the image as v1.

@ajshedivy I'm merging in the SPRAS Cytoscape pull request!

@agitter agitter merged commit e67167f into master Sep 22, 2023
10 checks passed
@agitter agitter deleted the cytoscape branch September 22, 2023 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants