Merge pull request #46 from bcdev/forman-45-fix_lock_path

Fix #45

Author: forman

CHANGES.md

@@ -1,5 +1,10 @@
 ## Version 0.3.1 (in development)
 
+### Fixes
+
+* Fixed an issue where an absolute lock file path was computed if the target 
+  Zarr path was relative in the local filesystem, and had no parent directory.
+  [#45]
 
 ## Version 0.3.0 (from 2024-01-26)
 

tests/fsutil/test_fileobj.py

@@ -80,15 +80,15 @@ def test_file_protocol(self):
         self.assertEqual(None, zarr_dir.storage_options)
         self.assertIsInstance(zarr_dir.fs, fsspec.AbstractFileSystem)
         self.assertEqual("file", to_protocol(zarr_dir.fs))
-        self.assertEqual(os.path.abspath("test.zarr").replace("\\", "/"), zarr_dir.path)
+        self.assertEqual(to_abs("test.zarr"), zarr_dir.path)
 
     def test_local_protocol(self):
         zarr_dir = FileObj("test.zarr")
         self.assertEqual("test.zarr", zarr_dir.uri)
         self.assertEqual(None, zarr_dir.storage_options)
         self.assertIsInstance(zarr_dir.fs, fsspec.AbstractFileSystem)
         self.assertEqual("file", to_protocol(zarr_dir.fs))
-        self.assertEqual(os.path.abspath("test.zarr").replace("\\", "/"), zarr_dir.path)
+        self.assertEqual(to_abs("test.zarr"), zarr_dir.path)
 
     def test_s3_protocol(self):
         zarr_dir = FileObj("s3://eo-data/test.zarr")
@@ -131,7 +131,7 @@ def test_truediv_override(self):
             "eo-data/test.zarr/chl/.zarray",
         )
 
-    def test_parent(self):
+    def test_parent_with_uri(self):
         file = FileObj("s3://eo-data/test.zarr/.zmetadata")
         fs = file.fs
 
@@ -157,15 +157,26 @@ def test_parent(self):
             # noinspection PyUnusedLocal
             parent = parent.parent
 
+    def test_parent_with_local(self):
         # local filesystem
         file = FileObj("test.zarr/chl/.zarray")
         fs = file.fs
         parent = file.parent
         self.assertIsInstance(parent, FileObj)
         self.assertEqual("test.zarr/chl", parent.uri)
-        self.assertEqual(
-            os.path.abspath("test.zarr/chl").replace("\\", "/"), parent.path
-        )
+        self.assertEqual(to_abs("test.zarr/chl"), parent.path)
+        self.assertIs(fs, parent.fs)
+
+        parent = parent.parent
+        self.assertIsInstance(parent, FileObj)
+        self.assertEqual("test.zarr", parent.uri)
+        self.assertEqual(to_abs("test.zarr"), parent.path)
+        self.assertIs(fs, parent.fs)
+
+        parent = parent.parent
+        self.assertIsInstance(parent, FileObj)
+        self.assertEqual("", parent.uri)
+        self.assertEqual(to_abs(""), parent.path)
         self.assertIs(fs, parent.fs)
 
     def test_parent_with_chained_uri(self):
@@ -183,12 +194,10 @@ def test_parent_with_chained_uri(self):
         parent = file.parent
         self.assertIsInstance(parent, FileObj)
         self.assertEqual("test.zarr/chl::/eo-data/test.zarr", parent.uri)
-        self.assertEqual(
-            os.path.abspath("test.zarr/chl").replace("\\", "/"), parent.path
-        )
+        self.assertEqual(to_abs("test.zarr/chl"), parent.path)
         self.assertIs(fs, parent.fs)
 
-    def test_for_path(self):
+    def test_for_path_with_simple_uri(self):
         root = FileObj("s3://eo-data/test.zarr")
 
         derived = root.for_path("")
@@ -208,6 +217,11 @@ def test_for_path_with_chained_uri(self):
             root, derived, "dir://chl/.zarray::file:/eo-data/test.zarr", "chl/.zarray"
         )
 
