Repository: fsspec/filesystem_spec
Pull request #1574 (2024-04-15 19:42:19)

added type guards for MemoryFileSystem and name property to MemoryFile class
Hi everyone,

I've made small changes to the `MemoryFileSystem` class. The goal of this PR is to fix issues that prevent users from using this class with the `SimpleDirectoryReader` from llama_index. 

The first issue is that `SimpleDirectoryReader` will create Path objects and pass them to `fs.open(...)`, but the `MemoryFileSystem` class assumes path will be a string. I added a quick call to `stringify_path` in `_strip_protocol` to fix this issue.

The second issue is llamaindex assumes the file will have a `name` property, so I added this to the `MemoryFile` class.

austinsimpsond41: I'm thinking we can add a test for retrieving a file using Path. Is there anything else I'm missing?



Patch:
diff --git a/docs/source/index.rst b/docs/source/index.rst
index 5919968..1e103fd 100644
--- a/docs/source/index.rst
+++ b/docs/source/index.rst
@@ -53,6 +53,8 @@ The following libraries use ``fsspec`` internally for path and file handling:
    maintainable and modular data science code
 #. `pyxet`_, a Python library for mounting and
    accessing very large datasets from XetHub
+#. `Huggingface🤗 Datasets`_, a popular library to
+   load&manipulate data for Deep Learning models
 
 ``fsspec`` filesystems are also supported by:
 
@@ -69,9 +71,11 @@ The following libraries use ``fsspec`` internally for path and file handling:
 .. _DVC: https://dvc.org/
 .. _kedro: https://kedro.readthedocs.io/en/stable/01_introduction/01_introduction.html
 .. _pyxet: https://github.com/xetdata/pyxet
+.. _Huggingface🤗 Datasets: https://github.com/huggingface/datasets
 .. _pyarrow: https://arrow.apache.org/docs/python/
 .. _petl: https://petl.readthedocs.io/en/stable/io.html#petl.io.remotes.RemoteSource
 
+
 Installation
 ------------
 
diff --git a/fsspec/implementations/memory.py b/fsspec/implementations/memory.py
index 394fa97..83e7e74 100644
--- a/fsspec/implementations/memory.py
+++ b/fsspec/implementations/memory.py
@@ -4,9 +4,12 @@ import logging
 from datetime import datetime, timezone
 from errno import ENOTEMPTY
 from io import BytesIO
+from pathlib import PurePath, PureWindowsPath
 from typing import Any, ClassVar
 
 from fsspec import AbstractFileSystem
+from fsspec.implementations.local import LocalFileSystem
+from fsspec.utils import stringify_path
 
 logger = logging.getLogger("fsspec.memoryfs")
 
@@ -25,6 +28,12 @@ class MemoryFileSystem(AbstractFileSystem):
 
     @classmethod
     def _strip_protocol(cls, path):
+        if isinstance(path, PurePath):
+            if isinstance(path, PureWindowsPath):
+                return LocalFileSystem._strip_protocol(path)
+            else:
+                path = stringify_path(path)
+
         if path.startswith("memory://"):
             path = path[len("memory://") :]
         if "::" in path or "://" in path:
diff --git a/fsspec/spec.py b/fsspec/spec.py
index b6f73f3..9a7e4e8 100644
--- a/fsspec/spec.py
+++ b/fsspec/spec.py
@@ -1177,7 +1177,10 @@ class AbstractFileSystem(metaclass=_Cached):
         if path1 == path2:
             logger.debug("%s mv: The paths are the same, so no files were moved.", self)
         else:
-            self.copy(path1, path2, recursive=recursive, maxdepth=maxdepth)
+            # explicitly raise exception to prevent data corruption
+            self.copy(
+                path1, path2, recursive=recursive, maxdepth=maxdepth, onerror="raise"
+            )
             self.rm(path1, recursive=recursive)
 
     def rm_file(self, path):


Test patch:
diff --git a/fsspec/implementations/tests/test_memory.py b/fsspec/implementations/tests/test_memory.py
index 6646617..600022a 100644
--- a/fsspec/implementations/tests/test_memory.py
+++ b/fsspec/implementations/tests/test_memory.py
@@ -1,4 +1,5 @@
 import os
+from pathlib import PurePosixPath, PureWindowsPath
 
 import pytest
 
@@ -363,3 +364,19 @@ def test_cp_two_files(m):
         "/target/file0",
         "/target/file1",
     ]
+
+
+def test_open_path_posix(m):
+    path = PurePosixPath("/myfile/foo/bar")
+    with m.open(path, "wb") as f:
+        f.write(b"some\nlines\nof\ntext")
+
+    assert m.read_text(path) == "some\nlines\nof\ntext"
+
+
+def test_open_path_windows(m):
+    path = PureWindowsPath("C:\\myfile\\foo\\bar")
+    with m.open(path, "wb") as f:
+        f.write(b"some\nlines\nof\ntext")
+
+    assert m.read_text(path) == "some\nlines\nof\ntext"
diff --git a/fsspec/tests/abstract/mv.py b/fsspec/tests/abstract/mv.py
new file mode 100644
index 0000000..39f6caa
--- /dev/null
+++ b/fsspec/tests/abstract/mv.py
@@ -0,0 +1,57 @@
+import os
+
+import pytest
+
+import fsspec
+
+
+def test_move_raises_error_with_tmpdir(tmpdir):
+    # Create a file in the temporary directory
+    source = tmpdir.join("source_file.txt")
+    source.write("content")
+
+    # Define a destination that simulates a protected or invalid path
+    destination = tmpdir.join("non_existent_directory/destination_file.txt")
+
+    # Instantiate the filesystem (assuming the local file system interface)
+    fs = fsspec.filesystem("file")
+
+    # Use the actual file paths as string
+    with pytest.raises(FileNotFoundError):
+        fs.mv(str(source), str(destination))
+
+
+@pytest.mark.parametrize("recursive", (True, False))
+def test_move_raises_error_with_tmpdir_permission(recursive, tmpdir):
+    # Create a file in the temporary directory
+    source = tmpdir.join("source_file.txt")
+    source.write("content")
+
+    # Create a protected directory (non-writable)
+    protected_dir = tmpdir.mkdir("protected_directory")
+    protected_path = str(protected_dir)
+
+    # Set the directory to read-only
+    if os.name == "nt":
+        os.system(f'icacls "{protected_path}" /deny Everyone:(W)')
+    else:
+        os.chmod(protected_path, 0o555)  # Sets the directory to read-only
+
+    # Define a destination inside the protected directory
+    destination = protected_dir.join("destination_file.txt")
+
+    # Instantiate the filesystem (assuming the local file system interface)
+    fs = fsspec.filesystem("file")
+
+    # Try to move the file to the read-only directory, expecting a permission error
+    with pytest.raises(PermissionError):
+        fs.mv(str(source), str(destination), recursive=recursive)
+
+    # Assert the file was not created in the destination
+    assert not os.path.exists(destination)
+
+    # Cleanup: Restore permissions so the directory can be cleaned up
+    if os.name == "nt":
+        os.system(f'icacls "{protected_path}" /remove:d Everyone')
+    else:
+        os.chmod(protected_path, 0o755)  # Restore write permission for cleanup