Bugfiles for v0.23.0 (#724)

- The recursive machine config update did not work for dictionaries as this caused in-place modification of the memory location of the general config
- The destination selection needs to not run with string None as this causes problems if the directory is root and shorter than length 4

---------

Co-authored-by: Eu Pin Tien <eu-pin.tien@diamond.ac.uk>

Author: stephen-riggs

src/murfey/client/analyser.py

@@ -398,7 +398,9 @@ def _xml_file(self, data_file: Path) -> Path:
         base_dir, mid_dir = find_longest_data_directory(data_file, data_directories)
         if not base_dir:
             return data_file.with_suffix(".xml")
-        return base_dir / self._environment.visit / mid_dir / file_name
+        # Add the visit directory to the file path and return it
+        # The file is moved from a location where the visit name is not part of its path
+        return base_dir / self._environment.visit / (mid_dir or "") / file_name
 
     def enqueue(self, rsyncer: RSyncerUpdate):
         if not self._stopping and rsyncer.outcome == TransferResult.SUCCESS:

src/murfey/client/contexts/spa.py

@@ -3,7 +3,7 @@
 import logging
 from itertools import count
 from pathlib import Path
-from typing import Any, Dict, List, Optional, OrderedDict, Tuple
+from typing import Any, Optional, OrderedDict
 
 import xmltodict
 
@@ -50,10 +50,10 @@ def _file_transferred_to(
 
 
 def _grid_square_metadata_file(
-    f: Path, data_directories: List[Path], visit: str, grid_square: int
+    f: Path, data_directories: list[Path], visit: str, grid_square: int
 ) -> Path:
     base_dir, mid_dir = find_longest_data_directory(f, data_directories)
-    if not base_dir:
+    if not base_dir or not mid_dir:
         raise ValueError(f"Could not determine grid square metadata path for {f}")
     metadata_file = (
         base_dir
@@ -113,7 +113,7 @@ def __init__(self, acquisition_software: str, basepath: Path, token: str):
         super().__init__("SPA", acquisition_software, token)
         self._basepath = basepath
         self._processing_job_stash: dict = {}
-        self._foil_holes: Dict[int, List[int]] = {}
+        self._foil_holes: dict[int, list[int]] = {}
 
     def gather_metadata(
         self, metadata_file: Path, environment: MurfeyInstanceEnvironment | None = None
@@ -287,7 +287,7 @@ def _position_analysis(
             and self._foil_holes.get(grid_square) is None
         ):
             self._foil_holes[grid_square] = []
-            gs_pix_position: Tuple[
+            gs_pix_position: tuple[
                 Optional[int],
                 Optional[int],
                 Optional[float],
@@ -586,7 +586,7 @@ def _register_processing_job(
         self,
         tag: str,
         environment: MurfeyInstanceEnvironment,
-        parameters: Dict[str, Any] | None = None,
+        parameters: dict[str, Any] | None = None,
     ):
         return
 

src/murfey/client/destinations.py

@@ -15,17 +15,19 @@
 
 def find_longest_data_directory(
     match_path: Path, data_directories: list[str] | list[Path]
-):
+) -> tuple[Path | None, Path | None]:
     """
     Determine the longest path in the data_directories list
     which the match path is relative to
     """
     base_dir: Path | None = None
     mid_dir: Path | None = None
     for dd in data_directories:
-        dd_base = str(Path(dd).absolute())
-        if str(match_path).startswith(str(dd)) and len(dd_base) > len(str(base_dir)):
-            base_dir = Path(dd_base)
+        dd_base = Path(dd).absolute()
+        if match_path.absolute().is_relative_to(dd_base) and len(dd_base.parts) > (
+            len(base_dir.parts) if base_dir else 0
+        ):
+            base_dir = dd_base
             mid_dir = match_path.absolute().relative_to(Path(base_dir)).parent
     return base_dir, mid_dir
 

src/murfey/util/config.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import copy
 import os
 import socket
 from functools import lru_cache
@@ -183,7 +184,7 @@ def _recursive_update(base: dict[str, Any], new: dict[str, Any]):
                 base[key].extend(value)
             # Otherwise, overwrite/add values as normal
             else:
-                base[key] = value
+                base[key] = copy.deepcopy(value)
         return base
 
     # Load the dict from the file

tests/client/test_destinations.py

@@ -2,8 +2,65 @@
 from unittest import mock
 
 import pytest
+from pytest_mock import MockerFixture
+
+from murfey.client.destinations import (
+    determine_default_destination,
+    find_longest_data_directory,
+)
+
+
+@pytest.mark.parametrize(
+    "test_params",
+    (
+        # File to match | Use Path? | Expected result
+        (
+            "X:/cm12345-6/Supervisor/Images-Disc1/file",
+            True,
+            ("X:/", "cm12345-6/Supervisor/Images-Disc1"),
+        ),
+        (
+            "X:/DATA/cm12345-6/Supervisor/Images-Disc1/file",
+            False,
+            ("X:/DATA", "cm12345-6/Supervisor/Images-Disc1"),
+        ),
+        (
+            "X:/DoseFractions/cm12345-6/Supervisor/Images-Disc1/file",
+            True,
+            ("X:/DoseFractions", "cm12345-6/Supervisor/Images-Disc1"),
+        ),
+        (
+            "X:/DoseFractions/DATA/cm12345-6/Supervisor/Images-Disc1/file",
+            False,
+            ("X:/DoseFractions/DATA", "cm12345-6/Supervisor/Images-Disc1"),
+        ),
+    ),
+)
+def test_find_longest_data_directory(
+    mocker: MockerFixture, test_params: tuple[str, bool, tuple[str, str]]
+):
+    # Unpack test params
+    match_path, use_path, (expected_base_dir, expected_mid_dir) = test_params
+
+    # Construct data directories using strings or Paths as needed
+    data_directories: list[str] | list[Path] = [
+        "X:",
+        "X:/DATA",
+        "X:/DoseFractions",
+        "X:/DoseFractions/DATA",
+    ]
+    if use_path:
+        data_directories = [Path(dd) for dd in data_directories]
+    # Patch Pathlib's 'absolute()' function, since we are simulating Windows on Linux
+    mocker.patch("murfey.client.destinations.Path.absolute", lambda self: self)
+
+    base_dir, mid_dir = find_longest_data_directory(
+        Path(match_path),
+        data_directories,
+    )
+    assert base_dir == Path(expected_base_dir)
+    assert mid_dir == Path(expected_mid_dir)
 
-from murfey.client.destinations import determine_default_destination
 
 source_list = [
     ["X:/DoseFractions/cm12345-6/Supervisor", "Supervisor", True, "extra_name"],
@@ -71,6 +128,56 @@ def test_determine_default_destinations_suggested_path(mock_post, mock_get, sour
     )
 
 
+@mock.patch("murfey.client.destinations.capture_get")
+@mock.patch("murfey.client.destinations.capture_post")
+def test_determine_default_destinations_short_path(mock_post, mock_get):
+    """Test for the case where the data directory is a drive"""
+    mock_environment = mock.Mock()
+    mock_environment.murfey_session = 2
+    mock_environment.instrument_name = "m01"
+    mock_environment.destination_registry = {}
+
+    mock_get().json.return_value = {"data_directories": ["X:/"]}
+    mock_post().json.return_value = {"suggested_path": "/base_path/2025/cm12345-6/raw"}
+
+    destination = determine_default_destination(
+        visit="cm12345-6",
+        source=Path("X:/cm12345-6/Supervisor"),
+        destination="2025",
+        environment=mock_environment,
+        token="token",
+        touch=True,
+        extra_directory="",
+        use_suggested_path=True,
+    )
+    mock_get.assert_any_call(
+        base_url=str(mock_environment.url.geturl()),
+        router_name="session_control.router",
+        function_name="machine_info_by_instrument",
+        token="token",
+        instrument_name="m01",
+    )
+    mock_post.assert_any_call(
+        base_url=str(mock_environment.url.geturl()),
+        router_name="file_io_instrument.router",
+        function_name="suggest_path",
+        token="token",
+        visit_name="cm12345-6",
+        session_id=2,
+        data={
+            "base_path": "2025/cm12345-6/raw",
+            "touch": True,
+            "extra_directory": "",
+        },
+    )
+
+    assert destination == "/base_path/2025/cm12345-6/raw/"
+    assert (
+        mock_environment.destination_registry.get("Supervisor")
+        == "/base_path/2025/cm12345-6/raw"
+    )
+
+
 @mock.patch("murfey.client.destinations.capture_get")
 @pytest.mark.parametrize("sources", source_list)
 def test_determine_default_destinations_skip_suggested(mock_get, sources):

tests/util/test_config.py

@@ -1,3 +1,4 @@
+import copy
 from pathlib import Path
 from typing import Any
 
@@ -132,19 +133,31 @@ def mock_instrument_config():
     }
 
 
+@pytest.fixture
+def mock_second_instrument_config(mock_instrument_config: dict[str, Any]):
+    """Test the case of recursive dictionaries which are present
+    in both the general and instrument configs"""
+    second_instrument_config = copy.deepcopy(mock_instrument_config)
+    second_instrument_config.update(
+        {"pkg_1": {"file_path": "/path/to/pkg_1/file_2.txt"}}
+    )
+    return second_instrument_config
+
+
 @pytest.fixture
 def mock_hierarchical_machine_config_yaml(
     mock_general_config: dict[str, Any],
     mock_tem_shared_config: dict[str, Any],
     mock_instrument_config: dict[str, Any],
+    mock_second_instrument_config: dict[str, Any],
     tmp_path: Path,
 ):
     # Create machine config (with all currently supported keys) for the instrument
     hierarchical_config = {
         "general": mock_general_config,
         "tem": mock_tem_shared_config,
         "m01": mock_instrument_config,
-        "m02": mock_instrument_config,
+        "m02": mock_second_instrument_config,
     }
     config_file = tmp_path / "config" / "murfey-machine-config-hierarchical.yaml"
     config_file.parent.mkdir(parents=True, exist_ok=True)
@@ -210,6 +223,7 @@ def test_get_machine_config(
     mock_general_config: dict[str, Any],
     mock_tem_shared_config: dict[str, Any],
     mock_instrument_config: dict[str, Any],
+    mock_second_instrument_config: dict[str, Any],
     mock_hierarchical_machine_config_yaml: Path,
     mock_standard_machine_config_yaml: Path,
     test_params: tuple[str, list[str]],
@@ -353,14 +367,26 @@ def test_get_machine_config(
                 == mock_instrument_config["node_creator_queue"]
             )
             # Extra keys
-            assert config[i].pkg_1 == {
-                "file_path": "/path/to/pkg_1/file.txt",
-                "command": [
-                    "/path/to/executable",
-                    "--some_arg",
-                    "-a",
-                    "./path/to/file",
-                ],
-                "step_size": 100,
-            }
+            if config_to_test == "standard" or i == "m01":
+                assert config[i].pkg_1 == {
+                    "file_path": "/path/to/pkg_1/file.txt",
+                    "command": [
+                        "/path/to/executable",
+                        "--some_arg",
+                        "-a",
+                        "./path/to/file",
+                    ],
+                    "step_size": 100,
+                }
+            elif i == "m02":
+                assert config[i].pkg_1 == {
+                    "file_path": "/path/to/pkg_1/file_2.txt",
+                    "command": [
+                        "/path/to/executable",
+                        "--some_arg",
+                        "-a",
+                        "./path/to/file",
+                    ],
+                    "step_size": 100,
+                }
             assert config[i].pkg_2 == mock_general_config["pkg_2"]