+    def test_for_path_with_empty_parent(self):
+        root = FileObj("test.zarr").parent
+        derived = root.for_path("test.zarr")
+        self.assert_derived_ok(root, derived, "test.zarr", to_abs("test.zarr"))
+
     def assert_derived_ok(
         self, root: FileObj, derived: FileObj, expected_uri: str, expected_path: str
     ):
@@ -217,13 +231,13 @@ def assert_derived_ok(
         self.assertIs(root.storage_options, derived.storage_options)
 
     # noinspection PyMethodMayBeStatic
-    def test_for_path_with_abs_path(self):
+    def test_raises_for_path_with_abs_path(self):
         fo = FileObj("file:/eo-data/test.zarr")
         with pytest.raises(ValueError, match="rel_path must be relative"):
             fo.for_path("/test-2.zarr")
 
     # noinspection PyMethodMayBeStatic
-    def test_for_path_with_wrong_type(self):
+    def test_raises_for_path_with_wrong_type(self):
         fo = FileObj("file:/eo-data/test.zarr")
         with pytest.raises(TypeError, match="rel_path must have type str"):
             # noinspection PyTypeChecker
@@ -282,3 +296,7 @@ def to_protocol(fs: fsspec.AbstractFileSystem):
     if isinstance(fs.protocol, tuple):
         return fs.protocol[0]
     return fs.protocol
+
+
+def to_abs(path: str) -> str:
+    return os.path.abspath(path).replace("\\", "/")

tests/fsutil/test_path.py

@@ -7,33 +7,33 @@
 import pytest
 
 from zappend.fsutil.path import split_components
-from zappend.fsutil.path import split_filename
+from zappend.fsutil.path import split_parent
 
 
 class SplitFilenameTest(unittest.TestCase):
     # noinspection PyMethodMayBeStatic
     def test_empty_path(self):
         with pytest.raises(ValueError):
-            split_filename("")
-
-    def test_split_filename_1c(self):
-        self.assertEqual(("", ""), split_filename("/"))
-        self.assertEqual(("", "a"), split_filename("a"))
-        self.assertEqual(("", "a"), split_filename("/a"))
-        self.assertEqual(("a", ""), split_filename("a/"))
-        self.assertEqual(("/a", ""), split_filename("/a/"))
-
-    def test_split_filename_2c(self):
-        self.assertEqual(("a", "b"), split_filename("a/b"))
-        self.assertEqual(("/a", "b"), split_filename("/a/b"))
-        self.assertEqual(("a/b", ""), split_filename("a/b/"))
-        self.assertEqual(("/a/b", ""), split_filename("/a/b/"))
-
-    def test_split_filename_3c(self):
-        self.assertEqual(("a/b", "c"), split_filename("a/b/c"))
-        self.assertEqual(("/a/b", "c"), split_filename("/a/b/c"))
-        self.assertEqual(("a/b/c", ""), split_filename("a/b/c/"))
-        self.assertEqual(("/a/b/c", ""), split_filename("/a/b/c/"))
+            split_parent("")
+
+    def test_split_parent_1c(self):
+        self.assertEqual(("/", ""), split_parent("/"))
+        self.assertEqual(("", "a"), split_parent("a"))
+        self.assertEqual(("/", "a"), split_parent("/a"))
+        self.assertEqual(("a", ""), split_parent("a/"))
+        self.assertEqual(("/a", ""), split_parent("/a/"))
+
+    def test_split_parent_2c(self):
+        self.assertEqual(("a", "b"), split_parent("a/b"))
+        self.assertEqual(("/a", "b"), split_parent("/a/b"))
+        self.assertEqual(("a/b", ""), split_parent("a/b/"))
+        self.assertEqual(("/a/b", ""), split_parent("/a/b/"))
+
+    def test_split_parent_3c(self):
+        self.assertEqual(("a/b", "c"), split_parent("a/b/c"))
+        self.assertEqual(("/a/b", "c"), split_parent("/a/b/c"))
+        self.assertEqual(("a/b/c", ""), split_parent("a/b/c/"))
+        self.assertEqual(("/a/b/c", ""), split_parent("/a/b/c/"))
 
 
 class SplitComponentsTest(unittest.TestCase):

tests/fsutil/test_transaction.py

@@ -141,7 +141,7 @@ def test_it_raises_on_locked_target(self):
         test_root.mkdir()
         rollback_dir = FileObj("memory://rollback")
         with Transaction(test_root, rollback_dir):
-            with pytest.raises(OSError, match="Target is locked: memory:///test.lock"):
+            with pytest.raises(OSError, match="Target is locked: memory://test.lock"):
                 with Transaction(test_root, rollback_dir):
                     pass
 
@@ -239,3 +239,35 @@ def test_it_raises_on_illegal_callback_calls(self):
         ):
             with Transaction(test_root, rollback_dir) as callback:
                 callback("replace_ifle", "I/am/the/path", b"I/m/the/data")
