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

Download pipelines for Tower #2247

Merged
merged 43 commits into from
Jun 1, 2023

Conversation

MatthiasZepper
Copy link
Member

@MatthiasZepper MatthiasZepper commented Apr 24, 2023

This PR proposes the addition of a --tower flag to nf-core download to simplify the addition of downloaded pipelines to Seqeralabs Tower SaaS application and addresses #1899.

Instead of downloading a compressed archive of the pipelines source code, a clone of the pipeline's GitHub repository is created in the local cache, modified and subsequently bare-cloned to the output directory to be used with Tower. As before, the tool can detect the required Singularity images and download those as well. Since configuration profiles are likely managed separately with the help of Tower, downloading the configs is omitted.

Enabling those changes required some alterations to the download command:

  • The new functionality also uses a cached GitHub repository, as first introduced by @ErikDanielsson for the modules. Thus, most functionality was moved to a generic superclass for both repo types to inherit from.
  • Both downloads (classic or Tower) now support downloading multiple revisions with the same invocation (variables and functions were changed from str to dict / list).
  • The find_container_images() function has been improved to support the detection of container images declared as variables. Download e.g. the RNA-seq pipeline 3.7 for an exemplary case that was not handled correctly by the current version.
  • Refactored the CLI for --singularity-cache in nf-core download from a flag to an argument. The prior options were renamed to amend (container images are only saved in the $NXF_SINGULARITY_CACHEDIR) and copy (a copy of the image is saved with the download). remote was newly introduced and allows to provide a table of contents of a remote cache via an additional argument --singularity-cache-index (#2247).

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@MatthiasZepper MatthiasZepper added WIP Work in progress enhancement labels Apr 24, 2023
@MatthiasZepper MatthiasZepper force-pushed the DownloadForTower branch 3 times, most recently from 4f3b8de to fddffad Compare April 26, 2023 17:48
nf_core/__main__.py Outdated Show resolved Hide resolved
nf_core/__main__.py Outdated Show resolved Hide resolved
nf_core/download.py Outdated Show resolved Hide resolved
nf_core/download.py Outdated Show resolved Hide resolved
nf_core/download.py Show resolved Hide resolved
nf_core/download.py Show resolved Hide resolved
nf_core/download.py Outdated Show resolved Hide resolved
nf_core/download.py Show resolved Hide resolved
nf_core/download.py Show resolved Hide resolved
nf_core/download.py Outdated Show resolved Hide resolved
@MatthiasZepper MatthiasZepper marked this pull request as ready for review April 27, 2023 11:02
@MatthiasZepper
Copy link
Member Author

Will still add tests for the new functionality, but you may already have a go on reviewing the code itself.

@MatthiasZepper MatthiasZepper added the awaiting-changes Needs changes after a review. Remove label when ready to be reviewed again. label Apr 27, 2023
@MatthiasZepper MatthiasZepper self-assigned this Apr 27, 2023
@MatthiasZepper MatthiasZepper removed WIP Work in progress awaiting-changes Needs changes after a review. Remove label when ready to be reviewed again. labels Apr 27, 2023
@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #2247 (259afa6) into dev (ef62904) will increase coverage by 0.03%.
The diff coverage is 67.20%.

@@            Coverage Diff             @@
##              dev    #2247      +/-   ##
==========================================
+ Coverage   72.93%   72.96%   +0.03%     
==========================================
  Files          77       78       +1     
  Lines        8446     8730     +284     
==========================================
+ Hits         6160     6370     +210     
- Misses       2286     2360      +74     
Impacted Files Coverage Δ
nf_core/modules/lint/main_nf.py 68.19% <0.00%> (ø)
nf_core/utils.py 82.06% <14.28%> (-0.38%) ⬇️
nf_core/download.py 61.91% <60.61%> (+9.19%) ⬆️
nf_core/synced_repo.py 81.11% <81.11%> (ø)
nf_core/__main__.py 57.09% <100.00%> (+0.14%) ⬆️
nf_core/modules/modules_repo.py 75.36% <100.00%> (-8.29%) ⬇️

@MatthiasZepper
Copy link
Member Author

🎉 All tests are now working. Seems that this might be part of version 2.9 ☢️ Plutonium Penguin 🐧 (or whatever it will be called). Yeah!

README.md Outdated Show resolved Hide resolved
nf_core/download.py Outdated Show resolved Hide resolved
nf_core/download.py Outdated Show resolved Hide resolved
nf_core/download.py Outdated Show resolved Hide resolved
@mashehu
Copy link
Contributor

mashehu commented May 8, 2023

Testing it locally. I don't think the following prompt is needed when running it with --tower, because you would always want them with singularity containers, no?
In addition to the pipeline code, this tool can download software containers. ? Download software container images:

ToDo: "sh", "bash", "dash", "ash","csh", "tcsh", "ksh", "zsh", "fish", "cmd", "powershell", "pwsh"?
"""

if os.environ["SHELL"] == "/bin/bash":
Copy link
Member

Choose a reason for hiding this comment

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

I was testing this on Gitpod, and it failed because there isn't a variable $SHELL. We should probably check first if the variable exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I switched it to os.getenv("SHELL", "") now, which has a default value if the variable is not set.

@MatthiasZepper
Copy link
Member Author

Testing it locally. I don't think the following prompt is needed when running it with --tower, because you would always want them with singularity containers, no? In addition to the pipeline code, this tool can download software containers. ? Download software container images:

I am pretty indifferent there. Tower does not require Singularity containers and are bare-clone can also be used to launch a pipeline directly with Nextflow without Tower, so it might be that people do not want to download them?

@MatthiasZepper MatthiasZepper force-pushed the DownloadForTower branch 2 times, most recently from ab696d0 to c7cede3 Compare May 9, 2023 15:03
MatthiasZepper and others added 19 commits May 25, 2023 14:27
…link from singularity to apptainer on the system.
Co-authored-by: Matthias Hörtenhuber <[email protected]>
Co-authored-by: Matthias Hörtenhuber <[email protected]>
…onal for Tower downloads. Given that there is the option to provide the list of remote containers to skip their download, I agree that this is reasonable.
MatthiasZepper and others added 2 commits May 26, 2023 23:52
…revisions may also be branches. Therefore, I rewrote this function to account for revisions that are not releases.
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants