Download: Refactor CLI commands and introduce `--download-configs` as well as `--container-library`. #2336

MatthiasZepper · 2023-06-21T20:28:28Z

To address issue nf-core download interprets custom registry #2311, a new parameter --container-library was created allowing to specify the container library (registry) from which container images in OCI format (Docker) should be pulled.
In the course of this, all CLI parameters related to container images were refactored for consistency. Although downloading other images than those of the Singularity/Apptainer container system is not supported for the time being, a generic name for the parameters seemed preferable. So the new parameter --singularity-cache-index introduced in Download pipelines for Tower #2247 has been renamed to --container-cache-index prior to release. No previously released single letter parameters were reused as shorthand notation for a different parameter. Therefore, I refrained from using -c for --configuration, because it had been used before for --container.
Tabular representation of CLI parameter changes in this PR:

Old parameter	New parameter
new parameter	`-d` / `--download-configuration`
`-c`/ `--container <VALUE>`	`-s` / `--container-system <VALUE>`
new parameter	`-l` / `--container-library <VALUE>`
`--singularity-cache`	`-u` / `--container-cache-utilisation <VALUE>`
`-i` / `--singularity-cache-index <VALUE>`	`-i` / `--container-cache-index <VALUE>`

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

codecov · 2023-06-21T20:55:05Z

Codecov Report

Merging #2336 (9041d93) into dev (4f32c07) will decrease coverage by 0.39%.
The diff coverage is 32.11%.

@@            Coverage Diff             @@
##              dev    #2336      +/-   ##
==========================================
- Coverage   73.08%   72.69%   -0.39%     
==========================================
  Files          78       78              
  Lines        8779     8864      +85     
==========================================
+ Hits         6416     6444      +28     
- Misses       2363     2420      +57

Impacted Files	Coverage Δ
nf_core/download.py	`58.23% <30.23%> (-3.46%)`	⬇️
nf_core/__main__.py	`57.23% <62.50%> (+0.06%)`	⬆️

... and 1 file with indirect coverage changes

…gularity_images()

adamrtalbot

The only thing I really don't like is container-library. It's not a library, it's a registry. I think using library is confusing - I personally wouldn't consider that meant the default registry. Perhaps we could use -l but call it container-registry everywhere? Not consistent but at least it's obvious what it does.

adamrtalbot · 2023-06-26T08:42:21Z

README.md

 3. If they start with `http` they are downloaded directly within Python (default 4 at a time, you can customise this with `--parallel-downloads`)
-4. If they look like a Docker image name, they are fetched using a `singularity pull` command
+4. If they look like a Docker image name, they are fetched using a `singularity pull` command. Choose the container libraries (registries) queried by providing one or multiple `--container-library` parameter(s). For example, if you call `nf-core download` with `-l quay.io -l ghcr.io -l docker.io`, every image will be pulled from `quay.io` unless an error is encountered. Subsequently, `ghcr.io` and then `docker.io` will be queried for any image that has failed before.


That's a cool idea. You can also try to read this from /etc/containers/registries.conf if you really wanted.

I am open to improvements, but I felt that reading various config files (e.g. also the various Nextflow configs) was overengineering considering how desperately the next tools release is anticipated? If I recall correctly, it was you who rightfully pointed out via personal message that I was not really focussing enough on the essentials of that feature?

Yup I agree that's not important! I was just adding a comment because I thought you might be interested.

I am interested for sure, but ironically I have neither a /etc/containers/registries.conf nor a $HOME/.config/containers/registries.conf on my system. I presume that this is GNU/Linux specific?

adamrtalbot · 2023-06-26T08:50:31Z

nf_core/download.py

+                                if not self.container_library:
+                                    log.error(e.message)
+                                    log.error(e.helpmessage)
+                                    sys.exit(1)


Suggested change

sys.exit(1)

raise e

You should never system.exit(1) unless you absolutely have to, you will lose all traceback, error handling etc. If running this in a different system you may cause it to break without explanation.

Fair point. However, mind that the download command in the 2.8 version of tools is littered with those sys.exit(1) commands. So I have just adapted every new and refactored function to the existing code style and implementation patterns of download.py.

I agree that we should find a better mechanism then, but I think it is beyond the scope of the PR to change it here. For this line specifically, I think, that just raising the error will not suffice, as it is technically a non-recoverable critical error that should stop the execution. Do you have a suggestion (link to a blog post or doc entry) how to exit more gracefully and cleaner?

Yes, use raise e. That will cause the program to drop out with exit status 1, while showing a nice traceback and providing all the appropriate logs. If you had imported the module you could now do this if you wanted to ignore the error:

