Allow spaces (and any other characters that work on the platform) in paths (#327)

* Remove validate_path and use a path with space in tests (set up to fail)

* Change test to space in path + special character; use bat rather than ps1 on win b/c easier to quote properly

* Reformat with black

Author: ethanbb

.gitignore

@@ -135,9 +135,7 @@ dmypy.json
 .vscode/
 
 # test files
-tests/tmp
-tests/videos
-tests/ground_truths*
+tests/test data
 
 # VSCode
-.vscode/
+.vscode/

environment.yml

@@ -9,3 +9,4 @@ dependencies:
   - tqdm
   - psutil
   - filelock >= 3.15.1
+  - mslex

environment_rtd.yml

@@ -9,3 +9,4 @@ dependencies:
   - psutil
   - pydata-sphinx-theme < 0.10.0
   - filelock >= 3.15.1
+  - mslex

mesmerize_core/batch_utils.py

@@ -5,8 +5,6 @@
 
 import pandas as pd
 
-from .utils import validate_path
-
 CURRENT_BATCH_PATH: Path = None  # only one batch at a time
 PARENT_DATA_PATH: Path = None
 
@@ -44,7 +42,7 @@ def set_parent_raw_data_path(path: Union[Path, str]) -> Path:
         Full parent data path
     """
     global PARENT_DATA_PATH
-    path = Path(validate_path(path))
+    path = Path(path)
     if not path.is_dir():
         raise NotADirectoryError(
             "The directory passed to `set_parent_raw_data_path()` does not exist.\n"
@@ -192,8 +190,6 @@ def load_batch(path: Union[str, Path]) -> pd.DataFrame:
 
     """
 
-    path = validate_path(path)
-
     df = pd.read_pickle(Path(path))
 
     df.paths.set_batch_path(path)
@@ -235,8 +231,6 @@ def create_batch(path: Union[str, Path], remove_existing: bool = False) -> pd.Da
         df = create_batch("/path/to/new_batch.pickle")
 
     """
-    path = validate_path(path)
-
     if Path(path).is_file():
         if remove_existing:
             os.remove(path)

mesmerize_core/caiman_extensions/common.py

@@ -9,7 +9,6 @@
 from datetime import datetime
 import time
 from copy import deepcopy
-import shlex
 
 import numpy as np
 import pandas as pd
@@ -29,11 +28,16 @@
     get_parent_raw_data_path,
     load_batch,
 )
-from ..utils import validate_path, IS_WINDOWS, make_runfile, warning_experimental
+from ..utils import IS_WINDOWS, make_runfile, warning_experimental
 from .cnmf import cnmf_cache
 from .. import algorithms
 from ..movie_readers import default_reader
 
+import shlex
+import mslex
+
+lex = mslex if IS_WINDOWS else shlex
+
 
 ALGO_MODULES = {
     "cnmf": algorithms.cnmf,
@@ -113,7 +117,6 @@ def add_item(
 
         # make sure path is within batch dir or parent raw data path
         input_movie_path = self._df.paths.resolve(input_movie_path)
-        validate_path(input_movie_path)
 
         # get relative path
         input_movie_path = self._df.paths.split(input_movie_path)[1]
@@ -545,10 +548,7 @@ def _run_subprocess(self, runfile_path: str, wait: bool, **kwargs):
 
         # Get the dir that contains the input movie
         parent_path = self._series.paths.resolve(self._series.input_movie_path).parent
-        if not IS_WINDOWS:
-            self.process = Popen(runfile_path, cwd=parent_path)
-        else:
-            self.process = Popen(f"powershell {runfile_path}", cwd=parent_path)
+        self.process = Popen([runfile_path], cwd=parent_path)
 
         if wait:
             self.process.wait()
@@ -647,16 +647,18 @@ def run(self, backend: Optional[str] = None, wait: bool = True, **kwargs):
 
         # Create the runfile in the batch dir using this Series' UUID as the filename
         if IS_WINDOWS:
-            runfile_ext = ".ps1"
+            runfile_ext = ".bat"
         else:
             runfile_ext = ".runfile"
         runfile_path = str(
             batch_path.parent.joinpath(self._series["uuid"] + runfile_ext)
         )
 
-        args_str = f"--batch-path {batch_path} --uuid {self._series.uuid}"
+        args_str = (
+            f"--batch-path {lex.quote(str(batch_path))} --uuid {self._series.uuid}"
+        )
         if get_parent_raw_data_path() is not None:
-            args_str += f" --data-path {get_parent_raw_data_path()}"
+            args_str += f" --data-path {lex.quote(str(get_parent_raw_data_path()))}"
 
         # make the runfile
         runfile_path = make_runfile(
@@ -666,14 +668,10 @@ def run(self, backend: Optional[str] = None, wait: bool = True, **kwargs):
             filename=runfile_path,  # path to create runfile
             args_str=args_str,
         )
-        try:
-            self.process = getattr(self, f"_run_{backend}")(
-                runfile_path, wait=wait, **kwargs
-            )
-        except:
-            with open(runfile_path, "r") as f:
-                raise ValueError(f.read())
 
+        self.process = getattr(self, f"_run_{backend}")(
+            runfile_path, wait=wait, **kwargs
+        )
         return self.process
 
     def get_input_movie_path(self) -> Path:

mesmerize_core/utils.py

@@ -15,13 +15,17 @@
 import sys
 from tempfile import NamedTemporaryFile
 from subprocess import check_call
+import shlex
+import mslex
 
 if os.name == "nt":
     IS_WINDOWS = True
     HOME = "USERPROFILE"
+    lex = mslex
 else:
     IS_WINDOWS = False
     HOME = "HOME"
+    lex = shlex
 
 if "MESMERIZE_LRU_CACHE" in os.environ.keys():
     MESMERIZE_LRU_CACHE = os.environ["MESMERIZE_LRU_CACHE"]
@@ -52,15 +56,6 @@ def fn(self, *args, **kwargs):
     return catcher
 
 
-def validate_path(path: Union[str, Path]):
-    if not regex.match(r"^[A-Za-z0-9@/\\:._-]*$", str(path)):
-        raise ValueError(
-            "Paths must only contain alphanumeric characters, "
-            "hyphens ( - ), underscores ( _ ) or periods ( . )"
-        )
-    return path
-
-
 def make_runfile(
     module_path: str, args_str: Optional[str] = None, filename: Optional[str] = None
 ) -> str:
@@ -87,13 +82,13 @@ def make_runfile(
 
     if filename is None:
         if IS_WINDOWS:
-            sh_file = os.path.join(os.environ[HOME], "run.ps1")
+            filename = os.path.join(os.environ[HOME], "run.bat")
         else:
-            sh_file = os.path.join(os.environ[HOME], "run.sh")
+            filename = os.path.join(os.environ[HOME], "run.sh")
     else:
         if IS_WINDOWS:
-            if not filename.endswith(".ps1"):
-                filename = filename + ".ps1"
+            if not filename.endswith(".bat"):
+                filename = filename + ".bat"
 
     sh_file = filename
 
@@ -107,13 +102,13 @@ def make_runfile(
 
             if "VIRTUAL_ENV" in os.environ.keys():
                 f.write(
-                    f'export PATH={os.environ["PATH"]}\n'
-                    f'export VIRTUAL_ENV={os.environ["VIRTUAL_ENV"]}\n'
-                    f'export LD_LIBRARY_PATH={os.environ["LD_LIBRARY_PATH"]}\n'
+                    f'export PATH={lex.quote(os.environ["PATH"])}\n'
+                    f'export VIRTUAL_ENV={lex.quote(os.environ["VIRTUAL_ENV"])}\n'
+                    f'export LD_LIBRARY_PATH={lex.quote(os.environ["LD_LIBRARY_PATH"])}\n'
                 )
 
             if "PYTHONPATH" in os.environ.keys():
-                f.write(f'export PYTHONPATH={os.environ["PYTHONPATH"]}\n')
+                f.write(f'export PYTHONPATH={lex.quote(os.environ["PYTHONPATH"])}\n')
 
             # for k, v in os.environ.items():  # copy the current environment
             #     if '\n' in v:
@@ -133,28 +128,29 @@ def make_runfile(
                 # add command to run the python script in the conda environment
                 # that was active at the time that this shell script was generated
                 f.write(
-                    f'{os.environ["CONDA_EXE"]} run -p {os.environ["CONDA_PREFIX"]} python {module_path} {args_str}'
+                    f'{lex.quote(os.environ["CONDA_EXE"])} run -p {lex.quote(os.environ["CONDA_PREFIX"])} '
+                    f"python {lex.quote(module_path)} {args_str}"
                 )
             else:
-                f.write(f"python {module_path} {args_str}")  # call the script to run
+                f.write(
+                    f"python {lex.quote(module_path)} {args_str}"
+                )  # call the script to run
 
     else:
         with open(sh_file, "w") as f:
             for k, v in os.environ.items():  # copy the current environment
                 if regex.match(r"^.*[()]", str(k)) or regex.match(r"^.*[()]", str(v)):
                     continue
-                with NamedTemporaryFile(suffix=".ps1", delete=False) as tmp:
-                    try:  # windows powershell is stupid so make sure all the env var names work
-                        tmp.write(f'$env:{k}="{v}";\n')
-                        tmp.close()
-                        check_call(f"powershell {tmp.name}")
-                        os.unlink(tmp.name)
-                    except:
-                        continue
-                f.write(
-                    f'$env:{k}="{v}";\n'
-                )  # write only env vars that powershell likes
-            f.write(f"{sys.executable} {module_path} {args_str}")
+                # with NamedTemporaryFile(suffix=".bat", delete=False, mode="w") as tmp:
+                #     try:  # windows powershell is stupid so make sure all the env var names work
+                #         tmp.write(f'$env:{k}="{v}";\n')
+                #         tmp.close()
+                #         check_call(f"powershell {tmp.name}")
+                #         os.unlink(tmp.name)
+                #     except:
+                #         continue
+                f.write(f"SET {k}={lex.quote(v)};\n")
+            f.write(f"{lex.quote(sys.executable)} {lex.quote(module_path)} {args_str}")
 
     st = os.stat(sh_file)
     os.chmod(sh_file, st.st_mode | S_IEXEC)

requirements.txt

@@ -6,6 +6,7 @@ setuptools
 wheel
 numpy
 matplotlib
+mslex
 click
 psutil
 jupyterlab

tests/test_core.py

@@ -34,11 +34,13 @@
 from copy import deepcopy
 
 # don't call "resolve" on these - want to make sure we can handle non-canonical paths correctly
-tmp_dir = Path(os.path.dirname(os.path.abspath(__file__)), "tmp")
-vid_dir = Path(os.path.dirname(os.path.abspath(__file__)), "videos")
-ground_truths_dir = Path(os.path.dirname(os.path.abspath(__file__)), "ground_truths")
+tmp_dir = Path(os.path.dirname(os.path.abspath(__file__)), "test data", "tmp")
+vid_dir = Path(os.path.dirname(os.path.abspath(__file__)), "test data", "videos")
+ground_truths_dir = Path(
+    os.path.dirname(os.path.abspath(__file__)), "test data", "ground_truths"
+)
 ground_truths_file = Path(
-    os.path.dirname(os.path.abspath(__file__)), "ground_truths.zip"
+    os.path.dirname(os.path.abspath(__file__)), "test data", "ground_truths.zip"
 )
 
 os.makedirs(tmp_dir, exist_ok=True)
@@ -74,7 +76,8 @@ def _download_ground_truths():
 
 
 def get_tmp_filename():
-    return os.path.join(tmp_dir, f"{uuid4()}.pickle")
+    # add a $ (legal on both UNIX and Windows) to ensure we are escaping it correctly
+    return os.path.join(tmp_dir, f"{uuid4()}$test.pickle")
 
 
 def clear_tmp():