114 singularity integration #118

SimoneBendazzoli93 · 2025-09-25T02:01:01Z

fixes #114

…ation algorithms

…nvironment variable configuration

…environment variable setup

brainless-bot · 2025-09-25T02:01:19Z

🤖 Code Formatting Reminder

Hello there! 👋 It looks like the code in this pull request might benefit from some formatting improvements.
Fix the issues locally or use our auto format action by commenting /format on this PR!

Copilot

Pull Request Overview

This PR adds Singularity container support as an alternative to Docker for running BraTS algorithms, addressing issue #114. The implementation allows users to switch between Docker and Singularity backends via an environment variable or method parameter.

Implements Singularity container runtime functionality alongside existing Docker support
Adds backend selection mechanism through environment variable or explicit parameter
Updates all algorithm inference methods to support the new backend parameter

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
brats/core/singularity.py	New module implementing Singularity container execution logic
brats/core/brats_algorithm.py	Updated to dispatch between Docker and Singularity backends
brats/constants.py	Added Backends enum for backend type definitions
brats/core/segmentation_algorithms.py	Added backend parameter support to inference methods
brats/core/missing_mri_algorithms.py	Added backend parameter support to inference methods
brats/core/inpainting_algorithms.py	Added backend parameter support to inference methods
pyproject.toml	Added spython dependency for Singularity support
tests/core/test_singularity.py	Comprehensive test suite for Singularity functionality
README.md	Updated documentation with Singularity installation instructions

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

brats/core/singularity.py

tests/core/test_singularity.py

neuronflow · 2025-09-25T02:02:20Z

/format

brainless-bot · 2025-09-25T02:02:30Z

🤖 I will now format your code with black. Check the status here.

Co-authored-by: Copilot <[email protected]>

…unction

neuronflow

README.md also needs adjustments :)

please have a look at co-pilot comments and ensure tests are passing :)

neuronflow · 2025-09-25T02:09:31Z

please mark as ready for review once @MarcelRosier and me should review :)

…_mlcube

…singularity.py

…ityHelpers

…orithms.py

Copilot

Pull Request Overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-25T08:22:07Z

brats/core/singularity.py

+    force_cpu: bool,
+    internal_external_name_map: Optional[Dict[str, str]] = None,
+):
+    """Run a docker container for the provided algorithm.


The docstring incorrectly states 'Run a docker container' when this function actually runs a Singularity container. This should be updated to reflect the correct container technology.

Suggested change

"""Run a docker container for the provided algorithm.

"""Run a Singularity container for the provided algorithm.

Copilot · 2025-09-25T08:22:07Z

brats/core/singularity.py

+    image = _ensure_image(image=algorithm.run_args.docker_image)
+
+    additional_files_path = _get_additional_files_path(algorithm)
+    print(f"Additional files path: {additional_files_path}")


This debug print statement should be replaced with a proper logger call for consistency with the rest of the codebase. Use logger.debug(f'Additional files path: {additional_files_path}') instead.

Suggested change

print(f"Additional files path: {additional_files_path}")

logger.debug(f"Additional files path: {additional_files_path}")

Copilot · 2025-09-25T08:22:07Z

brats/core/singularity.py

+    executor = Client.run(
+        image,
+        options=options,
+        args=["infer", *command_args.split(" "), *extra_args],


Splitting command_args by spaces can break arguments that contain spaces (e.g., file paths with spaces). This could cause incorrect argument parsing. Consider using a more robust argument parsing approach or ensure command_args is already properly tokenized.

Copilot · 2025-09-25T08:22:08Z

brats/core/missing_mri_algorithms.py

+            log_file (Optional[Path | str], optional): Save logs to this file. Defaults to None.
+            backend (str, optional): Backend to use for inference. Defaults to "docker".


[nitpick] The docstring formatting is inconsistent. Line 134 has a period at the end while line 135 doesn't. For consistency, both lines should either have periods or not have them.

Copilot · 2025-09-25T08:22:08Z

brats/core/brats_algorithm.py

                cuda_devices=self.cuda_devices,
                force_cpu=self.force_cpu,
            )
+


[nitpick] This extra blank line is unnecessary and reduces code readability. Remove the empty line after the runner() function call.

Suggested change

MarcelRosier · 2025-09-25T08:24:45Z

brats/core/brats_algorithm.py

        inputs: dict[str, Path | str],
        output_file: Path | str,
        log_file: Optional[Path | str] = None,
+        backend: str = "docker",


Why not use the Backend enum as type and default value? Would be cleaner IMO

MarcelRosier · 2025-09-25T08:25:34Z

brats/core/brats_algorithm.py

        data_folder: Path | str,
        output_folder: Path | str,
        log_file: Optional[Path | str] = None,
+        backend: str = "docker",


MarcelRosier · 2025-09-25T08:26:18Z

brats/core/inpainting_algorithms.py

        mask: Path | str,
        output_file: Path | str,
        log_file: Optional[Path | str] = None,
+        backend: Optional[str] = None,


same for all algorithm classes: use the enum

MarcelRosier · 2025-09-25T08:28:30Z

brats/core/inpainting_algorithms.py

+            backend (str, optional): Backend to use for inference. Defaults to "docker".
        """
