From d7369126cf40fad019696f569db3037237ab1c32 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 24 Jan 2024 13:19:39 -0800 Subject: [PATCH 1/3] Be consistent about copying and updating dict Sometimes we mess with stuff in __getstate__, e.g. to avoid reflexive relationships between nodes and channels, but getting the state should never modify the current state, so always make a fresh dictionary. Similarly, the returned state might possibly not have all the content our current state does, so use update instead of overwriting. --- pyiron_workflow/channels.py | 4 +--- pyiron_workflow/interfaces.py | 4 ++-- pyiron_workflow/io.py | 2 +- pyiron_workflow/node.py | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/pyiron_workflow/channels.py b/pyiron_workflow/channels.py index b2cde355..16fd3923 100644 --- a/pyiron_workflow/channels.py +++ b/pyiron_workflow/channels.py @@ -729,11 +729,9 @@ def __round__(self): # Because we override __getattr__ we need to get and set state for serialization def __getstate__(self): - return self.__dict__ + return dict(self.__dict__) def __setstate__(self, state): - # Update instead of overriding in case some other attributes were added on the - # main process while a remote process was working away self.__dict__.update(**state) diff --git a/pyiron_workflow/interfaces.py b/pyiron_workflow/interfaces.py index 90e19338..9e30390f 100644 --- a/pyiron_workflow/interfaces.py +++ b/pyiron_workflow/interfaces.py @@ -147,10 +147,10 @@ def __getitem__(self, item): ) from e def __getstate__(self): - return self.__dict__ + return dict(self.__dict__) def __setstate__(self, state): - self.__dict__ = state + self.__dict__.update(**state) def register(self, package_identifier: str, domain: Optional[str] = None) -> None: """ diff --git a/pyiron_workflow/io.py b/pyiron_workflow/io.py index 18ed8d01..f1cf6232 100644 --- a/pyiron_workflow/io.py +++ b/pyiron_workflow/io.py @@ -157,7 +157,7 @@ def to_dict(self): def __getstate__(self): # Compatibility with python <3.11 - return self.__dict__ + return dict(self.__dict__) def __setstate__(self, state): # Because we override getattr, we need to use __dict__ assignment directly in diff --git a/pyiron_workflow/node.py b/pyiron_workflow/node.py index d73cc859..9bc949b1 100644 --- a/pyiron_workflow/node.py +++ b/pyiron_workflow/node.py @@ -1040,7 +1040,7 @@ def replace_with(self, other: Node | type[Node]): warnings.warn(f"Could not replace_node {self.label}, as it has no parent.") def __getstate__(self): - state = self.__dict__ + state = dict(self.__dict__) state["_parent"] = None # I am not at all confident that removing the parent here is the _right_ # solution. From 1ce79ce3d450991fa36c5020b89555888b6a69db Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 24 Jan 2024 13:20:03 -0800 Subject: [PATCH 2/3] Unparent local nodes before taking remote In the case of running on an executor --- pyiron_workflow/composite.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pyiron_workflow/composite.py b/pyiron_workflow/composite.py index a3224745..68a4cc2e 100644 --- a/pyiron_workflow/composite.py +++ b/pyiron_workflow/composite.py @@ -192,6 +192,9 @@ def process_run_result(self, run_output): return DotDict(self.outputs.to_value_dict()) def _parse_remotely_executed_self(self, other_self): + # Un-parent existing nodes before ditching them + for node in self: + node._parent = None self.__setstate__(other_self.__getstate__()) def disconnect_run(self) -> list[tuple[Channel, Channel]]: From 1f3893d294b8ce87bac07b57ab5fc8d737403140 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 24 Jan 2024 13:45:22 -0800 Subject: [PATCH 3/3] :bug: stop running By setting `running=False` on the local instance _before_ updating the state, we were then re-writing `running=True` when updating the state. Update the flag on the received instance as well, and add a test so the same issue doesn't crop up again. --- pyiron_workflow/composite.py | 1 + tests/unit/test_macro.py | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/pyiron_workflow/composite.py b/pyiron_workflow/composite.py index 68a4cc2e..8f4f0cd8 100644 --- a/pyiron_workflow/composite.py +++ b/pyiron_workflow/composite.py @@ -195,6 +195,7 @@ def _parse_remotely_executed_self(self, other_self): # Un-parent existing nodes before ditching them for node in self: node._parent = None + other_self.running = False # It's done now self.__setstate__(other_self.__getstate__()) def disconnect_run(self) -> list[tuple[Channel, Channel]]: diff --git a/tests/unit/test_macro.py b/tests/unit/test_macro.py index afc45ac1..3b9cdcb0 100644 --- a/tests/unit/test_macro.py +++ b/tests/unit/test_macro.py @@ -238,6 +238,10 @@ def test_with_executor(self): returned_nodes = result.result(timeout=120) # Wait for the process to finish sleep(1) + self.assertFalse( + macro.running, + msg="Macro should be done running" + ) self.assertIsNot( original_one, returned_nodes.one,