fix: Bug in volume error handling (#272)

#### Relevant issue or PR
n/a

#### Description of changes
Ensure error is raised correctly if a passed volume path does not exist.

#### Testing done
manual

Author: dionhaefner

tesseract_core/sdk/engine.py

@@ -38,6 +38,7 @@
     Container,
     ContainerError,
     Image,
+    NotFound,
     build_docker_image,
     is_podman,
 )
@@ -721,9 +722,12 @@ def serve(
         compose_file.flush()
 
         project_name = f"tesseract-{_id_generator()}"
-        if not docker_client.compose.up(compose_file.name, project_name):
-            raise RuntimeError("Cannot serve Tesseracts")
-        return project_name
+        try:
+            docker_client.compose.up(compose_file.name, project_name)
+        except APIError as exc:
+            raise RuntimeError("Cannot serve Tesseracts") from exc
+
+    return project_name
 
 
 def _create_docker_compose_template(
@@ -802,27 +806,23 @@ def _create_docker_compose_template(
 
         services.append(service)
 
-    docker_volumes = {}  # Dictionary of volume names mapped to whether or not they already exist
-    if volumes:
-        for volume in volumes:
-            source = volume.split(":")[0]
-            # Check if source exists to determine if specified volume is a docker volume
-            if not Path(source).exists():
-                # Check if volume exists
-                if not docker_client.volumes.get(source):
-                    if "/" not in source:
-                        docker_volumes[source] = False
-                    else:
-                        raise ValueError(
-                            f"Volume/Path {source} does not already exist, "
-                            "and new volume cannot be created due to '/' in name."
-                        )
-                else:
-                    # Docker volume is external
-                    docker_volumes[source] = True
+    external_volume_map = {}
+    for source in parsed_volumes:
+        # Check if the volume is a local bind-mount
+        if _is_local_volume(source):
+            continue
+
+        # Check if the volume is an existing Docker volume
+        try:
+            docker_client.volumes.get(source)
+        except NotFound:
+            external_volume_map[source] = False
+        else:
+            # If the volume exists, it is an external Docker volume
+            external_volume_map[source] = True
 
     template = ENV.get_template("docker-compose.yml")
-    return template.render(services=services, docker_volumes=docker_volumes)
+    return template.render(services=services, named_volumes=external_volume_map)
 
 
 def _id_generator(
@@ -832,6 +832,11 @@ def _id_generator(
     return "".join(random.choice(chars) for _ in range(size))
 
 
+def _is_local_volume(volume: str) -> bool:
+    """Check if a volume is a local path."""
+    return "/" in volume or "." in volume
+
+
 def _parse_volumes(options: list[str]) -> dict[str, dict[str, str]]:
     """Parses volume mount strings to dict accepted by docker SDK.
 
@@ -852,8 +857,12 @@ def _parse_option(option: str):
                 "(must be `/path/to/source:/path/totarget:(ro|rw)`)",
             )
 
-        is_local_mount = "/" in source or Path(source).exists()
-        if is_local_mount:
+        if _is_local_volume(source):
+            if not Path(source).exists():
+                raise RuntimeError(
+                    f"Source path {source} does not exist, "
+                    "please provide a valid local path."
+                )
             # Docker doesn't like paths like ".", so we convert to absolute path here
             source = str(Path(source).resolve())
         return source, {"bind": target, "mode": mode}

tesseract_core/sdk/templates/docker-compose.yml

@@ -42,14 +42,11 @@ services:
     {% endif %}
 {% endfor %}
 
-
-{%- if docker_volumes %}
+{%- if named_volumes %}
 volumes:
-{%- for name, external in docker_volumes.items() %}
+{%- for name, external in named_volumes.items() %}
   {{ name }}:
-    {%- if external %}
-    external: true
-    {% endif %}
+    external: {{ external }}
 {% endfor %}
 {% endif %}
 

tests/conftest.py

@@ -334,6 +334,11 @@ def wait(self, **kwargs: Any):
             """Mock wait method for Container."""
             return {"StatusCode": 0, "Error": None}
 
+        @property
+        def name(self):
+            """Mock name property for Container."""
+            return json.dumps(self.return_args)
+
         @property
         def attrs(self):
             """Mock attrs method for Container."""
@@ -363,6 +368,26 @@ def info() -> tuple:
             """Mock info method for DockerClient."""
             return "", ""
 
+        class volumes:
+            """Mock of CLIDockerClient.volumes."""
+
+            @staticmethod
+            def create(name: str) -> Any:
+                """Mock of CLIDockerClient.volumes.create."""
+                return {"Name": name}
+
+            @staticmethod
+            def get(name: str) -> Any:
+                """Mock of CLIDockerClient.volumes.get."""
+                if "/" in name:
+                    raise NotFound(f"Volume {name} not found")
+                return {"Name": name}
+
+            @staticmethod
+            def list() -> list[Any]:
+                """Mock of CLIDockerClient.volumes.list."""
+                return [{"Name": "test_volume"}]
+
         class images:
             """Mock of CLIDockerClient.images."""
 
@@ -451,4 +476,12 @@ def exists(project_id: str) -> bool:
         tesseract_core.sdk.docker_client, "CLIDockerClient", MockedDocker
     )
 
+    def hacked_get(url, *args, **kwargs):
+        if url.endswith("/health"):
+            # Simulate a successful health check
+            return type("Response", (), {"status_code": 200, "json": lambda: {}})()
+        raise NotImplementedError(f"Mocked get request to {url} not implemented")
+
+    engine.requests.get = hacked_get
+
     yield mock_instance

tests/sdk_tests/test_engine.py

@@ -182,11 +182,11 @@ def test_run_tesseract_file_input(mocked_docker, tmpdir):
         "foobar",
         "apply",
         [f"@{infile}", "--output-path", str(outdir)],
-        volumes=["/path/on/host:/path/in/container:ro"],
+        volumes=[f"{tmpdir}:/path/in/container:ro"],
     )
     res = json.loads(res)
-    assert res["volumes"].keys() == {str(infile), str(outdir), "/path/on/host"}
-    assert res["volumes"]["/path/on/host"] == {
+    assert res["volumes"].keys() == {str(infile), str(outdir), f"{tmpdir}"}
+    assert res["volumes"][f"{tmpdir}"] == {
         "mode": "ro",
         "bind": "/path/in/container",
     }
@@ -286,6 +286,50 @@ def test_serve_tesseracts(mocked_docker):
     assert project_name_multi_tesseract
 
 
+@pytest.mark.parametrize("no_compose", [True, False])
+def test_serve_tesseract_volumes(mocked_docker, tmpdir, no_compose):
+    """Test running a tesseract with volumes."""
+    # Test with a single volume
+    res = engine.serve(
+        ["foobar"],
+        volumes=[f"{tmpdir}:/path/in/container:ro"],
+        no_compose=no_compose,
+    )
+
+    if no_compose:
+        # Currently no good way to test return value of serve with no_compose=True
+        # since it returns a project ID.
+        res = json.loads(res)
+        assert res["volumes"].keys() == {f"{tmpdir}"}
+        assert res["volumes"][f"{tmpdir}"] == {
+            "mode": "ro",
+            "bind": "/path/in/container",
+        }
+
+    # Test with a named volume
+    res = engine.serve(
+        ["foobar"],
+        volumes=["my_named_volume:/path/in/container:ro"],
+        no_compose=no_compose,
+    )
+
+    if no_compose:
+        res = json.loads(res)
+        assert res["volumes"].keys() == {"my_named_volume"}
+        assert res["volumes"]["my_named_volume"] == {
+            "mode": "ro",
+            "bind": "/path/in/container",
+        }
+
+    with pytest.raises(RuntimeError):
+        # Test with a volume that does not exist
+        engine.serve(
+            ["foobar"],
+            volumes=["/non/existent/path:/path/in/container:ro"],
+            no_compose=no_compose,
+        )
+
+
 def test_needs_docker(mocked_docker, monkeypatch):
     @engine.needs_docker
     def run_something_with_docker():