+        if backend is None:
+            backend_env = os.environ.get("BRATS_ORCHESTRATOR_BACKEND")


no difference but prettier: backend = os.environ.get("BRATS_ORCHESTRATOR_BACKEND", "docker")

MarcelRosier · 2025-09-25T08:39:49Z

brats/core/singularity.py

+    # ensure output folder exists
+    output_path.mkdir(parents=True, exist_ok=True)
+
+    volume_mappings = _get_volume_mappings_mlcube(


This will only work for algorithms up to 2024. In 2025 mlcube was not used anymore, see the difference here

MarcelRosier · 2025-09-25T08:41:53Z

brats/core/singularity.py

+    _sanity_check_output(
+        data_path=data_path,
+        output_path=output_path,
+        container_output="",


for debugging its really helpful to pass the actual output. always logging it on info lvl as done rn seems a bit excessive

neuronflow

I agree with Copilot's and @MarcelRosier's comments

SimoneBendazzoli93 added 9 commits September 25, 2025 03:24

Add backend parameter to inference methods for inpainting and segment…

87e226c

…ation algorithms

Add Backends enum for algorithm execution environments

e59321d

Add backend support for inference methods in BraTSAlgorithm class

b4c4903

Add Singularity support with image management and container execution

9139798

Add spython dependency to pyproject.toml

82b3123

revert poetry.lock file

4632521

Refactor backend parameter handling in inference methods to support e…

3941650

…nvironment variable configuration

Add Singularity support to README with installation instructions and …

c4cf081

…environment variable setup

Add spython package to poetry.lock for Singularity support

939b1be

SimoneBendazzoli93 requested a review from neuronflow September 25, 2025 02:01

SimoneBendazzoli93 self-assigned this Sep 25, 2025

Copilot AI review requested due to automatic review settings September 25, 2025 02:01

Copilot AI reviewed Sep 25, 2025

View reviewed changes

brats/core/singularity.py Outdated Show resolved Hide resolved

brats/core/singularity.py Outdated Show resolved Hide resolved

tests/core/test_singularity.py Outdated Show resolved Hide resolved

Autoformat with black

1092233

neuronflow requested a review from MarcelRosier September 25, 2025 02:03

SimoneBendazzoli93 and others added 3 commits September 25, 2025 11:06

Update brats/core/singularity.py

189a6eb

Co-authored-by: Copilot <[email protected]>

Remove unnecessary options for container execution in run_container f…

4df7322

…unction

Remove unused import of _ensure_image in test_singularity.py

3d3d8e2

neuronflow requested changes Sep 25, 2025

View reviewed changes

neuronflow marked this pull request as draft September 25, 2025 02:08

SimoneBendazzoli93 added 5 commits September 25, 2025 04:22

Update volume mapping function references to use _get_volume_mappings…

7eb59eb

…_mlcube

Rename _build_args to _build_command_args in singularity.py and test_…

2ec7c27

…singularity.py

Update test assertions to use mock_build_command_args in TestSingular…

11ab654

…ityHelpers

Rename run_container to run_docker_container in test_brats_algorithm.py

5984364

Rename run_container to run_docker_container in test_segmentation_alg…

87f030e

…orithms.py

SimoneBendazzoli93 marked this pull request as ready for review September 25, 2025 08:20

SimoneBendazzoli93 requested a review from Copilot September 25, 2025 08:20

Copilot AI reviewed Sep 25, 2025

View reviewed changes

MarcelRosier requested changes Sep 25, 2025

View reviewed changes

neuronflow requested changes Sep 26, 2025

View reviewed changes

Merge branch 'main' into 114-singularity-integration

5657c26

	"""Run a docker container for the provided algorithm.
	"""Run a Singularity container for the provided algorithm.

	print(f"Additional files path: {additional_files_path}")
	logger.debug(f"Additional files path: {additional_files_path}")

		log_file (Optional[Path \| str], optional): Save logs to this file. Defaults to None.
		backend (str, optional): Backend to use for inference. Defaults to "docker".

114 singularity integration #118

Are you sure you want to change the base?

114 singularity integration #118

Uh oh!

Conversation

SimoneBendazzoli93 commented Sep 25, 2025

Uh oh!

brainless-bot bot commented Sep 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neuronflow commented Sep 25, 2025

Uh oh!

brainless-bot bot commented Sep 25, 2025

Uh oh!

neuronflow left a comment

Choose a reason for hiding this comment

Uh oh!

neuronflow commented Sep 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

MarcelRosier Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

MarcelRosier Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

MarcelRosier Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

MarcelRosier Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

MarcelRosier Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

MarcelRosier Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

neuronflow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!