except RegistryNotFound: continue

Or more importantly:

except RegistryNotFound as err: # delete giant temporary files created during process # so we don't fill up storage with old cruft. raise err

Think of it like this, if you were running a website which was running this code, you've just caused it to die. No logs, no error, no traceback. Just dead.

Using sys.exit is generally bad in everything except in the __main__ block and even then, it should only be used in extreme circumstances. The most common reason to use system.exit(1) is if your software will do something Very Bad before it finishes.

Thanks for the explanation and suggestion(s).

I think we should indeed change that. I have now created the DownloadError as an exception that will be raised when the program needs to terminate, but the user should see no traceback.

adamrtalbot · 2023-06-26T08:56:38Z

nf_core/download.py

+                # Docker.io: denied: requested access to the resource is denied
+                #                    unauthorized: authentication required
+                # Quay.io: StatusCode: 404,  <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final//EN">\n']
+                # ghcr.io: Requesting bearer token: invalid status code from registry 400 (Bad Request)


These should all return the same 404 error in response to the request, I guess it's because this is in a subprocess that you don't get a standardised response?

No, the actual problem is that all the error handling is just done by "parsing" (actually just running through regexes) whatever singularity prints to stdout/stderr in response to trying singularity pull with the container path. This was acceptable, because in the previous version, scanning for the keyword "FATAL" and then killing the command was all that was needed.

Now, for the first time, it was required to distinguish different errors based on what was returned. I was/am not really happy with that regex solution, because I anticipate that this will be very flaky (e.g. because the wording changes with different versions of Singularity/Apptainer or the error messages are printed in a different language due to localization) but also had no better idea how to address this.

Indeed, avoiding singularity pull entirely and testing the path directly from Python now seems like an excellent idea to me. However, I fear that the Python request library just handles http? I think, this might be a different protocol and thus the 404 code returned by Quay.io is not standard?

Darn, that's frustrating. I'm not certain how singularity works but certainly docker pull is standardised so you can use the normal requests. Oh well, I agree this might break but it looks pretty good to me now.

adamrtalbot · 2023-06-26T08:59:24Z

nf_core/download.py

+    class RegistryNotFound(ConnectionRefusedError):
+        """The specified registry does not resolve to a valid IP address"""
+
+        def __init__(self, error_log):
+            self.error_log = error_log
+            self.message = (
+                f'[bold red]The specified container library "{self.error_log.registry}" is invalid or unreachable.[/]\n'
+            )
+            self.helpmessage = (
+                f'Please check, if you made a typo when providing "-l / --library {self.error_log.registry}"\n'
+            )
+            super().__init__(self.message, self.helpmessage, self.error_log)
+
+    class ImageNotFound(FileNotFoundError):
+        """The image can not be found in the registry"""
+
+        def __init__(self, error_log):
+            self.error_log = error_log
+            self.message = (
+                f'[bold red]"Pulling "{self.error_log.container}" from "{self.error_log.address}" failed.[/]\n'
+            )
+            self.helpmessage = f'Saving image of "{self.error_log.container}" failed.\nPlease troubleshoot the command \n"{" ".join(self.error_log.singularity_command)}" manually.f\n'
+            super().__init__(self.message)
+
+    class InvalidTag(AttributeError):
+        """Image and registry are valid, but the (version) tag is not"""
+
+        def __init__(self, error_log):
+            self.error_log = error_log
+            self.message = f'[bold red]"{self.error_log.address.split(":")[-1]}" is not a valid tag of "{self.error_log.container}"[/]\n'
+            self.helpmessage = f'Please chose a different library than {self.error_log.registry}\nor try to locate the "{self.error_log.address.split(":")[-1]}" version of "{self.error_log.container}" manually.\nPlease troubleshoot the command \n"{" ".join(self.error_log.singularity_command)}" manually.\n'
+            super().__init__(self.message)
+
+    class ImageExists(FileExistsError):
+        """Image already exists in cache/output directory."""
+
+        def __init__(self, error_log):
+            self.error_log = error_log
+            self.message = (
+                f'[bold red]"{self.error_log.container}" already exists at destination and cannot be pulled[/]\n'
+            )
+            self.helpmessage = f'Saving image of "{self.error_log.container}" failed, because "{self.error_log.out_path}" exists.\nPlease troubleshoot the command \n"{" ".join(self.error_log.singularity_command)}" manually.\n'
+            super().__init__(self.message)
+
+    class OtherError(RuntimeError):
+        """Undefined error with the container"""
+
+        def __init__(self, error_log):
+            self.error_log = error_log
+            self.message = f'[bold red]"{self.error_log.container}" failed for unclear reasons.[/]\n'
+            self.helpmessage = f'Pulling of "{self.error_log.container}" failed.\nPlease troubleshoot the command \n"{" ".join(self.error_log.singularity_command)}" manually.\n'
+            super().__init__(self.message, self.helpmessage, self.error_log)


