Moved all singularity-related download functions into a new file #3509

muffato · 2025-03-25T11:35:13Z

Closes #3019
Supersedes #3163

Ongoing work part of the "Workflows to go" hackathon project. I've started moving all singularity functions into a new file.

Overview of the moves from nf_core/pipelines/download.py

Object or function	New file	New name
DownloadError	downloads/utils	(same name)
DownloadProgress	downloads/utils	(same name)
ContainerError	downloads/singularity	(same name)
symlink_singularity_images	downloads/singularity	symlink_registries
singularity_image_filenames	downloads/singularity	get_container_filename
singularity_copy_cache_image	downloads/singularity	SingularityFetcher.copy_image
singularity_download_image	downloads/utils	FileDownloader.download_file
singularity_pull_image	downloads/singularity	SingularityFetcher.pull_image
get_singularity_images (part of)	downloads/singularity	SingularityFetcher.fetch_containers, SingularityFetcher.pull_images, SingularityFetcher.download_images
get_singularity_images (part of)	downloads/utils	FileDownloader.download_files_in_parallel

In this move, I wanted to have clean copy/download/pull_image functions that know nothing about a "cache" or a "library". They're just given a path they need to put their file into.
I was also able to decouple things and move code from methods to functions when access to class variables etc wasn't really needed. Or in the case of the file downloads, I've introduced a dedicated class where only the necessary state is tracked.

So far, we've got this functionality in downloads.utils:

