From 299d4c6db5382d2d919acd330aa97b8b3104a71c Mon Sep 17 00:00:00 2001 From: Samuel Garcia Date: Fri, 1 Dec 2023 10:38:02 +0100 Subject: [PATCH 01/12] Improve the BaseExtractor.to_dict() relative_to machanism to make iot safer on windows. --- src/spikeinterface/core/base.py | 33 +++--- src/spikeinterface/core/binaryfolder.py | 5 +- src/spikeinterface/core/core_tools.py | 108 +++++++++++++++++++ src/spikeinterface/core/npyfoldersnippets.py | 5 +- src/spikeinterface/core/sortingfolder.py | 5 +- 5 files changed, 131 insertions(+), 25 deletions(-) diff --git a/src/spikeinterface/core/base.py b/src/spikeinterface/core/base.py index 33a7ca41f5..794f610bd9 100644 --- a/src/spikeinterface/core/base.py +++ b/src/spikeinterface/core/base.py @@ -15,7 +15,7 @@ import numpy as np from .globals import get_global_tmp_folder, is_set_global_tmp_folder -from .core_tools import check_json, is_dict_extractor, recursive_path_modifier, SIJsonEncoder +from .core_tools import check_json, is_dict_extractor, SIJsonEncoder, make_paths_relative, make_paths_absolute, check_paths_relative from .job_tools import _shared_job_kwargs_doc @@ -383,7 +383,8 @@ def to_dict( to_dict_kwargs = dict( include_annotations=include_annotations, include_properties=include_properties, - relative_to=None, # '_make_paths_relative' is already recursive! + # make_paths_relative() will make the recusrivity later: + relative_to=None, folder_metadata=folder_metadata, recursive=recursive, ) @@ -414,7 +415,7 @@ def to_dict( "module": module, "kwargs": kwargs, "version": module_version, - "relative_paths": (relative_to is not None), + # "relative_paths": (relative_to is not None), } dump_dict["version"] = module_version # Can be spikeinterface, spikefores, etc. @@ -431,10 +432,20 @@ def to_dict( # include only main properties dump_dict["properties"] = {k: self._properties.get(k, None) for k in self._main_properties} - if relative_to is not None: + if relative_to is None: + dump_dict["relative_paths"] = False + else: relative_to = Path(relative_to).resolve().absolute() assert relative_to.is_dir(), "'relative_to' must be an existing directory" - dump_dict = _make_paths_relative(dump_dict, relative_to) + + if check_paths_relative(dump_dict, relative_to): + dump_dict["relative_paths"] = True + dump_dict = make_paths_relative(dump_dict, relative_to) + else: + dump_dict["relative_paths"] = False + # @alessio @ramon @zach: is this warning wanted or not ? + warnings.warn("Try to BaseExtractor.to_dict() using relative_to but there is no common folder") + if folder_metadata is not None: if relative_to is not None: @@ -463,7 +474,7 @@ def from_dict(dictionary: dict, base_folder: Optional[Union[Path, str]] = None) # for pickle dump relative_path was not in the dict, this ensure compatibility if dictionary.get("relative_paths", False): assert base_folder is not None, "When relative_paths=True, need to provide base_folder" - dictionary = _make_paths_absolute(dictionary, base_folder) + dictionary = make_paths_absolute(dictionary, base_folder) extractor = _load_extractor_from_dict(dictionary) folder_metadata = dictionary.get("folder_metadata", None) if folder_metadata is not None: @@ -1008,16 +1019,6 @@ def save_to_zarr( return cached -def _make_paths_relative(d, relative) -> dict: - relative = str(Path(relative).resolve().absolute()) - func = lambda p: os.path.relpath(str(p), start=relative) - return recursive_path_modifier(d, func, target="path", copy=True) - - -def _make_paths_absolute(d, base): - base = Path(base) - func = lambda p: str((base / p).resolve().absolute()) - return recursive_path_modifier(d, func, target="path", copy=True) def _load_extractor_from_dict(dic) -> BaseExtractor: diff --git a/src/spikeinterface/core/binaryfolder.py b/src/spikeinterface/core/binaryfolder.py index b3ae8c3145..3f1bfc6be6 100644 --- a/src/spikeinterface/core/binaryfolder.py +++ b/src/spikeinterface/core/binaryfolder.py @@ -3,9 +3,8 @@ import numpy as np -from .base import _make_paths_absolute from .binaryrecordingextractor import BinaryRecordingExtractor -from .core_tools import define_function_from_class +from .core_tools import define_function_from_class, make_paths_absolute class BinaryFolderRecording(BinaryRecordingExtractor): @@ -40,7 +39,7 @@ def __init__(self, folder_path): assert d["relative_paths"] - d = _make_paths_absolute(d, folder_path) + d = make_paths_absolute(d, folder_path) BinaryRecordingExtractor.__init__(self, **d["kwargs"]) diff --git a/src/spikeinterface/core/core_tools.py b/src/spikeinterface/core/core_tools.py index f8ea1dff35..587e636277 100644 --- a/src/spikeinterface/core/core_tools.py +++ b/src/spikeinterface/core/core_tools.py @@ -1,4 +1,5 @@ from pathlib import Path +import pathlib from typing import Union import os import sys @@ -851,6 +852,113 @@ def recursive_path_modifier(d, func, target="path", copy=True) -> dict: raise ValueError(f"{k} key for path must be str or list[str]") + +def _get_paths_list(d): + # this explore a dict and get all paths flatten in a list + # the trick is to use a closure func called by recursive_path_modifier() + path_list = [] + def append_to_path(p): + path_list.append(p) + recursive_path_modifier(d, append_to_path, target="path", copy=False) + return path_list + + +def _relative_to(p, relative_folder): + # custum os.path.relpath() with more checks + + relative_folder = Path(relative_folder).resolve().absolute() + p = Path(p).resolve().absolute() + # the as_posix transform \\ to / on window which make better json files + rel_to = os.path.relpath(p.as_posix(), start=relative_folder.as_posix()) + return rel_to + +def check_paths_relative(input_dict, relative_folder) -> bool: + """ + Check if relative path is possible to be applied on a dict describing an BaseExtractor. + + For instance on windows, if some data are on a driver "d:/" and the folder is on drive "C:/" it returns False. + + Parameters + ---------- + input_dict: dict + A dict describing an extactor obtained by BaseExtractor.to_dict() + relative_folder: str or Path + The folder to be relative to. + + Returns + ------- + relative_possible: bool + """ + path_list = _get_paths_list(input_dict) + relative_folder = Path(relative_folder).resolve().absolute() + not_possible = [] + for p in path_list: + p = Path(p) + # check path is not an URL + if 'http' in str(p): + not_possible.append(p) + + # If windows path check have same drive + if isinstance(p, pathlib.WindowsPath) and isinstance(relative_folder, pathlib.WindowsPath): + # check that on same drive + # important note : for UNC path on window the "//host/shared" is the drive + if p.drive != relative_folder.drive: + not_possible.append(p) + + # check relative is possible + try: + _relative_to(p, relative_folder) + except ValueError: + not_possible.append(p) + + return len(not_possible) == 0 + + +def make_paths_relative(input_dict, relative_folder) -> dict: + """ + Recursivelly transform a dict describing an BaseExtractor to make every path relative to a folder. + + Parameters + ---------- + input_dict: dict + A dict describing an extactor obtained by BaseExtractor.to_dict() + relative_folder: str or Path + The folder to be relative to. + + Returns + ------- + output_dict: dict + A copy of the input dict with modified paths. + """ + relative_folder = Path(relative_folder).resolve().absolute() + func = lambda p: _relative_to(p, relative_folder) + output_dict = recursive_path_modifier(input_dict, func, target="path", copy=True) + return output_dict + + +def make_paths_absolute(input_dict, base_folder): + """ + Recursivelly transform a dict describing an BaseExtractor to make every path absolut given a base_folder. + + Parameters + ---------- + input_dict: dict + A dict describing an extactor obtained by BaseExtractor.to_dict() + base_folder: str or Path + The folder to be relative to. + + Returns + ------- + output_dict: dict + A copy of the input dict with modified paths. + """ + base_folder = Path(base_folder) + # use as_posix instead of str to make the path unix like even on window + func = lambda p: (base_folder / p).resolve().absolute().as_posix() + output_dict = recursive_path_modifier(input_dict, func, target="path", copy=True) + return output_dict + + def recursive_key_finder(d, key): # Find all values for a key on a dictionary, even if nested for k, v in d.items(): diff --git a/src/spikeinterface/core/npyfoldersnippets.py b/src/spikeinterface/core/npyfoldersnippets.py index 271a8f4f12..6a036022dc 100644 --- a/src/spikeinterface/core/npyfoldersnippets.py +++ b/src/spikeinterface/core/npyfoldersnippets.py @@ -3,9 +3,8 @@ import numpy as np -from .base import _make_paths_absolute from .npysnippetsextractor import NpySnippetsExtractor -from .core_tools import define_function_from_class +from .core_tools import define_function_from_class, make_paths_absolute class NpyFolderSnippets(NpySnippetsExtractor): @@ -41,7 +40,7 @@ def __init__(self, folder_path): assert d["relative_paths"] - d = _make_paths_absolute(d, folder_path) + d = make_paths_absolute(d, folder_path) NpySnippetsExtractor.__init__(self, **d["kwargs"]) diff --git a/src/spikeinterface/core/sortingfolder.py b/src/spikeinterface/core/sortingfolder.py index c86eb0d796..40614d4685 100644 --- a/src/spikeinterface/core/sortingfolder.py +++ b/src/spikeinterface/core/sortingfolder.py @@ -3,10 +3,9 @@ import numpy as np -from .base import _make_paths_absolute from .basesorting import BaseSorting, BaseSortingSegment from .npzsortingextractor import NpzSortingExtractor -from .core_tools import define_function_from_class +from .core_tools import define_function_from_class, make_paths_absolute from .numpyextractors import NumpySortingSegment @@ -107,7 +106,7 @@ def __init__(self, folder_path): assert d["relative_paths"] - d = _make_paths_absolute(d, folder_path) + d = make_paths_absolute(d, folder_path) NpzSortingExtractor.__init__(self, **d["kwargs"]) From adc288e5f09fe7a4a5a367f57cace1a14fdc494f Mon Sep 17 00:00:00 2001 From: Samuel Garcia Date: Fri, 1 Dec 2023 11:00:45 +0100 Subject: [PATCH 02/12] Some tests for path manipualtion --- .../core/tests/test_core_tools.py | 54 +++++++++++++++---- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/src/spikeinterface/core/tests/test_core_tools.py b/src/spikeinterface/core/tests/test_core_tools.py index 8e0fe4a744..df02558750 100644 --- a/src/spikeinterface/core/tests/test_core_tools.py +++ b/src/spikeinterface/core/tests/test_core_tools.py @@ -2,7 +2,7 @@ from multiprocessing.shared_memory import SharedMemory from pathlib import Path import importlib - +import pathlib import pytest import numpy as np @@ -10,6 +10,9 @@ write_binary_recording, write_memory_recording, recursive_path_modifier, + make_paths_relative, + make_paths_absolute, + check_paths_relative, ) from spikeinterface.core.binaryrecordingextractor import BinaryRecordingExtractor from spikeinterface.core.generate import NoiseGeneratorRecording @@ -163,17 +166,17 @@ def test_write_memory_recording(): write_memory_recording(recording, dtype=None, verbose=False, n_jobs=2, total_memory="200k", progress_bar=True) -def test_recursive_path_modifier(): +def test_path_utils_functions(): # this test nested depth 2 path modifier d = { "kwargs": { - "path": "/yep/path1", + "path": "/yep/sub/path1", "recording": { "module": "mock_module", "class": "mock_class", "version": "1.2", "annotations": {}, - "kwargs": {"path": "/yep/path2"}, + "kwargs": {"path": "/yep/sub/path2"}, }, } } @@ -182,13 +185,46 @@ def test_recursive_path_modifier(): assert d2["kwargs"]["path"].startswith("/yop") assert d2["kwargs"]["recording"]["kwargs"]["path"].startswith("/yop") + d3 = make_paths_relative(d, Path("/yep")) + assert d3["kwargs"]["path"] == "sub/path1" + assert d3["kwargs"]["recording"]["kwargs"]["path"] == "sub/path2" + + d4 = make_paths_absolute(d3, "/yop") + assert d4["kwargs"]["path"].startswith("/yop") + assert d4["kwargs"]["recording"]["kwargs"]["path"].startswith("/yop") + + + + if platform.system() == "Windows": + # test for windows Path + d = { + "kwargs": { + "path": "c:\\yep\\sub\\path1", + "recording": { + "kwargs": {"path": "c:\\yep\\sub\\path2"}, + }, + } + } + d2 = make_paths_relative(d, pathlib.PureWindowsPath("c:\\yep\\")) + print(d2) + + + # make_paths_relative, + # make_paths_absolute, + # check_paths_relative, + + + + + if __name__ == "__main__": # Create a temporary folder using the standard library - import tempfile + # import tempfile - with tempfile.TemporaryDirectory() as tmpdirname: - tmp_path = Path(tmpdirname) - test_write_binary_recording(tmp_path) + # with tempfile.TemporaryDirectory() as tmpdirname: + # tmp_path = Path(tmpdirname) + # test_write_binary_recording(tmp_path) # test_write_memory_recording() - # test_recursive_path_modifier() + + test_path_utils_functions() From 5753dc51227435e68e0942061f0c761b0d6b9441 Mon Sep 17 00:00:00 2001 From: Samuel Garcia Date: Fri, 1 Dec 2023 12:03:48 +0100 Subject: [PATCH 03/12] fix relative path test on windows --- src/spikeinterface/core/core_tools.py | 11 ++- .../core/tests/test_core_tools.py | 75 +++++++++++++------ 2 files changed, 58 insertions(+), 28 deletions(-) diff --git a/src/spikeinterface/core/core_tools.py b/src/spikeinterface/core/core_tools.py index 587e636277..a81e1382d0 100644 --- a/src/spikeinterface/core/core_tools.py +++ b/src/spikeinterface/core/core_tools.py @@ -859,7 +859,7 @@ def _get_paths_list(d): path_list = [] def append_to_path(p): path_list.append(p) - recursive_path_modifier(d, append_to_path, target="path", copy=False) + recursive_path_modifier(d, append_to_path, target="path", copy=True) return path_list @@ -870,7 +870,7 @@ def _relative_to(p, relative_folder): p = Path(p).resolve().absolute() # the as_posix transform \\ to / on window which make better json files rel_to = os.path.relpath(p.as_posix(), start=relative_folder.as_posix()) - return rel_to + return Path(rel_to).as_posix() def check_paths_relative(input_dict, relative_folder) -> bool: """ @@ -897,19 +897,22 @@ def check_paths_relative(input_dict, relative_folder) -> bool: # check path is not an URL if 'http' in str(p): not_possible.append(p) + continue # If windows path check have same drive if isinstance(p, pathlib.WindowsPath) and isinstance(relative_folder, pathlib.WindowsPath): # check that on same drive # important note : for UNC path on window the "//host/shared" is the drive - if p.drive != relative_folder.drive: + if p.resolve().absolute().drive != relative_folder.drive: not_possible.append(p) + continue # check relative is possible try: - _relative_to(p, relative_folder) + p2 = _relative_to(p, relative_folder) except ValueError: not_possible.append(p) + continue return len(not_possible) == 0 diff --git a/src/spikeinterface/core/tests/test_core_tools.py b/src/spikeinterface/core/tests/test_core_tools.py index df02558750..99a2c54b61 100644 --- a/src/spikeinterface/core/tests/test_core_tools.py +++ b/src/spikeinterface/core/tests/test_core_tools.py @@ -167,32 +167,32 @@ def test_write_memory_recording(): def test_path_utils_functions(): - # this test nested depth 2 path modifier - d = { - "kwargs": { - "path": "/yep/sub/path1", - "recording": { - "module": "mock_module", - "class": "mock_class", - "version": "1.2", - "annotations": {}, - "kwargs": {"path": "/yep/sub/path2"}, - }, + if platform.system() != "Windows": + # posix path + d = { + "kwargs": { + "path": "/yep/sub/path1", + "recording": { + "module": "mock_module", + "class": "mock_class", + "version": "1.2", + "annotations": {}, + "kwargs": {"path": "/yep/sub/path2"}, + }, + } } - } - - d2 = recursive_path_modifier(d, lambda p: p.replace("/yep", "/yop")) - assert d2["kwargs"]["path"].startswith("/yop") - assert d2["kwargs"]["recording"]["kwargs"]["path"].startswith("/yop") - d3 = make_paths_relative(d, Path("/yep")) - assert d3["kwargs"]["path"] == "sub/path1" - assert d3["kwargs"]["recording"]["kwargs"]["path"] == "sub/path2" + d2 = recursive_path_modifier(d, lambda p: p.replace("/yep", "/yop")) + assert d2["kwargs"]["path"].startswith("/yop") + assert d2["kwargs"]["recording"]["kwargs"]["path"].startswith("/yop") - d4 = make_paths_absolute(d3, "/yop") - assert d4["kwargs"]["path"].startswith("/yop") - assert d4["kwargs"]["recording"]["kwargs"]["path"].startswith("/yop") + d3 = make_paths_relative(d, Path("/yep")) + assert d3["kwargs"]["path"] == "sub/path1" + assert d3["kwargs"]["recording"]["kwargs"]["path"] == "sub/path2" + d4 = make_paths_absolute(d3, "/yop") + assert d4["kwargs"]["path"].startswith("/yop") + assert d4["kwargs"]["recording"]["kwargs"]["path"].startswith("/yop") if platform.system() == "Windows": @@ -201,12 +201,39 @@ def test_path_utils_functions(): "kwargs": { "path": "c:\\yep\\sub\\path1", "recording": { + "module": "mock_module", + "class": "mock_class", + "version": "1.2", + "annotations": {}, "kwargs": {"path": "c:\\yep\\sub\\path2"}, }, } } - d2 = make_paths_relative(d, pathlib.PureWindowsPath("c:\\yep\\")) - print(d2) + + d2 = make_paths_relative(d, "c:\\yep") + # the str be must unix like path even on windows for more portability + assert d2["kwargs"]["path"] == "sub/path1" + assert d2["kwargs"]["recording"]["kwargs"]["path"] == "sub/path2" + + # same drive + assert check_paths_relative(d, "c:\\yep") + # not the same drive + assert not check_paths_relative(d, "d:\\yep") + + d = { + "kwargs": { + "path": "\\host\share\yep\\sub\\path1", + } + } + # UNC cannot be relative to d: drive + assert not check_paths_relative(d, "d:\\yep") + + # UNC can be relative to the same UNC + assert check_paths_relative(d, "\\host\share") + + + + # make_paths_relative, From e07bed4a92f6ae35e96e471eee194c321ad0cdf9 Mon Sep 17 00:00:00 2001 From: Samuel Garcia Date: Fri, 1 Dec 2023 12:12:02 +0100 Subject: [PATCH 04/12] Clean. --- src/spikeinterface/core/base.py | 3 +++ src/spikeinterface/core/tests/test_core_tools.py | 12 ------------ 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/src/spikeinterface/core/base.py b/src/spikeinterface/core/base.py index 794f610bd9..d3b81bf8ba 100644 --- a/src/spikeinterface/core/base.py +++ b/src/spikeinterface/core/base.py @@ -322,6 +322,9 @@ def to_dict( to a dictionary. The resulting dictionary can be used to re-initialize the extractor through the `load_extractor_from_dict` function. + In some situations, relative_to is not possible, (for instance differnt drive on window or url like path), then + the relative will ignored and absolut (relative_to=None) will be done instead. + Examples -------- >>> dump_dict = original_extractor.to_dict() diff --git a/src/spikeinterface/core/tests/test_core_tools.py b/src/spikeinterface/core/tests/test_core_tools.py index 99a2c54b61..5051880c53 100644 --- a/src/spikeinterface/core/tests/test_core_tools.py +++ b/src/spikeinterface/core/tests/test_core_tools.py @@ -233,18 +233,6 @@ def test_path_utils_functions(): - - - - # make_paths_relative, - # make_paths_absolute, - # check_paths_relative, - - - - - - if __name__ == "__main__": # Create a temporary folder using the standard library # import tempfile From 9d03d1bdc66158c970826f9a7812632d5a4609b8 Mon Sep 17 00:00:00 2001 From: Garcia Samuel Date: Fri, 1 Dec 2023 14:21:14 +0100 Subject: [PATCH 05/12] Zach is my friend Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com> --- src/spikeinterface/core/base.py | 6 +++--- src/spikeinterface/core/core_tools.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/spikeinterface/core/base.py b/src/spikeinterface/core/base.py index d3b81bf8ba..642757005d 100644 --- a/src/spikeinterface/core/base.py +++ b/src/spikeinterface/core/base.py @@ -322,8 +322,8 @@ def to_dict( to a dictionary. The resulting dictionary can be used to re-initialize the extractor through the `load_extractor_from_dict` function. - In some situations, relative_to is not possible, (for instance differnt drive on window or url like path), then - the relative will ignored and absolut (relative_to=None) will be done instead. + In some situations, 'relative_to' is not possible, (for instance different drives on Windows or url-like path), then + the relative will ignored and absolute (relative_to=None) will be done instead. Examples -------- @@ -421,7 +421,7 @@ def to_dict( # "relative_paths": (relative_to is not None), } - dump_dict["version"] = module_version # Can be spikeinterface, spikefores, etc. + dump_dict["version"] = module_version # Can be spikeinterface, spikeforest, etc. if include_annotations: dump_dict["annotations"] = self._annotations diff --git a/src/spikeinterface/core/core_tools.py b/src/spikeinterface/core/core_tools.py index a81e1382d0..47774312a0 100644 --- a/src/spikeinterface/core/core_tools.py +++ b/src/spikeinterface/core/core_tools.py @@ -876,7 +876,7 @@ def check_paths_relative(input_dict, relative_folder) -> bool: """ Check if relative path is possible to be applied on a dict describing an BaseExtractor. - For instance on windows, if some data are on a driver "d:/" and the folder is on drive "C:/" it returns False. + For instance on windows, if some data are on a drive "D:/" and the folder is on drive "C:/" it returns False. Parameters ---------- @@ -919,7 +919,7 @@ def check_paths_relative(input_dict, relative_folder) -> bool: def make_paths_relative(input_dict, relative_folder) -> dict: """ - Recursivelly transform a dict describing an BaseExtractor to make every path relative to a folder. + Recursively transform a dict describing an BaseExtractor to make every path relative to a folder. Parameters ---------- @@ -941,7 +941,7 @@ def make_paths_relative(input_dict, relative_folder) -> dict: def make_paths_absolute(input_dict, base_folder): """ - Recursivelly transform a dict describing an BaseExtractor to make every path absolut given a base_folder. + Recursively transform a dict describing an BaseExtractor to make every path absolute given a base_folder. Parameters ---------- From f4db6277fd9c0249e3f482476c4ae3354a9c4fdf Mon Sep 17 00:00:00 2001 From: Garcia Samuel Date: Mon, 4 Dec 2023 09:44:32 +0100 Subject: [PATCH 06/12] Update src/spikeinterface/core/tests/test_core_tools.py Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com> --- src/spikeinterface/core/tests/test_core_tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/spikeinterface/core/tests/test_core_tools.py b/src/spikeinterface/core/tests/test_core_tools.py index 5051880c53..2c59312230 100644 --- a/src/spikeinterface/core/tests/test_core_tools.py +++ b/src/spikeinterface/core/tests/test_core_tools.py @@ -229,7 +229,7 @@ def test_path_utils_functions(): assert not check_paths_relative(d, "d:\\yep") # UNC can be relative to the same UNC - assert check_paths_relative(d, "\\host\share") + assert check_paths_relative(d, r"\\host\share") From 283639cb8497e15b42121132498aff5caf4c929a Mon Sep 17 00:00:00 2001 From: Samuel Garcia Date: Mon, 4 Dec 2023 21:58:27 +0100 Subject: [PATCH 07/12] oups --- src/spikeinterface/core/tests/test_core_tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/spikeinterface/core/tests/test_core_tools.py b/src/spikeinterface/core/tests/test_core_tools.py index 2c59312230..d7f5882808 100644 --- a/src/spikeinterface/core/tests/test_core_tools.py +++ b/src/spikeinterface/core/tests/test_core_tools.py @@ -222,7 +222,7 @@ def test_path_utils_functions(): d = { "kwargs": { - "path": "\\host\share\yep\\sub\\path1", + "path": r"\\host\share\yep\\sub\\path1", } } # UNC cannot be relative to d: drive From 9749d161cc8e85fbb91aa270c209263ba3798dd9 Mon Sep 17 00:00:00 2001 From: Samuel Garcia Date: Mon, 4 Dec 2023 22:00:01 +0100 Subject: [PATCH 08/12] Path with windows raw string in tests. --- src/spikeinterface/core/tests/test_core_tools.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/spikeinterface/core/tests/test_core_tools.py b/src/spikeinterface/core/tests/test_core_tools.py index d7f5882808..f72f64df86 100644 --- a/src/spikeinterface/core/tests/test_core_tools.py +++ b/src/spikeinterface/core/tests/test_core_tools.py @@ -199,13 +199,13 @@ def test_path_utils_functions(): # test for windows Path d = { "kwargs": { - "path": "c:\\yep\\sub\\path1", + "path": r"c:\yep\sub\path1", "recording": { "module": "mock_module", "class": "mock_class", "version": "1.2", "annotations": {}, - "kwargs": {"path": "c:\\yep\\sub\\path2"}, + "kwargs": {"path": r"c:\yep\sub\path2"}, }, } } @@ -216,17 +216,17 @@ def test_path_utils_functions(): assert d2["kwargs"]["recording"]["kwargs"]["path"] == "sub/path2" # same drive - assert check_paths_relative(d, "c:\\yep") + assert check_paths_relative(d, r"c:\yep") # not the same drive - assert not check_paths_relative(d, "d:\\yep") + assert not check_paths_relative(d, r"d:\yep") d = { "kwargs": { - "path": r"\\host\share\yep\\sub\\path1", + "path": r"\\host\share\yep\sub\path1", } } # UNC cannot be relative to d: drive - assert not check_paths_relative(d, "d:\\yep") + assert not check_paths_relative(d, r"d:\yep") # UNC can be relative to the same UNC assert check_paths_relative(d, r"\\host\share") From 3e6ddee0faa19d87bd145e0d6d06d51c7314507e Mon Sep 17 00:00:00 2001 From: Samuel Garcia Date: Mon, 4 Dec 2023 22:02:29 +0100 Subject: [PATCH 09/12] Remove an annoying warning. --- src/spikeinterface/core/base.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/spikeinterface/core/base.py b/src/spikeinterface/core/base.py index 642757005d..e7d5914132 100644 --- a/src/spikeinterface/core/base.py +++ b/src/spikeinterface/core/base.py @@ -445,9 +445,10 @@ def to_dict( dump_dict["relative_paths"] = True dump_dict = make_paths_relative(dump_dict, relative_to) else: + # A warning will be very annoying for end user. + # So the swith back to absolut path but silently! + # warnings.warn("Try to BaseExtractor.to_dict() using relative_to but there is no common folder") dump_dict["relative_paths"] = False - # @alessio @ramon @zach: is this warning wanted or not ? - warnings.warn("Try to BaseExtractor.to_dict() using relative_to but there is no common folder") if folder_metadata is not None: From 1e5e746745071f6b027006011a29269fee0fcf74 Mon Sep 17 00:00:00 2001 From: Garcia Samuel Date: Tue, 5 Dec 2023 11:01:21 +0100 Subject: [PATCH 10/12] Thank you Ramon Co-authored-by: Heberto Mayorquin --- src/spikeinterface/core/base.py | 1 - src/spikeinterface/core/core_tools.py | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/spikeinterface/core/base.py b/src/spikeinterface/core/base.py index e7d5914132..2e6f594689 100644 --- a/src/spikeinterface/core/base.py +++ b/src/spikeinterface/core/base.py @@ -418,7 +418,6 @@ def to_dict( "module": module, "kwargs": kwargs, "version": module_version, - # "relative_paths": (relative_to is not None), } dump_dict["version"] = module_version # Can be spikeinterface, spikeforest, etc. diff --git a/src/spikeinterface/core/core_tools.py b/src/spikeinterface/core/core_tools.py index 47774312a0..7aaee69be9 100644 --- a/src/spikeinterface/core/core_tools.py +++ b/src/spikeinterface/core/core_tools.py @@ -866,8 +866,8 @@ def append_to_path(p): def _relative_to(p, relative_folder): # custum os.path.relpath() with more checks - relative_folder = Path(relative_folder).resolve().absolute() - p = Path(p).resolve().absolute() + relative_folder = Path(relative_folder).resolve() + p = Path(p).resolve() # the as_posix transform \\ to / on window which make better json files rel_to = os.path.relpath(p.as_posix(), start=relative_folder.as_posix()) return Path(rel_to).as_posix() From 59cd0839f76f84df6893c78bddae403e672acba1 Mon Sep 17 00:00:00 2001 From: Alessio Buccino Date: Tue, 5 Dec 2023 12:09:43 +0100 Subject: [PATCH 11/12] Apply suggestions from code review Small fixes --- src/spikeinterface/core/base.py | 2 +- src/spikeinterface/core/core_tools.py | 5 ++--- src/spikeinterface/core/tests/test_core_tools.py | 1 - 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/spikeinterface/core/base.py b/src/spikeinterface/core/base.py index 2e6f594689..abaa753c3d 100644 --- a/src/spikeinterface/core/base.py +++ b/src/spikeinterface/core/base.py @@ -445,7 +445,7 @@ def to_dict( dump_dict = make_paths_relative(dump_dict, relative_to) else: # A warning will be very annoying for end user. - # So the swith back to absolut path but silently! + # So let's switch back to absolute path, but silently! # warnings.warn("Try to BaseExtractor.to_dict() using relative_to but there is no common folder") dump_dict["relative_paths"] = False diff --git a/src/spikeinterface/core/core_tools.py b/src/spikeinterface/core/core_tools.py index 7aaee69be9..294c064cc3 100644 --- a/src/spikeinterface/core/core_tools.py +++ b/src/spikeinterface/core/core_tools.py @@ -1,5 +1,4 @@ -from pathlib import Path -import pathlib +from pathlib import Path, WindowsPath from typing import Union import os import sys @@ -900,7 +899,7 @@ def check_paths_relative(input_dict, relative_folder) -> bool: continue # If windows path check have same drive - if isinstance(p, pathlib.WindowsPath) and isinstance(relative_folder, pathlib.WindowsPath): + if isinstance(p, WindowsPath) and isinstance(relative_folder, WindowsPath): # check that on same drive # important note : for UNC path on window the "//host/shared" is the drive if p.resolve().absolute().drive != relative_folder.drive: diff --git a/src/spikeinterface/core/tests/test_core_tools.py b/src/spikeinterface/core/tests/test_core_tools.py index f72f64df86..7566863c8f 100644 --- a/src/spikeinterface/core/tests/test_core_tools.py +++ b/src/spikeinterface/core/tests/test_core_tools.py @@ -2,7 +2,6 @@ from multiprocessing.shared_memory import SharedMemory from pathlib import Path import importlib -import pathlib import pytest import numpy as np From 7db2cb7bd904c384a27443da74bbab1a68a5b1be Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 5 Dec 2023 11:48:37 +0000 Subject: [PATCH 12/12] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/spikeinterface/core/base.py | 14 +++++++++----- src/spikeinterface/core/core_tools.py | 14 ++++++++------ src/spikeinterface/core/tests/test_core_tools.py | 10 ++++------ 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/spikeinterface/core/base.py b/src/spikeinterface/core/base.py index abaa753c3d..ee7671c6ed 100644 --- a/src/spikeinterface/core/base.py +++ b/src/spikeinterface/core/base.py @@ -15,7 +15,14 @@ import numpy as np from .globals import get_global_tmp_folder, is_set_global_tmp_folder -from .core_tools import check_json, is_dict_extractor, SIJsonEncoder, make_paths_relative, make_paths_absolute, check_paths_relative +from .core_tools import ( + check_json, + is_dict_extractor, + SIJsonEncoder, + make_paths_relative, + make_paths_absolute, + check_paths_relative, +) from .job_tools import _shared_job_kwargs_doc @@ -446,10 +453,9 @@ def to_dict( else: # A warning will be very annoying for end user. # So let's switch back to absolute path, but silently! - # warnings.warn("Try to BaseExtractor.to_dict() using relative_to but there is no common folder") + # warnings.warn("Try to BaseExtractor.to_dict() using relative_to but there is no common folder") dump_dict["relative_paths"] = False - if folder_metadata is not None: if relative_to is not None: folder_metadata = Path(folder_metadata).resolve().absolute().relative_to(relative_to) @@ -1022,8 +1028,6 @@ def save_to_zarr( return cached - - def _load_extractor_from_dict(dic) -> BaseExtractor: """ Convert a dictionary into an instance of BaseExtractor or its subclass. diff --git a/src/spikeinterface/core/core_tools.py b/src/spikeinterface/core/core_tools.py index 294c064cc3..442516dd75 100644 --- a/src/spikeinterface/core/core_tools.py +++ b/src/spikeinterface/core/core_tools.py @@ -851,13 +851,14 @@ def recursive_path_modifier(d, func, target="path", copy=True) -> dict: raise ValueError(f"{k} key for path must be str or list[str]") - def _get_paths_list(d): # this explore a dict and get all paths flatten in a list # the trick is to use a closure func called by recursive_path_modifier() path_list = [] + def append_to_path(p): path_list.append(p) + recursive_path_modifier(d, append_to_path, target="path", copy=True) return path_list @@ -867,10 +868,11 @@ def _relative_to(p, relative_folder): relative_folder = Path(relative_folder).resolve() p = Path(p).resolve() - # the as_posix transform \\ to / on window which make better json files + # the as_posix transform \\ to / on window which make better json files rel_to = os.path.relpath(p.as_posix(), start=relative_folder.as_posix()) return Path(rel_to).as_posix() + def check_paths_relative(input_dict, relative_folder) -> bool: """ Check if relative path is possible to be applied on a dict describing an BaseExtractor. @@ -883,7 +885,7 @@ def check_paths_relative(input_dict, relative_folder) -> bool: A dict describing an extactor obtained by BaseExtractor.to_dict() relative_folder: str or Path The folder to be relative to. - + Returns ------- relative_possible: bool @@ -894,7 +896,7 @@ def check_paths_relative(input_dict, relative_folder) -> bool: for p in path_list: p = Path(p) # check path is not an URL - if 'http' in str(p): + if "http" in str(p): not_possible.append(p) continue @@ -905,14 +907,14 @@ def check_paths_relative(input_dict, relative_folder) -> bool: if p.resolve().absolute().drive != relative_folder.drive: not_possible.append(p) continue - + # check relative is possible try: p2 = _relative_to(p, relative_folder) except ValueError: not_possible.append(p) continue - + return len(not_possible) == 0 diff --git a/src/spikeinterface/core/tests/test_core_tools.py b/src/spikeinterface/core/tests/test_core_tools.py index 7566863c8f..006fae5fb8 100644 --- a/src/spikeinterface/core/tests/test_core_tools.py +++ b/src/spikeinterface/core/tests/test_core_tools.py @@ -187,13 +187,12 @@ def test_path_utils_functions(): d3 = make_paths_relative(d, Path("/yep")) assert d3["kwargs"]["path"] == "sub/path1" - assert d3["kwargs"]["recording"]["kwargs"]["path"] == "sub/path2" + assert d3["kwargs"]["recording"]["kwargs"]["path"] == "sub/path2" d4 = make_paths_absolute(d3, "/yop") assert d4["kwargs"]["path"].startswith("/yop") assert d4["kwargs"]["recording"]["kwargs"]["path"].startswith("/yop") - if platform.system() == "Windows": # test for windows Path d = { @@ -212,7 +211,7 @@ def test_path_utils_functions(): d2 = make_paths_relative(d, "c:\\yep") # the str be must unix like path even on windows for more portability assert d2["kwargs"]["path"] == "sub/path1" - assert d2["kwargs"]["recording"]["kwargs"]["path"] == "sub/path2" + assert d2["kwargs"]["recording"]["kwargs"]["path"] == "sub/path2" # same drive assert check_paths_relative(d, r"c:\yep") @@ -231,7 +230,6 @@ def test_path_utils_functions(): assert check_paths_relative(d, r"\\host\share") - if __name__ == "__main__": # Create a temporary folder using the standard library # import tempfile @@ -239,6 +237,6 @@ def test_path_utils_functions(): # with tempfile.TemporaryDirectory() as tmpdirname: # tmp_path = Path(tmpdirname) # test_write_binary_recording(tmp_path) - # test_write_memory_recording() - + # test_write_memory_recording() + test_path_utils_functions()