+
+    def test_paths_for_uri(self):
+        t = Transaction(FileObj("memory:///target.zarr"), FileObj("memory:///temp"))
+        self.assertEqual(FileObj("memory:///target.zarr"), t.target_dir)
+        self.assertEqual(FileObj("memory:///target.zarr.lock"), t.lock_file)
+        self.assertTrue(t.rollback_dir.uri.startswith("memory:///temp/zappend-"))
+        # Note, 2 slashes only!
+        t = Transaction(FileObj("memory://target.zarr"), FileObj("memory://temp"))
+        self.assertEqual(FileObj("memory://target.zarr"), t.target_dir)
+        self.assertEqual(FileObj("memory://target.zarr.lock"), t.lock_file)
+        self.assertTrue(t.rollback_dir.uri.startswith("memory://temp/zappend-"))
+
+    def test_paths_for_local(self):
+        t = Transaction(FileObj("./target.zarr"), FileObj("temp"))
+        self.assertEqual(FileObj("./target.zarr"), t.target_dir)
+        self.assertEqual(FileObj("./target.zarr.lock"), t.lock_file)
+        self.assertTrue(t.rollback_dir.uri.startswith("temp/zappend-"))
+
+        t = Transaction(FileObj("out/target.zarr"), FileObj("out/temp"))
+        self.assertEqual(FileObj("out/target.zarr"), t.target_dir)
+        self.assertEqual(FileObj("out/target.zarr.lock"), t.lock_file)
+        self.assertTrue(t.rollback_dir.uri.startswith("out/temp/zappend-"))
+
+        t = Transaction(FileObj("/out/target.zarr"), FileObj("/out/temp"))
+        self.assertEqual(FileObj("/out/target.zarr"), t.target_dir)
+        self.assertEqual(FileObj("/out/target.zarr.lock"), t.lock_file)
+        self.assertTrue(t.rollback_dir.uri.startswith("/out/temp/zappend-"))
+
+        t = Transaction(FileObj("target.zarr"), FileObj("temp"))
+        self.assertEqual(FileObj("target.zarr"), t.target_dir)
+        self.assertEqual(FileObj("target.zarr.lock"), t.lock_file)
+        self.assertTrue(t.rollback_dir.uri.startswith("temp/zappend-"))

tests/test_api.py

@@ -2,6 +2,7 @@
 # Permissions are hereby granted under the terms of the MIT License:
 # https://opensource.org/licenses/MIT.
 import os
+import shutil
 import unittest
 
 import xarray as xr
@@ -20,7 +21,7 @@ def test_no_slices(self):
         zappend([], target_dir=target_dir)
         self.assertFalse(FileObj(target_dir).exists())
 
-    def test_some_slices(self):
+    def test_some_slices_memory(self):
         target_dir = "memory://target.zarr"
         slices = [make_test_dataset(), make_test_dataset(), make_test_dataset()]
         zappend(slices, target_dir=target_dir)