This definitely feels like it's trying to reinvent the wheel. I'd guess this is because it's using a subprocess to call singularity instead of using a requests library to handle the HTTP error codes but if Singularity cant support this I guess that will have to be the solution. If we write a Docker implementation it can be a lot simpler.

I could not agree more. But that was the best idea I had, given the time pressure @drpatelh put on me. I think we can definitively avoid much of that mess by taking a step back and somehow avoiding singularity pull in the first place.

If we write a Docker implementation, it can be a lot simpler. Do you fancy doing this? Or have a suggestion how to test for valid docker images/tags without actually running singularity pull?

Yep we already have that in modules lint. I want to add docker to this and I have an open ticket for it, will start looking at it once this is merged.

How about using the Biocontainers API? This to me looks even better than relying purely on Docker.

nf_core/download.py

MatthiasZepper · 2023-06-26T10:20:08Z

The only thing I really don't like is container-library. It's not a library, it's a registry. I think using library is confusing - I personally wouldn't consider that meant the default registry. Perhaps we could use -l but call it container-registry everywhere? Not consistent but at least it's obvious what it does.

Well, I pondered a while over this. While registry also feels the most familiar to me, several terms like catalog, hub or registry are used, and library is a perfectly valid one of those:

adamrtalbot · 2023-06-26T10:48:03Z

The only thing I really don't like is container-library. It's not a library, it's a registry. I think using library is confusing - I personally wouldn't consider that meant the default registry. Perhaps we could use -l but call it container-registry everywhere? Not consistent but at least it's obvious what it does.

Well, I pondered a while over this. While registry also feels the most familiar to me, several terms like catalog, hub or registry are used, and library is a perfectly valid one of those:

"Docker Hub is the world's largest library and community for container images".

"The Sylabs Cloud Library is the place to push your containers to the cloud, so other users can pull, verify, and use them."

Hmm OK. In my head, registry = the hosting service and library = a collection of containers. It's not a big thing as long as the docs make it clear so maybe we just stick with container-library for now.

MatthiasZepper · 2023-06-26T12:21:14Z

Hmm OK. In my head, registry = the hosting service and library = a collection of containers. It's not a big thing as long as the docs make it clear so maybe we just stick with container-library for now.

Admittedly, I would have preferred registry, too - also for consistency with other tool subcommands. But -r is really the only letter in the alphabet we can't use, because it is already used for the revisions - also in nextflow.

We can't use -c either, because that would mean reusing it for a different parameter in two subsequent versions of tools, which may lead to confusion. Since it is a parameter that may well be specified multiple times, I'd also hate not giving it a single-letter short form, either.

…ect the UX negatively.

adamrtalbot

I've put 2 comments on but neither are of critical importance so I'll leave it up to you whether you want to add them or not. Nice work, this is looking like one of the best written parts of nf-core/tools!

nf_core/download.py

… be used to supress printing the traceback.

nf_core/__main__.py

…not specified. This was not handled atm. Also print the AssertionError error messages in DownloadError.

ewels changed the title ~~Issue 2311~~ Download: Fix container download when using custom registry Jun 22, 2023

MatthiasZepper force-pushed the Issue_2311 branch from 9047c94 to 2a091aa Compare June 22, 2023 12:34

MatthiasZepper changed the title ~~Download: Fix container download when using custom registry~~ Download: Refactor CLI commands and introduce --download-configs as well as --container-library. Jun 22, 2023

MatthiasZepper added 9 commits June 22, 2023 15:37

Refactor CLI commands of nf-core download.

a542152

Enable pulling from multiple container image libraries (registries).

ee39770

Adapt the tests.

f06772a

Fix the CLI test with regard to the revised CLI.

9485e22

Create a ContainerError class with several specific sub-errors.

130ce36

More extensive tests for the Singularity ContainerErrors.

cdbe6d1

Handle the Singularity exceptions in get_singularity_images().

2845e5c

Run issort, README and CHANGELOG updates.

82df716

Add a test for the wrapper around singularity_pull_image() in get_sin…

6e836c3

…gularity_images()

MatthiasZepper force-pushed the Issue_2311 branch from 2a091aa to 6e836c3 Compare June 22, 2023 14:18

Fix the test for get_singularity_images()

2e678e6

