-
Notifications
You must be signed in to change notification settings - Fork 192
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
Conversation
4f3b8de
to
fddffad
Compare
Will still add tests for the new functionality, but you may already have a go on reviewing the code itself. |
0b29d07
to
f11a0e5
Compare
f11a0e5
to
98d7448
Compare
Codecov Report
@@ 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
|
8eb187e
to
a7f8d69
Compare
🎉 All tests are now working. Seems that this might be part of version 2.9 ☢️ Plutonium Penguin 🐧 (or whatever it will be called). Yeah! |
49376f0
to
98959ef
Compare
98959ef
to
7e49fc7
Compare
Testing it locally. I don't think the following prompt is needed when running it with |
nf_core/download.py
Outdated
ToDo: "sh", "bash", "dash", "ash","csh", "tcsh", "ksh", "zsh", "fish", "cmd", "powershell", "pwsh"? | ||
""" | ||
|
||
if os.environ["SHELL"] == "/bin/bash": |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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? |
ab696d0
to
c7cede3
Compare
…link from singularity to apptainer on the system.
Co-authored-by: Matthias Hörtenhuber <[email protected]>
… to Version 2.9dev.
…n showing it separately.
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.
c7cede3
to
315b9a3
Compare
…revisions may also be branches. Therefore, I rewrote this function to account for revisions that are not releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR proposes the addition of a
--tower
flag tonf-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:
str
todict
/list
).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.--singularity-cache
innf-core download
from a flag to an argument. The prior options were renamed toamend
(container images are only saved in the$NXF_SINGULARITY_CACHEDIR
) andcopy
(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
CHANGELOG.md
is updateddocs
is updated