Allow relative_to with recursive=False (in-place on objects) #1824

alejoe91 · 2023-07-13T14:39:04Z

Might fix #1819

For very long chains, dumping with recursive=True is a performance killer. However, that was required to make paths relative.
This PRs refactors completely the recursive_path_finder function to allow to modify the paths in place on BaseExtractor objects. Note that this is only possible if copy=False and it raises an exception if copy=True

See: https://github.com/alejoe91/spikeinterface/blob/non-recursive-relative-paths/src/spikeinterface/core/core_tools.py#L825-L896

Also added a helper function dict_contains_extractor to check the "status" of a dictionary.
Removed a bunch of d = that do not play well with debugger..

@DradeAW can you test this?

alejoe91 · 2023-07-13T14:42:09Z

An option to make it compatible with copy=True, would be to clone and replace extractor objects in the dictionary...but one thing at a time

alejoe91 · 2023-07-14T10:56:41Z

src/spikeinterface/core/base.py

-    return recursive_path_modifier(d, func, target="path", copy=True)
+def _make_paths_relative(d, relative, copy=True) -> dict:
+    relative = Path(relative).absolute()
+    func = lambda p: os.path.relpath(p, start=relative)


os.path is compatible with pathlib objects

There is an equivalent pathlib function as well:
https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.relative_to

It doesn't work if paths are not direct relatives (i.e. it doesn't find .. locations)

I can attest to that: I had a similar problem a few months back, and the pathlib equivalent did not work, and I had to use os.path.relpath. (link to where I used it: https://github.com/BarbourLab/lussac/blob/9c71ed6ad1303460566bf46bacf22c25ee805fe3/src/lussac/utils/plotting.py#L26)

Thanks.
This is good to know. It is on a footnote on the link the I shared actually:

[PurePath.relative_to()](https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.relative_to) requires self to be the subpath of the argument, but [os.path.relpath()](https://docs.python.org/3/library/os.path.html#os.path.relpath) does not.

Double thanks, guys. I learn a couple of things about paths today. This is an enhancement that will be added in python 3:12, see the argument walk_up in the docs over there:

https://docs.python.org/3.12/library/pathlib.html#pathlib.PurePath.relative_to

The corresponding PR:
python/cpython#19813

h-mayorquin · 2023-07-16T13:03:24Z

src/spikeinterface/core/core_tools.py

    if copy:
-        dc = deepcopy(d)
+        if dict_contains_extractors(dictionary):
+            raise Exception("The copy argument is only available if the input dictionary does not contain objects")


Suggested change

raise Exception("The copy argument is only available if the input dictionary does not contain objects")

raise Exception("The copy argument is only available if the input dictionary does not contain extractor objects")

?

samuelgarcia · 2023-07-17T08:48:27Z

src/spikeinterface/core/base.py

+        skip_recursive_path_modifier_warning: bool
+            If True, skip the warning that is raised when `recursive=True` and `relative_to` is not None.


In which situation we would have recursive=True and relative_to not None ?
I think this option is too much detail and to_dict() is more or less internal.
So I think that either we are strict and we raise an error either we do not care but adding this flag is quite a bit heavy for the API no ?

as you said, to_dict is internal so we can keep it in my opinion

samuelgarcia · 2023-07-17T08:48:49Z

src/spikeinterface/core/base.py

-        try:
-            dump_dict["version"] = imported_module.__version__
-        except AttributeError:
-            dump_dict["version"] = "unknown"
-


Is it moved elsewhere ?

there were 2 of them, now it's at lines: https://github.com/alejoe91/spikeinterface/blob/non-recursive-relative-paths/src/spikeinterface/core/base.py#L370-L373

samuelgarcia · 2023-07-17T08:51:06Z

src/spikeinterface/core/base.py

            assert relative_to.is_dir(), "'relative_to' must be an existing directory"
-            dump_dict = _make_paths_relative(dump_dict, relative_to)
+            copy = False if dict_contains_extractors(dump_dict) else True


I am not sure to follow the logic here. Are we making an inplace modification of extarctors._kwargs ?
If this is the case I think this is not a good idea.
rec.to_dict() should not modify rec itself. Maybe I am missing something.

we are. that is the only way relative_to can work without recursive=True

samuelgarcia · 2023-07-17T08:51:25Z

src/spikeinterface/core/base.py

+        dictionary = self.to_dict(include_annotations=True, include_properties=True)
+        dictionary = deepcopy(dictionary)
+        clone = BaseExtractor.from_dict(dictionary)


I prefer "d"

d doesn't work with the debugger.

samuelgarcia · 2023-07-17T08:52:51Z

src/spikeinterface/core/base.py

-    def dump_to_json(self, file_path: Union[str, Path, None] = None, relative_to=None, folder_metadata=None) -> None:
+    def dump_to_json(
+        self, file_path: Union[str, Path, None] = None, relative_to=None, folder_metadata=None, recursive=False
+    ) -> None:


I do not understand. I think that json should be always reccursive no ?
how could it be not reccurssive ?

indeed it is, but it is handled by the SIJsonEncoder directly

samuelgarcia · 2023-07-17T08:58:16Z

src/spikeinterface/core/core_tools.py

+        else:
+            if not skip_warning:
+                warnings.warn(
+                    "The dictionary contains extractors, so the paths will be modified in-place!" "Use with caution"
+                )


I am not sure to be agree with inplace modification. I think we should raise error or force a copy always.

alejoe91 · 2023-07-17T10:39:19Z

closing this in favor of a simpler patch

Allow relative_to with recursive=False (in-place on objects)

d6888e2

alejoe91 added 3 commits July 13, 2023 19:31

Restore abs paths for dump_to_json and duimp_to_pickle

275b175

Motion: dump json onlu if json serializable

18fe2ff

Add double pass tests

164e452

alejoe91 added the core Changes to core module label Jul 14, 2023

alejoe91 commented Jul 14, 2023

View reviewed changes

alejoe91 added 3 commits July 14, 2023 15:50

Better errors/warnings

af91581

Ensure paths are modified on the same object ids

8646b8d

Add test for pickle failure

fdbb511

h-mayorquin reviewed Jul 16, 2023

View reviewed changes

samuelgarcia reviewed Jul 17, 2023

View reviewed changes

alejoe91 closed this Jul 17, 2023

alejoe91 deleted the non-recursive-relative-paths branch October 17, 2023 08:22

	raise Exception("The copy argument is only available if the input dictionary does not contain objects")
	raise Exception("The copy argument is only available if the input dictionary does not contain extractor objects")

		skip_recursive_path_modifier_warning: bool
		If True, skip the warning that is raised when `recursive=True` and `relative_to` is not None.

Allow relative_to with recursive=False (in-place on objects) #1824

Allow relative_to with recursive=False (in-place on objects) #1824

Uh oh!

Conversation

alejoe91 commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alejoe91 commented Jul 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-mayorquin Jul 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alejoe91 commented Jul 17, 2023

Uh oh!

Uh oh!

alejoe91 commented Jul 13, 2023 •

edited

Loading

h-mayorquin Jul 16, 2023 •

edited

Loading