@@ -29,16 +30,35 @@ def test_some_slices(self):
         self.assertEqual({"chl", "tsm"}, set(ds.data_vars))
         self.assertEqual({"time", "y", "x"}, set(ds.coords))
 
+    def test_some_slices_local(self):
+        target_dir = "target.zarr"
+        slices = [
+            "slice-1.zarr",
+            "slice-2.zarr",
+            "slice-3.zarr",
+        ]
+        for uri in slices:
+            make_test_dataset(uri=uri)
+        try:
+            zappend(slices, target_dir=target_dir)
+            ds = xr.open_zarr(target_dir)
+            self.assertEqual({"time": 9, "y": 50, "x": 100}, ds.sizes)
+            self.assertEqual({"chl", "tsm"}, set(ds.data_vars))
+            self.assertEqual({"time", "y", "x"}, set(ds.coords))
+        finally:
+            shutil.rmtree(target_dir, ignore_errors=True)
+            for slice_dir in slices:
+                shutil.rmtree(slice_dir, ignore_errors=True)
+
     def test_some_slices_with_profiling(self):
+        target_dir = "memory://target.zarr"
         slices = [
             "memory://slice-1.zarr",
             "memory://slice-2.zarr",
             "memory://slice-3.zarr",
         ]
         for uri in slices:
             make_test_dataset(uri=uri)
-
-        target_dir = "memory://target.zarr"
         try:
             zappend(
                 slices,

zappend/fsutil/fileobj.py

@@ -7,7 +7,7 @@
 
 import fsspec
 
-from .path import split_filename
+from .path import split_parent
 
 
 # Note, we could make FileObj an ABC and then introduce concrete
@@ -106,7 +106,7 @@ def close(self):
     @property
     def filename(self) -> str:
         """The filename part of the URI."""
-        return split_filename(self.path)[1]
+        return split_parent(self.path)[1]
 
     @property
     def parent(self) -> "FileObj":
@@ -115,21 +115,21 @@ def parent(self) -> "FileObj":
             # If uri is a chained URL, use path of first component
             first_uri, rest = self.uri.split("::", maxsplit=1)
             protocol, path = fsspec.core.split_protocol(first_uri)
-            parent_path, _ = split_filename(path)
+            parent_path, _ = split_parent(path)
             if protocol:
                 new_uri = f"{protocol}://{parent_path}::{rest}"
             else:
                 new_uri = f"{parent_path}::{rest}"
         else:
             protocol, path = fsspec.core.split_protocol(self.uri)
-            parent_path, _ = split_filename(path)
+            parent_path, _ = split_parent(path)
             if protocol:
                 new_uri = f"{protocol}://{parent_path}"
             else:
                 new_uri = parent_path
 
         if self._path is not None:
-            new_path, _ = split_filename(self._path)
+            new_path, _ = split_parent(self._path)
         else:
             # it is ok, we are still unresolved
             new_path = None
@@ -171,6 +171,10 @@ def for_path(self, rel_path: str) -> "FileObj":
             # If uri is a chained URL, add path to first component
             first_uri, rest = self.uri.split("::", maxsplit=1)
             new_uri = f"{first_uri}/{rel_path}::{rest}"
+        elif not self.uri:
+            new_uri = rel_path
+        elif self.uri.endswith("/"):
+            new_uri = self.uri + rel_path
         else:
             new_uri = f"{self.uri}/{rel_path}"
 

zappend/fsutil/path.py

@@ -3,13 +3,14 @@
 # https://opensource.org/licenses/MIT.
 
 
-def split_filename(path: str, sep: str = "/") -> tuple[str, str]:
+def split_parent(path: str, sep: str = "/") -> tuple[str, str]:
     if not path:
         raise ValueError("cannot get parent of empty path")
     comps = path.rsplit(sep, maxsplit=1)
     if len(comps) == 1:
         return "", comps[0]
-    return comps[0], comps[1]
+    parent, rest = comps
+    return parent or "/", rest
 
 
 def split_components(path: str, sep: str = "/") -> list[str]: