nf-core download <pipeline name> #53

ewels · 2018-05-30T23:24:31Z

New subcommand to download a pipeline and its singularity containers.

Validates pipeline input, fetches workflow details and release hash
Download workflow files
If -sis specified, downloads singularity images
- Automatically finds container names from workflow config
- Looks on the singularity hub API for a singularity image - downloads it if found
  - Includes md5 check
- If not found, tries to pull singularity images using singularity
- ~~If singularity is not installed, tries using docker (with docker2singularity)~~
  - removed due to configuration difficulties - may be possible to put back later with more investigation

To do:

Check that it works for other people
Write tests!

Example usage:

$ nf-core download methylseq -s

                                          ,--./,-.
          ___     __   __   __   ___     /,-._.--~\
    |\ | |__  __ /  ` /  \ |__) |__         }  {
    | \| |       \__, \__/ |  \ |___     \`-._,-`-,
                                          `._,._,'


INFO: Saving to 'nf-core-methylseq-1.0'

INFO: Downloading workflow files from GitHub

INFO: Fetching container names for workflow

INFO: Building singularity image 'nfcore/methylseq:1.0', saving to /Users/ewels/GitHub/nf-core/testing/nf-core-methylseq-1.0/singularity-images/nf-core-methylseq-1.0.simg

WARNING: Singularity is not installed. Attempting to use Docker instead.
Size: 4720 MB for the singularity container
(1/9) Creating an empty image...
Creating a sparse image with a maximum size of 4720MiB...
Using given image size of 4720
Formatting image (/sbin/mkfs.ext3)
Done. Image can be found at: /tmp/nfcore_methylseq_1.0-2018-04-17-d18eb7142a18.img
(2/9) Importing filesystem...
(3/9) Bootstrapping...
(4/9) Adding run script...
(5/9) Setting ENV variables...
Singularity: sexec (U=0,P=143)> Command=exec, Container=/tmp/nfcore_methylseq_1.0-2018-04-17-d18eb7142a18.img, CWD=/, Arg1=/bin/sh
(6/9) Adding mount points...
Singularity: sexec (U=0,P=149)> Command=exec, Container=/tmp/nfcore_methylseq_1.0-2018-04-17-d18eb7142a18.img, CWD=/, Arg1=/bin/sh
(7/9) Fixing permissions...
Singularity: sexec (U=0,P=155)> Command=exec, Container=/tmp/nfcore_methylseq_1.0-2018-04-17-d18eb7142a18.img, CWD=/, Arg1=/bin/sh
Singularity: sexec (U=0,P=196)> Command=exec, Container=/tmp/nfcore_methylseq_1.0-2018-04-17-d18eb7142a18.img, CWD=/, Arg1=/bin/sh
Debian GNU/Linux 9 \n \l
We're not running on BusyBox/Buildroot
Singularity: sexec (U=0,P=202)> Command=exec, Container=/tmp/nfcore_methylseq_1.0-2018-04-17-d18eb7142a18.img, CWD=/, Arg1=/bin/sh
(8/9) Stopping and removing the container...
d18eb7142a18
d18eb7142a18
(9/9) Moving the image to the output folder...
  2,381,807,616  48%   10.97MB/s    0:03:48

Example output:

$ tree -L 3
.
└── nf-core-methylseq-1.0
    ├── singularity-images
    │   └── nf-core-methylseq-1.0.simg
    └── workflow
        ├── CHANGELOG.md
        ├── Dockerfile
        ├── LICENCE.md
        ├── README.md
        ├── assets
        ├── bin
        ├── conf
        ├── docs
        ├── environment.yml
        ├── main.nf
        ├── nextflow.config
        └── tests

9 directories, 7 files

codecov-io · 2018-05-30T23:28:18Z

Codecov Report

Merging #53 into master will decrease coverage by 22.16%.
The diff coverage is 14.28%.

@@             Coverage Diff             @@
##           master      #53       +/-   ##
===========================================
- Coverage    94.8%   72.63%   -22.17%     
===========================================
  Files           4        6        +2     
  Lines         500      665      +165     
===========================================
+ Hits          474      483        +9     
- Misses         26      182      +156

Impacted Files	Coverage Δ
nf_core/download.py	`0% <0%> (ø)`
nf_core/utils.py	`100% <100%> (ø)`
nf_core/lint.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 360e41a...0278018. Read the comment docs.

* Validates pipeline input, fetches workflow details and release hash * Download workflow files * If -s specified, automatically finds container names from workflow config * Tries to pull singularity images using first singularity, then docker

maxulysse · 2018-05-31T06:29:30Z

scripts/nf-core

+    help = "Pipeline release"
+)
+@click.option(
+    '-s/-n', '--singularity/--no-singularity',


Wouldn't it be better to have something like -s yes / -s no instead?
I find it disturbing to have two options for two opposites things.

I guess - the above is the standard way to handle boolean flags with click (see docs), so this is the way I've done it in the past. But I don't really mind either way.

OK, I didn't knew the standards.
I'm fine with it then ;-)

Note that I did it as a single flag first - just -s to get singularity images and nothing to not get them. But then I thought it was better to be explicit and require this input from the user.

Yeah, after playing a bit more I agree. As it's required the help message says that it's missing -s if not supplied, which is kind of counter-intuitive.

Just need to find a nice way to handle y|Y|yes|Yes|YES for both options now. I bet click has a nice tidy way to handle this for you, if I can just find it..

I too would prefer just one parameter -s or --singularity-images with yes/no as only accepted values, that can be done by using choices or giving a call_back.

Or just --singularity/--no-singularity (without -s/-n)

Yes - I half implemented the Choice thing this morning, but if we want to handle case insensitivity and short/long form then the list of allowed options gets messy... 🤓 (y|Y|yes|Yes|YES|n|N|no|No|NO).

We could write a custom callback or string handling behaviour, but that feels like overkill and the help text is not particularly pretty then. click already has this kind of handling for the --yes behaviour. click.BOOL can also handle string values, so I figure that click probably has built-in support for this somehow...

Yeah!! looks like setting type=click.BOOL does the trick right ? for y/n values

Option:

@click.option('-s', '--singularity', type=click.BOOL, default=None, required=True)

Test:

$ python cli_test.py test -s y True $ python cli_test.py test -s yes True $ python cli_test.py test -s YEs True $ python cli_test.py test -s YeS True $ python cli_test.py test -s nO False $ python cli_test.py test -s n False $ python cli_test.py test -s NO False $ python cli_test.py test -s NOo Usage: cli_test.py test [OPTIONS] Error: Invalid value for "-s" / "--singularity": noo is not a valid boolean $ python cli_test.py test -s 0 False $ python cli_test.py test -s 1 True $ python cli_test.py test -s 2 Usage: cli_test.py test [OPTIONS] Error: Invalid value for "-s" / "--singularity": 2 is not a valid boolean

senthil10 · 2018-05-31T07:29:31Z

scripts/nf-core

+    help = "Pipeline release"
+)
+@click.option(
+    '-s/-n', '--singularity/--no-singularity',


I too would prefer just one parameter -s or --singularity-images with yes/no as only accepted values, that can be done by using choices or giving a call_back.

Or just --singularity/--no-singularity (without -s/-n)

senthil10 · 2018-05-31T07:34:48Z

nf_core/download.py

+
+        # If we got this far, must not be a nf-core pipeline
+        if self.pipeline.count('/') == 1:
+            # Looks like a GitHub address - try working with this repo


Why not check this upstream ? i.e. validate the param value at very beginning. That could improve the success rate of downloading something even higher 😉

Also could be good to log that it is not a nf-core pipeline (?)

Yup, should do both 👍 Was rushing to push the code so that I wasn't late into work for Remi's talk 😉

This is as early as I can check I think. In order to validate the pipeline name, I have to fetch the pipeline names from the website API and see if the supplied name matches any of them (the above for wf in wfs.remote_workflows loop). So this point in the code is the first time that I know it's not an nf-core pipeline.

Agree that logging is a good idea 👍

Logging added in fd90e46

senthil10 · 2018-05-31T07:49:48Z

nf_core/download.py

+    # Check that the outdir doesn't already exist
+    if os.path.exists(wf.outdir):
+        logging.error("Output directory '{}' already exists".format(wf.outdir))
+        sys.exit(1)


Do you really wanna exit ? If so why not have this check at the beginning (like as soon as receiving the parameter or in the class) that will save us all the trouble of doing everything 😇

Or just warn and save in a custom name so they can rename the folder later ?

Yeah, should probably handle this in a nicer way 👍

Ok, I remember..

If so why not have this check at the beginning

I can't check any earlier, as if --outdir is not specified, the default output directory name is generated based on the workflow name and version number. The version number is the last release if not specified. So basically, I don't know what the output directory name will be before I get the workflow details.

Or just warn and save in a custom name so they can rename the folder later?

Nah - this command isn't going to be super common to run (like multiqc or nextflow or something), so it's not such a big deal to just bail and exit I think. Less code and potentially less confusing.

Ok, I see the point now 👍

senthil10 · 2018-05-31T08:00:23Z

nf_core/download.py

+
+import nf_core.list, nf_core.lint
+
+def download_workflow(pipeline, release=None, singularity=False, outdir=None):


Just curious, Why make it as independent function rather than not including it in the Class below ?

Can't remember, was copying my earlier code structure for the other modules.

* Moved the nextflow config parsing into new common utils.py module * Made singularity download option a single command line boolean flag * Improved logging for download script

ewels · 2018-05-31T11:25:46Z

Now waiting for singularityhub/docker2singularity#30 to be merged so that we can create newer better singularity images using docker ✨

senthil10 · 2018-05-31T11:34:22Z

nf_core/download.py

+                    logging.info("Pipeline is in development. Using current code on master branch.")
+
+                # Set outdir name if not defined
+                if self.outdir is None:


Maybe this block can be moved up to L40 (keeping all outdir related stuff together), so this function will be clean and does only what the name suggests ?

I see what you're getting at, but I respectfully disagree 😆 I'm just setting all of the different variables in this function that will be used later on, including the outdir and the download link.

Hehe, it is a matter of personnel choice/prefference, so no right or wrong here 😇It makes sense in this context to update necessary variables (including outdir) in this function as this function was called by download_workflow (which uses outdir later on).

I suggested this considering more general context i.e. when (or if) fetch_workflow_details is called by any other function or script that don't have or need outdir variable, then updating outdir would be "out of context" 😜(Not an error but its just something not necessary then). That is why I would have put this part up directly in download_workflow 😉(Of course, since the class itself is more "download" related this might not happen, but like I said it is matter of personnel preference)

senthil10 · 2018-05-31T11:37:29Z

nf_core/download.py

+                    self.download_singularity_image(container)
+
+
+    def get_workflow(self):


Trying to be picky 😉 -> is it really a "get" function ? I mean it returns only True/False. So maybe naming set_workflow or collect_workflow or something else be more appropriate

Alright, I'll give you this one ;)

…yhub/docker2singularity#30

sven1103

Minor stuff

sven1103 · 2018-05-31T16:30:04Z

nf_core/download.py

+            logging.warn("Pipeline name doesn't match any nf-core workflows")
+            logging.info("Pipeline name looks like a GitHub address - attempting to download anyway")
+            self.wf_name = self.pipeline
+            if self.release is None:


better if not self.release

sven1103 · 2018-05-31T16:30:23Z

nf_core/download.py

+            if self.release is None:
+                self.release = 'master'
+            self.wf_sha = self.release
+            if self.outdir is None:


better if not self.outdir

sven1103 · 2018-05-31T16:41:22Z

nf_core/download.py

+                    self.outdir += '-{}'.format(self.release)
+            # Set the download URL and return
+            self.wf_download_url = 'https://github.com/{}/archive/{}.zip'.format(self.pipeline, self.release)
+            return True


Not a fan of boolean return values in such functions, that are not a question, like is_finished or so. Better raise a meaningful expection like raise LookupError("Pipeline {} not found on whatever".format(pipeline))

Agreed! Yes, I felt a bit dirty writing this but it worked. Now you've suggested the raise it seems obvious 😆

sven1103 · 2018-05-31T16:41:46Z

nf_core/download.py

+            logging.error("Not able to find pipeline '{}'".format(self.pipeline))
+            logging.info("Available pipelines: {}".format(', '.join([w.name for w in wfs.remote_workflows])))
+            return False
+


raise Exception here

* if not x: instead of if x is None: * Raise and catch exceptions instead of returning booleans

ewels · 2018-06-05T06:47:38Z

Hacky in-progress WIP code pushed sorry. Trying to download directly from singularity hub - I feel like it should be easy (curl works fine), but the requests.get line hangs for some reason. Haven't figured out why yet.

sven1103 · 2018-06-05T07:22:59Z

With "hang" you mean timeout? Or just forever?

ewels · 2018-06-05T07:31:51Z

Just forever. Timeout param has no effect. A very fast minimal example in a live python environment seemed fine, so I’m not sure what’s going on. Ran out of time and had to run into work for a meeting though..

sven1103 · 2018-06-05T07:52:08Z

nf_core/download.py

+                with open(out_path, 'wb') as f:
+                    for chunk in dl_request.iter_content(chunk_size=1024):
+                        if chunk:
+                            f.write(chunk)


Please do f.flush() too :)

This was intentional, due to this stack overflow comment

ewels · 2018-06-05T11:57:10Z

Missing tests, but done for now unless anyone has any more suggestions I think!

sven1103 · 2018-06-06T17:04:58Z

-s, --singularity [y/n] Download singularity containers? [required]
I am not so sure about that. An option is an option, and never required for a CLI to work. Otherwise it is a positional parameter. Just make it as boolean flag with default value False. Done

sven1103 · 2018-06-06T17:07:25Z

Next question: if the --singularity option is n, it is not pulling the container from DockerHub. Why? :D

ewels · 2018-06-06T21:22:31Z

Just make it as boolean flag with default value False. Done

This is what I did at the start of the pull request, but then I changed it at the request of the reviewers 😅 (sort of, anyway. See discussion)

Also note that I found having it as a flag with default False was quite annoying as I kept forgetting to specify it. For this reason I preferred having it as a required option.

Next question: if the --singularity option is n, it is not pulling the container from DockerHub. Why? :D

That's what the flag is for :) Not everyone uses singularity, but anyone running offline will need to download the pipeline files. So these should always be downloaded, but the singularity image should be optional. One could think of other optional downloads in the future too - reference genome files for example.

Downloading from singularity hub / docker hub shouldn't really matter. shub is preferable as it doesn't require singularity to be installed. There is no method to affect this through the command line, and I'm not really sure that there needs to be.

Basically, this was the logic I used to get to this interface:

People shouldn't have to download singularity images if they don't need them
Explicit choice of this is easier and will probably help to avoid unnecessary confusion / annoyance
Positional Boolean options don't really make sense (I don't think I've ever seen them anyway). Personally I prefer a required flag.

Happy to change this if anyone has a better solution though!

sven1103 · 2018-06-07T11:38:12Z

This is what I did at the start of the pull request, but then I changed it at the request of the reviewers sweat_smile (sort of, anyway. See discussion)

I am sorry, I really missed that.

Also note that I found having it as a flag with default False was quite annoying as I kept forgetting to specify it. For this reason I preferred having it as a required option.

Don't want to be picky, but we should stick to common Unix-like CLI. A required option is not an option. If you want to download the singularity image, provide the --singularity flag, else don't. If one wants to see the options, type -h/--help as for every UNIX CLI.

--singularity Pulls a singularity image with the software tools for the pipeline.

With the rest I totally agree, and I like the feature very much :)

So, now beat me :P

apeltzer · 2018-06-07T12:59:22Z

This can be basically seen from two points of view: Do we want people to be able to just run the command and then everything they need is there or do we want them to use this tool for specific use cases and then require them to specify exactly what they want to have?

I'd rather say keep it optional, as the download might take quite a while and then people can choose if they want to do that on a per-user basis. I understand having the problem to forget it, but also downloading things that you don't want everytime is quite similarly annoying ;-)

ewels · 2018-06-07T13:09:41Z

Yup, I didn't really have a strong opinion on this and agree with all of the points made. I'll switch the current logic with a single -s/--singularity flag which enables the download (disabled by default). Thanks for the discussion all! 👍

senthil10 · 2018-06-07T13:15:57Z

Would it be good/helpful to be able to just download the container images for given pipeline ?
(Like if someone forgot to give -s the first time)

sven1103 · 2018-06-07T13:17:23Z

@senthil10 I think this is a useful feature! 👍

probably with a flag --get-container (or similar)?

ewels · 2018-06-07T15:02:09Z

I'm a little confused by this last request - in discussion I think it was about rerunning the command if we forget to specify -s, but without having to download the pipeline files a second time. I would prefer not to do this, as using local files makes the versioning less failsafe and downloading them a second time only takes a second.

We have to download the pipeline files to get the container, as the pipeline config is parsed to get the container name dynamically. So we could do a special mode which does this and then deletes those files (or just saves them to /tmp), but I feel that this is kind of overkill in this scenario to be honest..

sven1103 · 2018-06-07T18:28:18Z

@ewels Ah ok, I see. Ok, then let's not stress this pull request to much, there are still tests to write. @senthil10 We can keep this in our mind, if there is a future request for such a feature coming up, we can still brainstorm about and implement it.

@ewels: I am happy to write the tests if you like :)

senthil10 · 2018-06-07T19:23:36Z

@sven1103 yes, I agree too 👍 @ewels have already done a great job with this PR :)

As you mentioned we can add this in the future depending upon need (maybe something to debate during nf-core hackathon 😉 ).

In general I think it can be a good feature to have, though its not a crucial feature it would still give little more flexibility for advance users. For instance if a user downloads just the pipeline first, then tweaks the pipeline a bit for fun or internal necessity and later if they ran into a situation where they needed the containers this feature might come in handy. It can be either done by nf-core or by the pipeline itself (a script to download all related containers). @maxulysse have already implemented this in sarek with a buildContainers script.

ewels · 2018-06-07T19:44:16Z

@sven1103 - if you could write the tests that would of course be amazing 🎉 😉

@senthil10 - I still don't really think that this would be particularly useful, since our pipelines have only one container (Sarek has loads, which is why a script makes sense). So it really is just singularity pull <name> which for advanced users is not hard.

The point of this download script is to specifically to:

get both the pipeline script and containers in one command
do it safely for a tagged, stable release only, for reproducibility.

ewels · 2018-06-07T19:44:34Z

...and if someone could leave a positive review and merge, that'd be fab 😉

senthil10 · 2018-06-07T20:18:43Z

@ewels I did say "it is not crucial, it would just give more flexibility" and I said that thinking of packages with multiple containers, I dint know all the packages in nf-core have only one container 😅

senthil10

As usual, extremely good coding @ewels 👏 👏

P.S: I will leave the merging to @sven1103 or others 😇

sven1103 · 2018-06-08T08:53:41Z

@ewels I think you did a great implementation with this feature 🎉 ! I really like it! I will merge and write the tests over the weekend.

maxulysse · 2018-06-08T08:54:20Z

@senthil10 @ewels The buildContainers script is IMHO only useful when having to build several containers.
With it I can build and push several Docker containers at once thanks to Nextflow.
Same thing with pulling Singularity containers.

It was also a good opportunity to fun trying some Nextflow stuff.

ewels · 2018-06-08T11:48:54Z

Thanks for the discussion everyone!

ewels added enhancement WIP Work in progress labels May 30, 2018

ewels requested a review from a team May 30, 2018 23:24

WIP: New nf-core download command

705a9a4

* Validates pipeline input, fetches workflow details and release hash * Download workflow files * If -s specified, automatically finds container names from workflow config * Tries to pull singularity images using first singularity, then docker

ewels force-pushed the nf-core-download branch from aec21cb to 705a9a4 Compare May 30, 2018 23:37

Make command line singularity option explicit and required.

770a87e

maxulysse reviewed May 31, 2018

View reviewed changes

Now works with any GH repo. Better logging.

9ce2083

senthil10 reviewed May 31, 2018

View reviewed changes

download improvements, small lint refactor

fd90e46

* Moved the nextflow config parsing into new common utils.py module * Made singularity download option a single command line boolean flag * Improved logging for download script

ewels force-pushed the nf-core-download branch from 12f3ec4 to fd90e46 Compare May 31, 2018 10:27

Move main download function into class

b0e5625

senthil10 reviewed May 31, 2018

View reviewed changes

Tweaks for @senthil10 and update to docker2singularity for singularit…

d53214d

…yhub/docker2singularity#30

sven1103 reviewed May 31, 2018

View reviewed changes

sven1103 mentioned this pull request May 31, 2018

Make a release #54

Closed

Download: Changes for @sven1103

48fff1c

* if not x: instead of if x is None: * Raise and catch exceptions instead of returning booleans

ewels mentioned this pull request Jun 1, 2018

Update docker2singularity to Singularity v2.5.1 singularityhub/docker2singularity#30

Merged

ewels added 2 commits June 5, 2018 00:40

Remove docker2singularity command

36a8d48

Late night WIP: Download from shub directly.

be28256

sven1103 reviewed Jun 5, 2018

View reviewed changes

ewels added 2 commits June 5, 2018 13:53

Got singularity hub download to work

80ad087

Check downloaded md5 hash against remote. Refactor a little.

119450c

ewels force-pushed the nf-core-download branch from 746ee5f to 119450c Compare June 5, 2018 11:53

ewels removed the WIP Work in progress label Jun 5, 2018

ewels requested a review from a team June 6, 2018 21:25

Make --singularity a simple enabling flag.

0278018

senthil10 approved these changes Jun 7, 2018

View reviewed changes

sven1103 approved these changes Jun 8, 2018

View reviewed changes

sven1103 merged commit f2862ae into nf-core:master Jun 8, 2018

ewels deleted the nf-core-download branch June 8, 2018 11:48


		import nf_core.list, nf_core.lint

		def download_workflow(pipeline, release=None, singularity=False, outdir=None):

		self.download_singularity_image(container)


		def get_workflow(self):

nf-core download <pipeline name> #53

nf-core download <pipeline name> #53

Conversation

ewels commented May 30, 2018 • edited Loading

codecov-io commented May 30, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewels commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sven1103 left a comment

Choose a reason for hiding this comment

sven1103 May 31, 2018 • edited Loading

Choose a reason for hiding this comment

sven1103 May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewels commented Jun 5, 2018

sven1103 commented Jun 5, 2018

ewels commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewels commented Jun 5, 2018

sven1103 commented Jun 6, 2018

sven1103 commented Jun 6, 2018 • edited Loading

ewels commented Jun 6, 2018 • edited Loading

sven1103 commented Jun 7, 2018

apeltzer commented Jun 7, 2018

ewels commented Jun 7, 2018

senthil10 commented Jun 7, 2018

sven1103 commented Jun 7, 2018 • edited Loading

ewels commented Jun 7, 2018

sven1103 commented Jun 7, 2018

senthil10 commented Jun 7, 2018

ewels commented Jun 7, 2018

ewels commented Jun 7, 2018

senthil10 commented Jun 7, 2018

senthil10 left a comment

Choose a reason for hiding this comment

sven1103 commented Jun 8, 2018

maxulysse commented Jun 8, 2018

ewels commented Jun 8, 2018

ewels commented May 30, 2018 •

edited

Loading

codecov-io commented May 30, 2018 •

edited

Loading

sven1103 May 31, 2018 •

edited

Loading

sven1103 May 31, 2018 •

edited

Loading

sven1103 commented Jun 6, 2018 •

edited

Loading

ewels commented Jun 6, 2018 •

edited

Loading

sven1103 commented Jun 7, 2018 •

edited

Loading