intermediate_file is a context manager for getting download/copy operations done in a temporary file that is deleted if there is a problem. This is to avoid polluting the cache/output directories with partial files (the question was raised in Container cache download #3163 (comment)) and I've rolled it out across all three source methods.
DownloadProgress is the extension of rich.progress. I've added helper methods to deal with the "main task" and "sub tasks"
FileDownloader can download files in parallel. I've reviewed the handling of errors and exceptions to make sure that no state if left behind.

and in downloads.singularity:

get_container_filename returns the name a container will have on disk, taking into consideration a set of registry prefixes to ignore.
symlink_registries creates symlinks for a container so that the same image file can be used across registries.

That's all completely covered in unit tests.

At this minute, I'm not 100% settled on this SingularityFetcher class. I want to look into decoupling it a bit more.

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

That looks already very promising. Thank you very much! I need to take another look tomorrow...

MatthiasZepper · 2025-03-25T20:51:32Z

nf_core/pipelines/download.py

+                else:
+                    raise FileNotFoundError("Singularity cache is required but no '$NXF_SINGULARITY_CACHEDIR' set!")
+
+            assert self.outdir


Sorry, but I dislike the assert here.

self.outdir should be set, as it is already handled by the __init__ function of DownloadWorkflow. But if it was None, you are producing an AssertionError that is not handled?

The assert is just there to please pylance/mypy, or whatever tool I've got running in my IDE.

We do it like this in other places in the code, with the comment, which makes the purpose more clear

Suggested change

assert self.outdir

assert self.outdir is not None # mypy

My IDE was saying that outdir was str | None | Unknown. I hope further refactoring of the class will lead to this function being in a scope where it's clear that outdir is defined and a string.

MatthiasZepper · 2025-03-25T20:55:16Z

nf_core/pipelines/download.py

+
+            # Check that the directories exist
+            if not os.path.isdir(out_path_dir):
+                log.debug(f"Output directory not found, creating: {out_path_dir}")


Minor remark, but Output directory is generic, so it is unclear which exactly was missing. If you print in the log.debug() that the Singularity images directory was missing, it is probably more meaningful.

nf_core/pipelines/downloads/singularity.py

MatthiasZepper · 2025-03-25T21:22:21Z

nf_core/pipelines/downloads/singularity.py

+    """
+
+    def __init__(
+        self, container_library: Iterable[str], registry_set: Iterable[str], progress: rich.progress.Progress


Ideally, the __init__() function should have a brief summary of the arguments, maybe something along these lines.

container_library (List[str]): The container libraries (registries) to pull containers from. Specified manually by the user. registry_set (Set[str]): A set of container libraries for creating symlinks. Consists of all container_libraries, registries defined in the config of a workflow and a core set of hardcoded registries occurring in various modules.

Might also be an opportunity to replace the generic name registry_set with something outlining its purpose? (Blaming myself here actually for picking that in the first place...)

I want to work more on this class and will definitely review the naming of the parameters.

That said, encouraged by your comment, I've moved a couple of methods to functions and renamed their registry_set parameter to registries (I realised that _set was probably an indicator it was a set object ?)

MatthiasZepper · 2025-03-25T21:28:40Z

nf_core/pipelines/downloads/singularity.py

+        self.kill_with_fire = False
+
+    def get_container_filename(self, container: str) -> str:
+        """Check Singularity cache for image, copy to destination folder if found.


The help text still needs to be updated. But I like your approach to break it up to smaller, logically separate functions!

I've updated the text. Hope that's clearer

tests/pipelines/test_download.py

MatthiasZepper · 2025-03-25T21:44:55Z

nf_core/pipelines/downloads/utils.py

+    """
+
+
+@contextlib.contextmanager


I do not understand this part of the code, but it looks fancy! Great that you found a way to deal with interrupted downloads and prevent persistent faulty images.

I've added some comments and made the code more explicit. I hope it's clearer

nf_core/pipelines/downloads/utils.py

nf_core/pipelines/downloads/singularity.py

MatthiasZepper · 2025-03-25T22:12:53Z

nf_core/pipelines/downloads/singularity.py

+                    # download into the cache
+                    fetch_list.append((container, cache_path))
+                    # and copy from the cache to the output
+                    containers_copy.append((container, cache_path, output_path))


Why a copy to output_path? But it is already late, so it is probably me...

This resonates with a comment you made in the previous PR. I need to double-check all those copy operations. Make sure that the "amend" / "copy" are respected, and avoid copying files if not needed

…environment variables

…have to be aware of the TaskID

codecov · 2025-03-26T12:23:08Z

Codecov Report

Attention: Patch coverage is 63.17204% with 137 lines in your changes missing coverage. Please review.

Project coverage is 77.45%. Comparing base (33830f9) to head (ea4bc89).
Report is 25 commits behind head on dev.

Files with missing lines	Patch %	Lines
nf_core/pipelines/downloads/singularity.py	44.11%	133 Missing ⚠️
nf_core/pipelines/download.py	90.90%	2 Missing ⚠️
nf_core/pipelines/downloads/utils.py	98.21%	2 Missing ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…ogress

MatthiasZepper

Thanks! This is looking really promising already!

MatthiasZepper · 2025-03-26T15:07:43Z

nf_core/pipelines/downloads/singularity.py

-            self.progress.update(task, description="Downloading singularity images")
-            self.download_images(containers_download, task, parallel_downloads=4)
+            self.progress.update_main_task(description="Downloading singularity images")
+            self.download_images(containers_download, parallel_downloads=4)


parallel_downloads is a CLI argument, currently stored in self.parallel_downloads of a DownloadWorkflow instance. I have not heard of anyone using it ever (except for debugging), but it is probably reasonable to keep it customizable?

If not, we should remove the CLI argument.

I didn't know there was a CLI argument ! I'll connect them back

tests/pipelines/test_download.py

…uarantee the task gets removed

- Replaced the generator with a callback + a list return - Fixed the handling of exceptions so that KeyboardInterrupt is always captured - More tets

mirpedrol · 2025-03-28T11:37:37Z

nf_core/pipelines/download.py

-        Attempt to use a local installation of singularity to pull the image.
+            # Find out what the library directory is
+            library_dir = os.environ.get("NXF_SINGULARITY_LIBRARYDIR")
+            if library_dir and not os.path.isdir(library_dir):


We are trying to use the library pathlib for files instead of os. Please try to use pathlib everywhere else, thanks!

Suggested change

if library_dir and not os.path.isdir(library_dir):

if library_dir and not Path(library_dir).is_dir():

mirpedrol · 2025-03-28T11:39:28Z

nf_core/pipelines/download.py

+            cache_dir = os.environ.get("NXF_SINGULARITY_CACHEDIR")
+            if self.container_cache_utilisation in ["amend", "copy"]:
+                if cache_dir:
+                    if not os.path.isdir(cache_dir):


Suggested change

if not os.path.isdir(cache_dir):

if not Path(cache_dir).is_dir():

mirpedrol · 2025-03-28T11:49:13Z

nf_core/pipelines/download.py

+                else:
+                    raise FileNotFoundError("Singularity cache is required but no '$NXF_SINGULARITY_CACHEDIR' set!")
+
+            assert self.outdir


We do it like this in other places in the code, with the comment, which makes the purpose more clear

Suggested change

assert self.outdir

assert self.outdir is not None # mypy

mirpedrol · 2025-03-28T11:55:08Z

nf_core/pipelines/downloads/singularity.py

+        for container, output_path in containers_pull:
+            # it is possible to try multiple registries / mirrors if multiple were specified.
+            # Iteration happens over a copy of self.container_library[:], as I want to be able to remove failing registries for subsequent images.
+            for library in self.container_library[:]:


Makes it more readable

Suggested change

for library in self.container_library[:]:

for library in self.container_library.copy():

mirpedrol · 2025-03-28T12:09:46Z

nf_core/pipelines/downloads/singularity.py

+                log.debug(f"Singularity command: {' '.join(singularity_command)}")
+
+                # Run the singularity pull command
+                with subprocess.Popen(


We have the function run_cmd() in nf_core/utils.py, I think it would be useful to reduce the code here

If you have time, it would be great to change indeed. But since it is our current codebase, I do not want to be too demanding either.

There is a big difference with nf_core.utils.run_cmd. Here we stream the output to update the progress bar. The motivation is that singularity pulls can take a while to run, and it's better for users to get a visual feedback that something is really happening.
nf_core.utils.run_cmd runs the command and only returns the output/error when the command ends. I'm not sure that's the right thing to do here.

I would rather propose to introduce nf_core.utils.stream_cmd

That sounds convincing to me, although I have admittedly not looked into the specific implementation of nf_core.utils.run_cmd.

mirpedrol · 2025-03-28T12:10:48Z

nf_core/pipelines/downloads/singularity.py

+            if not (shutil.which("singularity") or shutil.which("apptainer")):
+                raise OSError("Singularity/Apptainer is needed to pull images, but it is not installed or not in $PATH")


I think this is not needed as we already have the same check in pull_images()

In all fairness, this check is simply our current codebase.

Coincidentally, I want to get rid of those checks entirely, as they prevent running the tool on MacOS with recent pipelines that use Seqera Containers or at least disable that check with -f/--force. Instead, the tool should just output a list of commands that it could not run because singularity / apptainer was missing.

Can you explain your use case a bit more, @MatthiasZepper ? If someone runs nf-core pipelines download with --container-system singularity, it seems fair to complain if singularity/apptainer is not available.

Sure. I had the case in mind that you are running nf-core download on a different computer that you will use for executing the workflows later. Specifically, I was thinking of MacOS, which does not support singularity/apptainer.

We have a HPC that runs Linux, but are downloading the pipelines with our developers' Macbooks. If we need to pull a Docker container and convert it or have a mandatory oras:// download, I need to start my VM, but most nf-core pipelines actually have http:// downloads for all required modules, so it is possible to obtain a perfectly functional download without singularity/apptainer. Due to this check, nf-core download will nonetheless not run, which is why I have symlinked ~/bin/apptainer -> /usr/bin/true on my MacOS.

For all modules that use Seqera Containers, it is no longer possible to remove the Docker container, if there is a http:// download available. So for Seqera Container modules, you will always download both: The native Singularity container and convert the Docker image. This makes it even more attractive to dummy replace apptainer while downloading to save some bandwith and try out, which containers are actually missing.

Oki. In that case, I would propose adding a command-line option to allow missing containers as a result of a failure of the copy/download/pull command or because the command can't be run (i.e. your case). It would essentially turn all the errors into warnings. In your case, it'll still do all the downloads but skip the pulls and report the list of missing containers.

Sounds good to me, although applying --force instead of a dedicated CLI argument would probably suffice as well. In either way out of the scope of this PR, which is, I believe, already comprehensive enough and shouldn't get bogged down by too many small additions.

mirpedrol · 2025-03-28T12:13:52Z

nf_core/pipelines/downloads/__init__.py

As I mentioned in #3506 (comment), the directory name should be download

This was my decision to avoid import errors for now. Running git mv is later is pretty quick, I guess.

mirpedrol · 2025-03-28T12:20:39Z

nf_core/pipelines/downloads/utils.py

+                successful_downloads.append(future_downloads[future])
+
+        with concurrent.futures.ThreadPoolExecutor(max_workers=parallel_downloads) as pool:
+            # The entire block needs to be monitored for KeyboardInterrupt so that ntermediate files


Suggested change

# The entire block needs to be monitored for KeyboardInterrupt so that ntermediate files

# The entire block needs to be monitored for KeyboardInterrupt so that intermediate files

muffato requested review from MatthiasZepper and JulianFlesch March 25, 2025 11:35

muffato self-assigned this Mar 25, 2025

muffato added the download label Mar 25, 2025

muffato force-pushed the singularity_downloads branch from 5961e7b to 952bc92 Compare March 25, 2025 12:23

muffato added 3 commits March 25, 2025 14:54

Moved all singularity-related download functions into a new file

ae0d4dd

Added a helper method to use a temporary file as intermediate output

bbd297f

First round of fixing the tests

Loading
Loading status checks…

a9b2b00

muffato force-pushed the singularity_downloads branch from 952bc92 to a9b2b00 Compare March 25, 2025 17:09

MatthiasZepper reviewed Mar 25, 2025

View reviewed changes

muffato added 9 commits March 26, 2025 10:56

downloads.singularity functions should rely on proper parameters, no …

dc333b3

…environment variables

Duplicated with the line above

fbc09ac

Added tests for utils.intermediate_file and fixed it !

fee7182

Simpler implementation

c507d9e

Added helper functions to manage the main task so that callers don't …

aba5233

…have to be aware of the TaskID

No need to build a temp path here

0403a37

More tests

159c34e

Fixed the symlink_registries tests

Loading
Loading status checks…

693da64

Rewrote to support older versions of Python

Loading
Loading status checks…

c5379fb

Added a dest for the main_task-management methods added to DownloadPr…

8ee8eed

…ogress

MatthiasZepper reviewed Mar 26, 2025

View reviewed changes

Added some tests for the DownloadProgress object

Loading
Loading status checks…

71ab538

nikhil mentioned this pull request Mar 26, 2025

Add docker support in pipeline download #3516

Closed

4 tasks

muffato added 3 commits March 26, 2025 17:52

Turned two methods into functions, simplified testing

a141820

Test comments

4133c90

Theoretically it could be None, so check it really is a string

2b8fbd6

nikhil mentioned this pull request Mar 26, 2025

Add docker downloads #3517

Draft

4 tasks

muffato added 2 commits March 27, 2025 09:28

More type hints

7813f30

More tests for get_container_filename

cb71d42

muffato added 12 commits March 27, 2025 12:02

No need to keep a wraper as a method

2d8cb4d

Better comments

4bfb45e

New class to support the downloads

d6b8f6c

Introduced a context manager that creates a new sub task. Useful to g…

4abd736

…uarantee the task gets removed

Forgot that pytest.raises exists !

31f5627

Moved the download class to utils

06d3760

The output path cannot be a directory or a symbolic link

3857fae

Basic test for FileDownloader.download_file

31fe914

Empty files are not allowed

02cc2ca

Test log.debug too

b648bc1

Test the kill_with_fire flag

467ba3f

Refactored the FileDownloader

Loading
Loading status checks…

ea4bc89

- Replaced the generator with a callback + a list return - Fixed the handling of exceptions so that KeyboardInterrupt is always captured - More tets

mirpedrol reviewed Mar 28, 2025

View reviewed changes

	if library_dir and not os.path.isdir(library_dir):
	if library_dir and not Path(library_dir).is_dir():

	if not os.path.isdir(cache_dir):
	if not Path(cache_dir).is_dir():

	for library in self.container_library[:]:
	for library in self.container_library.copy():

		if not (shutil.which("singularity") or shutil.which("apptainer")):
		raise OSError("Singularity/Apptainer is needed to pull images, but it is not installed or not in $PATH")

	# The entire block needs to be monitored for KeyboardInterrupt so that ntermediate files
	# The entire block needs to be monitored for KeyboardInterrupt so that intermediate files

		"""


		@contextlib.contextmanager

Moved all singularity-related download functions into a new file #3509

Are you sure you want to change the base?

Moved all singularity-related download functions into a new file #3509

Conversation

muffato commented Mar 25, 2025 • edited Loading

PR checklist

MatthiasZepper 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

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

codecov bot commented Mar 26, 2025 • edited Loading

Codecov Report

MatthiasZepper 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

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

muffato commented Mar 25, 2025 •

edited

Loading

codecov bot commented Mar 26, 2025 •

edited

Loading