MatthiasZepper requested a review from adamrtalbot June 22, 2023 15:36

MatthiasZepper self-assigned this Jun 22, 2023

MatthiasZepper marked this pull request as ready for review June 22, 2023 16:09

adamrtalbot reviewed Jun 26, 2023

View reviewed changes

Replaced all 'sys.exit(1)' from the code. Hopefully this does not aff…

9fe1236

…ect the UX negatively.

adamrtalbot approved these changes Jun 26, 2023

View reviewed changes

nf_core/download.py Outdated Show resolved Hide resolved

nf_core/download.py Outdated Show resolved Hide resolved

MatthiasZepper added 2 commits June 26, 2023 17:57

Create a custom exception for intended program terminations. This can…

ee4a09a

… be used to supress printing the traceback.

Custom excepthook broke rich.traceback coloring. Fixed now.

2c93bd8

adamrtalbot reviewed Jun 27, 2023

View reviewed changes

nf_core/__main__.py Outdated Show resolved Hide resolved

Some Click versions may pass container_library as empty tuple when …

9041d93

…not specified. This was not handled atm. Also print the AssertionError error messages in DownloadError.

MatthiasZepper merged commit e22a4f7 into nf-core:dev Jun 27, 2023

MatthiasZepper deleted the Issue_2311 branch June 27, 2023 17:52

MatthiasZepper mentioned this pull request Jun 27, 2023

Add Apptainer (Singularity) to Gitpod nf-core base image #2345

Merged

4 tasks

mirpedrol mentioned this pull request Jun 28, 2023

nf-core download interprets custom registry #2311

Closed

adamrtalbot mentioned this pull request Jun 28, 2023

update modules in GATK's gcnvcaller pipeline nf-core/modules#3561

Merged

14 tasks

MatthiasZepper mentioned this pull request Aug 7, 2023

Ensure compatibility of nf-core download with Tower #1899

Closed

MatthiasZepper mentioned this pull request Feb 14, 2024

nf-core download has wrong name for singularity converted containers #2751

Closed

Aratz mentioned this pull request Apr 3, 2024

Update sequencing report service NationalGenomicsInfrastructure/miarka-provision#230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download: Refactor CLI commands and introduce `--download-configs` as well as `--container-library`. #2336

Download: Refactor CLI commands and introduce `--download-configs` as well as `--container-library`. #2336

MatthiasZepper commented Jun 21, 2023 •

edited

Loading

codecov bot commented Jun 21, 2023 •

edited

Loading

adamrtalbot left a comment

adamrtalbot Jun 26, 2023

MatthiasZepper Jun 26, 2023

adamrtalbot Jun 26, 2023

MatthiasZepper Jun 27, 2023

adamrtalbot Jun 26, 2023 •

edited

Loading

MatthiasZepper Jun 26, 2023 •

edited

Loading

adamrtalbot Jun 26, 2023

MatthiasZepper Jun 27, 2023

adamrtalbot Jun 26, 2023

MatthiasZepper Jun 26, 2023 •

edited

Loading

adamrtalbot Jun 26, 2023

adamrtalbot Jun 26, 2023

MatthiasZepper Jun 26, 2023

adamrtalbot Jun 26, 2023

MatthiasZepper Jun 27, 2023

MatthiasZepper commented Jun 26, 2023

adamrtalbot commented Jun 26, 2023

MatthiasZepper commented Jun 26, 2023

adamrtalbot left a comment

Download: Refactor CLI commands and introduce --download-configs as well as --container-library. #2336

Download: Refactor CLI commands and introduce --download-configs as well as --container-library. #2336

Conversation

MatthiasZepper commented Jun 21, 2023 • edited Loading

PR checklist

codecov bot commented Jun 21, 2023 • edited Loading

Codecov Report

adamrtalbot left a 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

adamrtalbot Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

MatthiasZepper Jun 26, 2023 • 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

MatthiasZepper Jun 26, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasZepper commented Jun 26, 2023

adamrtalbot commented Jun 26, 2023

MatthiasZepper commented Jun 26, 2023

adamrtalbot left a comment

Choose a reason for hiding this comment

Download: Refactor CLI commands and introduce `--download-configs` as well as `--container-library`. #2336

Download: Refactor CLI commands and introduce `--download-configs` as well as `--container-library`. #2336

MatthiasZepper commented Jun 21, 2023 •

edited

Loading

codecov bot commented Jun 21, 2023 •

edited

Loading

adamrtalbot Jun 26, 2023 •

edited

Loading

MatthiasZepper Jun 26, 2023 •

edited

Loading

MatthiasZepper Jun 26, 2023 •

edited

Loading