From 0487e5cab8893c714f929a6b9efbd12162d3e241 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Thu, 9 Jan 2025 16:25:52 -0800 Subject: [PATCH 01/43] Initialize _label to a string Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index de083b878..4f43e81a0 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -36,7 +36,7 @@ class Semantic(UsesState, HasLabel, HasParent, ABC): def __init__( self, label: str, *args, parent: SemanticParent | None = None, **kwargs ): - self._label = None + self._label = "" self._parent = None self._detached_parent_path = None self.label = label From 11716f79bd527a48d0b937b0b8488dd5b2b2cd69 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Thu, 9 Jan 2025 16:32:01 -0800 Subject: [PATCH 02/43] Hint the delimiter Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 4f43e81a0..e509e8ea3 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -31,7 +31,7 @@ class Semantic(UsesState, HasLabel, HasParent, ABC): accessible. """ - semantic_delimiter = "/" + semantic_delimiter: str = "/" def __init__( self, label: str, *args, parent: SemanticParent | None = None, **kwargs From 1d13f1c2d3b842d80c69e3f80e73b943e6610a7d Mon Sep 17 00:00:00 2001 From: liamhuber Date: Thu, 9 Jan 2025 16:38:46 -0800 Subject: [PATCH 03/43] Make SemanticParent a Generic Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 37 +++++++++++++++++++----------- pyiron_workflow/nodes/composite.py | 6 ++++- tests/unit/mixin/test_semantics.py | 22 ++++++++++++++---- 3 files changed, 45 insertions(+), 20 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index e509e8ea3..1dea063b5 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -13,9 +13,10 @@ from __future__ import annotations -from abc import ABC +from abc import ABC, abstractmethod from difflib import get_close_matches from pathlib import Path +from typing import Generic, TypeVar from bidict import bidict @@ -157,7 +158,10 @@ class CyclicPathError(ValueError): """ -class SemanticParent(Semantic, ABC): +ChildType = TypeVar("ChildType", bound=Semantic) + + +class SemanticParent(Semantic, Generic[ChildType], ABC): """ A semantic object with a collection of uniquely-named semantic children. @@ -182,19 +186,24 @@ def __init__( strict_naming: bool = True, **kwargs, ): - self._children = bidict() + self._children: bidict[str, ChildType] = bidict() self.strict_naming = strict_naming super().__init__(*args, label=label, parent=parent, **kwargs) + @classmethod + @abstractmethod + def child_type(cls) -> type[ChildType]: + pass + @property - def children(self) -> bidict[str, Semantic]: + def children(self) -> bidict[str, ChildType]: return self._children @property def child_labels(self) -> tuple[str]: return tuple(child.label for child in self) - def __getattr__(self, key): + def __getattr__(self, key) -> ChildType: try: return self._children[key] except KeyError as key_error: @@ -210,7 +219,7 @@ def __getattr__(self, key): def __iter__(self): return self.children.values().__iter__() - def __len__(self): + def __len__(self) -> int: return len(self.children) def __dir__(self): @@ -218,15 +227,15 @@ def __dir__(self): def add_child( self, - child: Semantic, + child: ChildType, label: str | None = None, strict_naming: bool | None = None, - ) -> Semantic: + ) -> ChildType: """ Add a child, optionally assigning it a new label in the process. Args: - child (Semantic): The child to add. + child (ChildType): The child to add. label (str|None): A (potentially) new label to assign the child. (Default is None, leave the child's label alone.) strict_naming (bool|None): Whether to append a suffix to the label if @@ -234,7 +243,7 @@ def add_child( use the class-level flag.) Returns: - (Semantic): The child being added. + (ChildType): The child being added. Raises: TypeError: When the child is not of an allowed class. @@ -244,9 +253,9 @@ def add_child( `strict_naming` is true. """ - if not isinstance(child, Semantic): + if not isinstance(child, self.child_type()): raise TypeError( - f"{self.label} expected a new child of type {Semantic.__name__} " + f"{self.label} expected a new child of type {self.child_type()} " f"but got {child}" ) @@ -278,7 +287,7 @@ def add_child( return child @staticmethod - def _ensure_path_is_not_cyclic(parent: SemanticParent | None, child: Semantic): + def _ensure_path_is_not_cyclic(parent: SemanticParent | None, child: ChildType): if parent is not None and parent.semantic_path.startswith( child.semantic_path + child.semantic_delimiter ): @@ -404,7 +413,7 @@ class ParentMostError(TypeError): """ -class ParentMost(SemanticParent, ABC): +class ParentMost(SemanticParent[ChildType], ABC): """ A semantic parent that cannot have any other parent. """ diff --git a/pyiron_workflow/nodes/composite.py b/pyiron_workflow/nodes/composite.py index 11d50583b..799e4a697 100644 --- a/pyiron_workflow/nodes/composite.py +++ b/pyiron_workflow/nodes/composite.py @@ -54,7 +54,7 @@ class FailedChildError(RuntimeError): """Raise when one or more child nodes raise exceptions.""" -class Composite(SemanticParent, HasCreator, Node, ABC): +class Composite(SemanticParent[Node], HasCreator, Node, ABC): """ A base class for nodes that have internal graph structure -- i.e. they hold a collection of child nodes and their computation is to execute that graph. @@ -154,6 +154,10 @@ def __init__( **kwargs, ) + @classmethod + def child_type(cls) -> type[Node]: + return Node + def activate_strict_hints(self): super().activate_strict_hints() for node in self: diff --git a/tests/unit/mixin/test_semantics.py b/tests/unit/mixin/test_semantics.py index dd40c5f04..f8a732fad 100644 --- a/tests/unit/mixin/test_semantics.py +++ b/tests/unit/mixin/test_semantics.py @@ -9,12 +9,24 @@ ) +class ConcreteParent(SemanticParent[Semantic]): + @classmethod + def child_type(cls) -> type[Semantic]: + return Semantic + + +class ConcreteParentMost(ParentMost[Semantic]): + @classmethod + def child_type(cls) -> type[Semantic]: + return Semantic + + class TestSemantics(unittest.TestCase): def setUp(self): - self.root = ParentMost("root") + self.root = ConcreteParentMost("root") self.child1 = Semantic("child1", parent=self.root) - self.middle1 = SemanticParent("middle", parent=self.root) - self.middle2 = SemanticParent("middle_sub", parent=self.middle1) + self.middle1 = ConcreteParent("middle", parent=self.root) + self.middle2 = ConcreteParent("middle_sub", parent=self.middle1) self.child2 = Semantic("child2", parent=self.middle2) def test_getattr(self): @@ -62,12 +74,12 @@ def test_parent(self): with self.assertRaises( TypeError, msg=f"{ParentMost.__name__} instances can't have parent" ): - self.root.parent = SemanticParent(label="foo") + self.root.parent = ConcreteParent(label="foo") with self.assertRaises( TypeError, msg=f"{ParentMost.__name__} instances can't be children" ): - some_parent = SemanticParent(label="bar") + some_parent = ConcreteParent(label="bar") some_parent.add_child(self.root) with self.subTest("Cyclicity exceptions"): From 265f6f856b34dc9c657bb77039217c7998ac8543 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Fri, 10 Jan 2025 10:59:14 -0800 Subject: [PATCH 04/43] Purge `ParentMost` If subclasses of `Semantic` want to limit their `parent` attribute beyond the standard requirement that it be a `SemanticParent`, they can handle that by overriding the `parent` setter and getter. The only place this was used was in `Workflow`, and so such handling is now exactly the case. Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 30 ------------------------------ pyiron_workflow/workflow.py | 21 +++++++++++++++++++-- tests/unit/mixin/test_semantics.py | 21 +-------------------- tests/unit/test_workflow.py | 5 ++--- 4 files changed, 22 insertions(+), 55 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 1dea063b5..cc36658d1 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -259,12 +259,6 @@ def add_child( f"but got {child}" ) - if isinstance(child, ParentMost): - raise ParentMostError( - f"{child.label} is {ParentMost.__name__} and may only take None as a " - f"parent but was added as a child to {self.label}" - ) - self._ensure_path_is_not_cyclic(self, child) self._ensure_child_has_no_other_parent(child) @@ -405,27 +399,3 @@ def __setstate__(self, state): # children). So, now return their parent to them: for child in self: child.parent = self - - -class ParentMostError(TypeError): - """ - To be raised when assigning a parent to a parent-most object - """ - - -class ParentMost(SemanticParent[ChildType], ABC): - """ - A semantic parent that cannot have any other parent. - """ - - @property - def parent(self) -> None: - return None - - @parent.setter - def parent(self, new_parent: None): - if new_parent is not None: - raise ParentMostError( - f"{self.label} is {ParentMost.__name__} and may only take None as a " - f"parent but got {type(new_parent)}" - ) diff --git a/pyiron_workflow/workflow.py b/pyiron_workflow/workflow.py index 791e17c80..8a5ddb29c 100644 --- a/pyiron_workflow/workflow.py +++ b/pyiron_workflow/workflow.py @@ -11,7 +11,6 @@ from bidict import bidict from pyiron_workflow.io import Inputs, Outputs -from pyiron_workflow.mixin.semantics import ParentMost from pyiron_workflow.nodes.composite import Composite if TYPE_CHECKING: @@ -20,7 +19,13 @@ from pyiron_workflow.storage import StorageInterface -class Workflow(ParentMost, Composite): +class ParentMostError(TypeError): + """ + To be raised when assigning a parent to a parent-most object + """ + + +class Workflow(Composite): """ Workflows are a dynamic composite node -- i.e. they hold and run a collection of nodes (a subgraph) which can be dynamically modified (adding and removing nodes, @@ -495,3 +500,15 @@ def replace_child( raise e return owned_node + + @property + def parent(self) -> None: + return None + + @parent.setter + def parent(self, new_parent: None): + if new_parent is not None: + raise ParentMostError( + f"{self.label} is a {self.__class__} and may only take None as a " + f"parent but got {type(new_parent)}" + ) diff --git a/tests/unit/mixin/test_semantics.py b/tests/unit/mixin/test_semantics.py index f8a732fad..0b63b94f0 100644 --- a/tests/unit/mixin/test_semantics.py +++ b/tests/unit/mixin/test_semantics.py @@ -3,7 +3,6 @@ from pyiron_workflow.mixin.semantics import ( CyclicPathError, - ParentMost, Semantic, SemanticParent, ) @@ -15,15 +14,9 @@ def child_type(cls) -> type[Semantic]: return Semantic -class ConcreteParentMost(ParentMost[Semantic]): - @classmethod - def child_type(cls) -> type[Semantic]: - return Semantic - - class TestSemantics(unittest.TestCase): def setUp(self): - self.root = ConcreteParentMost("root") + self.root = ConcreteParent("root") self.child1 = Semantic("child1", parent=self.root) self.middle1 = ConcreteParent("middle", parent=self.root) self.middle2 = ConcreteParent("middle_sub", parent=self.middle1) @@ -70,18 +63,6 @@ def test_parent(self): self.assertEqual(self.child1.parent, self.root) self.assertEqual(self.root.parent, None) - with self.subTest(f"{ParentMost.__name__} exceptions"): - with self.assertRaises( - TypeError, msg=f"{ParentMost.__name__} instances can't have parent" - ): - self.root.parent = ConcreteParent(label="foo") - - with self.assertRaises( - TypeError, msg=f"{ParentMost.__name__} instances can't be children" - ): - some_parent = ConcreteParent(label="bar") - some_parent.add_child(self.root) - with self.subTest("Cyclicity exceptions"): with self.assertRaises(CyclicPathError): self.middle1.parent = self.middle2 diff --git a/tests/unit/test_workflow.py b/tests/unit/test_workflow.py index 174013d36..f19032b70 100644 --- a/tests/unit/test_workflow.py +++ b/tests/unit/test_workflow.py @@ -9,9 +9,8 @@ from pyiron_workflow._tests import ensure_tests_in_python_path from pyiron_workflow.channels import NOT_DATA -from pyiron_workflow.mixin.semantics import ParentMostError from pyiron_workflow.storage import TypeNotFoundError, available_backends -from pyiron_workflow.workflow import Workflow +from pyiron_workflow.workflow import ParentMostError, Workflow ensure_tests_in_python_path() @@ -155,7 +154,7 @@ def test_io_map_bijectivity(self): self.assertEqual(3, len(wf.inputs_map), msg="All entries should be stored") self.assertEqual(0, len(wf.inputs), msg="No IO should be left exposed") - def test_is_parentmost(self): + def test_takes_no_parent(self): wf = Workflow("wf") wf2 = Workflow("wf2") From a6654d655234b93f6dafc3f6043387b75401bd94 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Fri, 10 Jan 2025 11:18:03 -0800 Subject: [PATCH 05/43] Update comment Signed-off-by: liamhuber --- pyiron_workflow/nodes/composite.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/pyiron_workflow/nodes/composite.py b/pyiron_workflow/nodes/composite.py index 799e4a697..e3a06cab2 100644 --- a/pyiron_workflow/nodes/composite.py +++ b/pyiron_workflow/nodes/composite.py @@ -424,8 +424,6 @@ def executor_shutdown(self, wait=True, *, cancel_futures=False): def __setattr__(self, key: str, node: Node): if isinstance(node, Composite) and key in ["_parent", "parent"]: # This is an edge case for assigning a node to an attribute - # We either defer to the setter with super, or directly assign the private - # variable (as requested in the setter) super().__setattr__(key, node) elif isinstance(node, Node): self.add_child(node, label=key) From ba1eb27acb88cb6a311323feae1c5b62cd8ec719 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Fri, 10 Jan 2025 11:30:47 -0800 Subject: [PATCH 06/43] Use generic type Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index cc36658d1..ab66afe70 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -342,15 +342,15 @@ def _add_suffix_to_label(self, label): ) return new_label - def remove_child(self, child: Semantic | str) -> Semantic: + def remove_child(self, child: ChildType | str) -> ChildType: if isinstance(child, str): child = self.children.pop(child) - elif isinstance(child, Semantic): + elif isinstance(child, self.child_type()): self.children.inv.pop(child) else: raise TypeError( f"{self.label} expected to remove a child of type str or " - f"{Semantic.__name__} but got {child}" + f"{self.child_type()} but got {child}" ) child.parent = None From 619d93662368496b67892223cbb8caffd25792f4 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Fri, 10 Jan 2025 11:34:07 -0800 Subject: [PATCH 07/43] Don't use generic in static method Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index ab66afe70..ec7725a6f 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -20,6 +20,7 @@ from bidict import bidict +from pyiron_workflow.compatibility import Self from pyiron_workflow.logging import logger from pyiron_workflow.mixin.has_interface_mixins import HasLabel, HasParent, UsesState @@ -281,7 +282,7 @@ def add_child( return child @staticmethod - def _ensure_path_is_not_cyclic(parent: SemanticParent | None, child: ChildType): + def _ensure_path_is_not_cyclic(parent: SemanticParent | None, child: Semantic): if parent is not None and parent.semantic_path.startswith( child.semantic_path + child.semantic_delimiter ): From 39083ef2eec11d392a4a8eb978c888322098fb7a Mon Sep 17 00:00:00 2001 From: liamhuber Date: Fri, 10 Jan 2025 11:48:52 -0800 Subject: [PATCH 08/43] Jump through mypy hoops It doesn't recognize the __set__ for fset methods on the property, so my usual routes for super'ing the setter are failing. This is annoying, but I don't see it being particularly harmful as the method is private. Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index ec7725a6f..3873a1eed 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -63,6 +63,13 @@ def parent(self) -> SemanticParent | None: @parent.setter def parent(self, new_parent: SemanticParent | None) -> None: + self._set_parent(new_parent) + + def _set_parent(self, new_parent: SemanticParent | None): + """ + mypy is uncooperative with super calls for setters, so we pull the behaviour + out. + """ if new_parent is self._parent: # Exit early if nothing is changing return @@ -365,7 +372,7 @@ def parent(self) -> SemanticParent | None: @parent.setter def parent(self, new_parent: SemanticParent | None) -> None: self._ensure_path_is_not_cyclic(new_parent, self) - super(SemanticParent, type(self)).parent.__set__(self, new_parent) + self._set_parent(new_parent) def __getstate__(self): state = super().__getstate__() From 82f8370bee6d1bb302e9082d41dee693361e573f Mon Sep 17 00:00:00 2001 From: liamhuber Date: Fri, 10 Jan 2025 11:49:03 -0800 Subject: [PATCH 09/43] Remove unused import Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 3873a1eed..31c5f19cb 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -20,7 +20,6 @@ from bidict import bidict -from pyiron_workflow.compatibility import Self from pyiron_workflow.logging import logger from pyiron_workflow.mixin.has_interface_mixins import HasLabel, HasParent, UsesState From 24947fa619f95b2e35651084f3552333d72b0f94 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Mon, 13 Jan 2025 17:28:51 -0800 Subject: [PATCH 10/43] Add dev note Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 31c5f19cb..e207ab920 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -200,6 +200,11 @@ def __init__( @classmethod @abstractmethod def child_type(cls) -> type[ChildType]: + # Dev note: In principle, this could be a regular attribute + # However, in other situations this is precluded (e.g. in channels) + # since it would result in circular references. + # Here we favour consistency over brevity, + # and maintain the X_type() class method pattern pass @property From c94e7e6e523609960c1f523cee21828165fb7eaa Mon Sep 17 00:00:00 2001 From: liamhuber Date: Mon, 13 Jan 2025 13:09:53 -0800 Subject: [PATCH 11/43] Remove HasParent An interface class guaranteeing the (Any-typed) attribute is too vague to be super useful, and redundant when it's _only_ used in `Semantic`. Having a `parent` will just be a direct feature of being semantic. Signed-off-by: liamhuber --- pyiron_workflow/mixin/has_interface_mixins.py | 11 ----------- pyiron_workflow/mixin/semantics.py | 4 ++-- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/pyiron_workflow/mixin/has_interface_mixins.py b/pyiron_workflow/mixin/has_interface_mixins.py index 2828ce7ef..7602b454a 100644 --- a/pyiron_workflow/mixin/has_interface_mixins.py +++ b/pyiron_workflow/mixin/has_interface_mixins.py @@ -53,17 +53,6 @@ def full_label(self) -> str: return self.label -class HasParent(ABC): - """ - A mixin to guarantee the parent interface exists. - """ - - @property - @abstractmethod - def parent(self) -> Any: - """A parent for the object.""" - - class HasChannel(ABC): """ A mix-in class for use with the :class:`Channel` class. diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index e207ab920..55a3aedb0 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -21,10 +21,10 @@ from bidict import bidict from pyiron_workflow.logging import logger -from pyiron_workflow.mixin.has_interface_mixins import HasLabel, HasParent, UsesState +from pyiron_workflow.mixin.has_interface_mixins import HasLabel, UsesState -class Semantic(UsesState, HasLabel, HasParent, ABC): +class Semantic(UsesState, HasLabel, ABC): """ An object with a unique semantic path. From 94f11ca1b0947837301f2efce5ad6f6fa86358cf Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 15:33:20 -0800 Subject: [PATCH 12/43] Pull out static method Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 55a3aedb0..b0caaf431 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -271,7 +271,7 @@ def add_child( f"but got {child}" ) - self._ensure_path_is_not_cyclic(self, child) + _ensure_path_is_not_cyclic(self, child) self._ensure_child_has_no_other_parent(child) @@ -292,18 +292,6 @@ def add_child( child.parent = self return child - @staticmethod - def _ensure_path_is_not_cyclic(parent: SemanticParent | None, child: Semantic): - if parent is not None and parent.semantic_path.startswith( - child.semantic_path + child.semantic_delimiter - ): - raise CyclicPathError( - f"{parent.label} cannot be the parent of {child.label}, because its " - f"semantic path is already in {child.label}'s path and cyclic paths " - f"are not allowed. (i.e. {child.semantic_path} is in " - f"{parent.semantic_path})" - ) - def _ensure_child_has_no_other_parent(self, child: Semantic): if child.parent is not None and child.parent is not self: raise ValueError( @@ -375,7 +363,7 @@ def parent(self) -> SemanticParent | None: @parent.setter def parent(self, new_parent: SemanticParent | None) -> None: - self._ensure_path_is_not_cyclic(new_parent, self) + _ensure_path_is_not_cyclic(new_parent, self) self._set_parent(new_parent) def __getstate__(self): @@ -411,3 +399,15 @@ def __setstate__(self, state): # children). So, now return their parent to them: for child in self: child.parent = self + + +def _ensure_path_is_not_cyclic(parent, child: Semantic): + if isinstance(parent, Semantic) and parent.semantic_path.startswith( + child.semantic_path + child.semantic_delimiter + ): + raise CyclicPathError( + f"{parent.label} cannot be the parent of {child.label}, because its " + f"semantic path is already in {child.label}'s path and cyclic paths " + f"are not allowed. (i.e. {child.semantic_path} is in " + f"{parent.semantic_path})" + ) \ No newline at end of file From 52bdd79d6a95427db3054cd3baa2d708645f5496 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 15:36:39 -0800 Subject: [PATCH 13/43] Pull cyclicity check up to Semantic Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index b0caaf431..d057339de 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -79,6 +79,8 @@ def _set_parent(self, new_parent: SemanticParent | None): f"{self.label}, but got {new_parent}" ) + _ensure_path_is_not_cyclic(new_parent, self) + if ( self._parent is not None and new_parent is not self._parent @@ -357,15 +359,6 @@ def remove_child(self, child: ChildType | str) -> ChildType: return child - @property - def parent(self) -> SemanticParent | None: - return self._parent - - @parent.setter - def parent(self, new_parent: SemanticParent | None) -> None: - _ensure_path_is_not_cyclic(new_parent, self) - self._set_parent(new_parent) - def __getstate__(self): state = super().__getstate__() From 6c234ae1601c124e921cf756a479d003f44eeace Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 15:52:41 -0800 Subject: [PATCH 14/43] De-parent SemanticParent from Semantic Because of the label arg vs kwarg problem, there is still a vestigial label arg in the SemanticParent init signature. Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 7 +++---- tests/unit/mixin/test_semantics.py | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index d057339de..2b50d693a 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -170,7 +170,7 @@ class CyclicPathError(ValueError): ChildType = TypeVar("ChildType", bound=Semantic) -class SemanticParent(Semantic, Generic[ChildType], ABC): +class SemanticParent(Generic[ChildType], ABC): """ A semantic object with a collection of uniquely-named semantic children. @@ -189,15 +189,14 @@ class SemanticParent(Semantic, Generic[ChildType], ABC): def __init__( self, - label: str, + label: str | None, # Vestigial while the label order is broken *args, - parent: SemanticParent | None = None, strict_naming: bool = True, **kwargs, ): self._children: bidict[str, ChildType] = bidict() self.strict_naming = strict_naming - super().__init__(*args, label=label, parent=parent, **kwargs) + super().__init__(*args, label=label, **kwargs) @classmethod @abstractmethod diff --git a/tests/unit/mixin/test_semantics.py b/tests/unit/mixin/test_semantics.py index 0b63b94f0..ab6212852 100644 --- a/tests/unit/mixin/test_semantics.py +++ b/tests/unit/mixin/test_semantics.py @@ -8,7 +8,7 @@ ) -class ConcreteParent(SemanticParent[Semantic]): +class ConcreteParent(SemanticParent[Semantic], Semantic): @classmethod def child_type(cls) -> type[Semantic]: return Semantic From ff25496aad961c905e11cc5faf8d57ed31a6a038 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:27:45 -0800 Subject: [PATCH 15/43] Remove redundant type check This is handled in the super class Signed-off-by: liamhuber --- pyiron_workflow/nodes/composite.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pyiron_workflow/nodes/composite.py b/pyiron_workflow/nodes/composite.py index e3a06cab2..e5e05ed4c 100644 --- a/pyiron_workflow/nodes/composite.py +++ b/pyiron_workflow/nodes/composite.py @@ -304,11 +304,6 @@ def add_child( label: str | None = None, strict_naming: bool | None = None, ) -> Node: - if not isinstance(child, Node): - raise TypeError( - f"Only new {Node.__name__} instances may be added, but got " - f"{type(child)}." - ) self._cached_inputs = None # Reset cache after graph change return super().add_child(child, label=label, strict_naming=strict_naming) From 260714a44421b67ec5c9547cda18e0fe1ecd404d Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:40:17 -0800 Subject: [PATCH 16/43] Give Semantic a generic parent type Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 27 ++++++++++++++++++-------- pyiron_workflow/node.py | 9 +++++++-- tests/unit/mixin/test_semantics.py | 31 +++++++++++++++++++----------- 3 files changed, 46 insertions(+), 21 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 2b50d693a..837cef44d 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -24,7 +24,10 @@ from pyiron_workflow.mixin.has_interface_mixins import HasLabel, UsesState -class Semantic(UsesState, HasLabel, ABC): +ParentType = TypeVar("ParentType", bound="SemanticParent") + + +class Semantic(UsesState, HasLabel, Generic[ParentType], ABC): """ An object with a unique semantic path. @@ -35,7 +38,7 @@ class Semantic(UsesState, HasLabel, ABC): semantic_delimiter: str = "/" def __init__( - self, label: str, *args, parent: SemanticParent | None = None, **kwargs + self, label: str, *args, parent: ParentType | None = None, **kwargs ): self._label = "" self._parent = None @@ -44,6 +47,11 @@ def __init__( self.parent = parent super().__init__(*args, **kwargs) + @classmethod + @abstractmethod + def parent_type(cls) -> type[ParentType]: + pass + @property def label(self) -> str: return self._label @@ -57,14 +65,14 @@ def label(self, new_label: str) -> None: self._label = new_label @property - def parent(self) -> SemanticParent | None: + def parent(self) -> ParentType | None: return self._parent @parent.setter - def parent(self, new_parent: SemanticParent | None) -> None: + def parent(self, new_parent: ParentType | None) -> None: self._set_parent(new_parent) - def _set_parent(self, new_parent: SemanticParent | None): + def _set_parent(self, new_parent: ParentType | None): """ mypy is uncooperative with super calls for setters, so we pull the behaviour out. @@ -73,9 +81,9 @@ def _set_parent(self, new_parent: SemanticParent | None): # Exit early if nothing is changing return - if new_parent is not None and not isinstance(new_parent, SemanticParent): + if new_parent is not None and not isinstance(new_parent, self.parent_type()): raise ValueError( - f"Expected None or a {SemanticParent.__name__} for the parent of " + f"Expected None or a {self.parent_type()} for the parent of " f"{self.label}, but got {new_parent}" ) @@ -136,7 +144,10 @@ def full_label(self) -> str: @property def semantic_root(self) -> Semantic: """The parent-most object in this semantic path; may be self.""" - return self.parent.semantic_root if isinstance(self.parent, Semantic) else self + if isinstance(self.parent, Semantic): + return self.parent.semantic_root + else: + return self def as_path(self, root: Path | str | None = None) -> Path: """ diff --git a/pyiron_workflow/node.py b/pyiron_workflow/node.py index 3b86a5e45..fa8023d1a 100644 --- a/pyiron_workflow/node.py +++ b/pyiron_workflow/node.py @@ -21,7 +21,7 @@ from pyiron_workflow.logging import logger from pyiron_workflow.mixin.injection import HasIOWithInjection from pyiron_workflow.mixin.run import ReadinessError, Runnable -from pyiron_workflow.mixin.semantics import Semantic +from pyiron_workflow.mixin.semantics import Semantic, ParentType from pyiron_workflow.mixin.single_output import ExploitsSingleOutput from pyiron_workflow.storage import StorageInterface, available_backends from pyiron_workflow.topology import ( @@ -41,7 +41,7 @@ class Node( HasIOWithInjection, - Semantic, + Semantic["Composite"], Runnable, ExploitsSingleOutput, ABC, @@ -319,6 +319,11 @@ def __init__( **kwargs, ) + @classmethod + def parent_type(cls) -> type[Composite]: + from pyiron_workflow.nodes.composite import Composite + return Composite + def _setup_node(self) -> None: """ Called _before_ :meth:`Node.__init__` finishes. diff --git a/tests/unit/mixin/test_semantics.py b/tests/unit/mixin/test_semantics.py index ab6212852..9d74da7fe 100644 --- a/tests/unit/mixin/test_semantics.py +++ b/tests/unit/mixin/test_semantics.py @@ -1,26 +1,34 @@ +from __future__ import annotations + import unittest from pathlib import Path from pyiron_workflow.mixin.semantics import ( CyclicPathError, Semantic, - SemanticParent, + SemanticParent, ParentType, ) -class ConcreteParent(SemanticParent[Semantic], Semantic): +class ConcreteSemantic(Semantic["ConcreteParent"]): + @classmethod + def parent_type(cls) -> type[ConcreteParent]: + return ConcreteParent + + +class ConcreteParent(SemanticParent[ConcreteSemantic], ConcreteSemantic): @classmethod - def child_type(cls) -> type[Semantic]: - return Semantic + def child_type(cls) -> type[ConcreteSemantic]: + return ConcreteSemantic class TestSemantics(unittest.TestCase): def setUp(self): self.root = ConcreteParent("root") - self.child1 = Semantic("child1", parent=self.root) + self.child1 = ConcreteSemantic("child1", parent=self.root) self.middle1 = ConcreteParent("middle", parent=self.root) self.middle2 = ConcreteParent("middle_sub", parent=self.middle1) - self.child2 = Semantic("child2", parent=self.middle2) + self.child2 = ConcreteSemantic("child2", parent=self.middle2) def test_getattr(self): with self.assertRaises(AttributeError) as context: @@ -40,18 +48,19 @@ def test_getattr(self): def test_label_validity(self): with self.assertRaises(TypeError, msg="Label must be a string"): - Semantic(label=123) + ConcreteSemantic(label=123) def test_label_delimiter(self): with self.assertRaises( - ValueError, msg=f"Delimiter '{Semantic.semantic_delimiter}' not allowed" + ValueError, + msg=f"Delimiter '{ConcreteSemantic.semantic_delimiter}' not allowed" ): - Semantic(f"invalid{Semantic.semantic_delimiter}label") + ConcreteSemantic(f"invalid{ConcreteSemantic.semantic_delimiter}label") def test_semantic_delimiter(self): self.assertEqual( "/", - Semantic.semantic_delimiter, + ConcreteSemantic.semantic_delimiter, msg="This is just a hard-code to the current value, update it freely so " "the test passes; if it fails it's just a reminder that your change is " "not backwards compatible, and the next release number should reflect " @@ -105,7 +114,7 @@ def test_as_path(self): ) def test_detached_parent_path(self): - orphan = Semantic("orphan") + orphan = ConcreteSemantic("orphan") orphan.__setstate__(self.child2.__getstate__()) self.assertIsNone( orphan.parent, msg="We still should not explicitly have a parent" From 3221cb990f8dd9e95feef1878834d6081514ba5f Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:45:07 -0800 Subject: [PATCH 17/43] Remove unused import Signed-off-by: liamhuber --- pyiron_workflow/node.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyiron_workflow/node.py b/pyiron_workflow/node.py index fa8023d1a..efcb9f2e7 100644 --- a/pyiron_workflow/node.py +++ b/pyiron_workflow/node.py @@ -21,7 +21,7 @@ from pyiron_workflow.logging import logger from pyiron_workflow.mixin.injection import HasIOWithInjection from pyiron_workflow.mixin.run import ReadinessError, Runnable -from pyiron_workflow.mixin.semantics import Semantic, ParentType +from pyiron_workflow.mixin.semantics import Semantic from pyiron_workflow.mixin.single_output import ExploitsSingleOutput from pyiron_workflow.storage import StorageInterface, available_backends from pyiron_workflow.topology import ( From 59a883d90ddaafb223a7e29f844593841170ecf6 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:46:02 -0800 Subject: [PATCH 18/43] Black Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 6 ++---- pyiron_workflow/node.py | 1 + tests/unit/mixin/test_semantics.py | 4 ++-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 837cef44d..ba7289b1c 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -37,9 +37,7 @@ class Semantic(UsesState, HasLabel, Generic[ParentType], ABC): semantic_delimiter: str = "/" - def __init__( - self, label: str, *args, parent: ParentType | None = None, **kwargs - ): + def __init__(self, label: str, *args, parent: ParentType | None = None, **kwargs): self._label = "" self._parent = None self._detached_parent_path = None @@ -413,4 +411,4 @@ def _ensure_path_is_not_cyclic(parent, child: Semantic): f"semantic path is already in {child.label}'s path and cyclic paths " f"are not allowed. (i.e. {child.semantic_path} is in " f"{parent.semantic_path})" - ) \ No newline at end of file + ) diff --git a/pyiron_workflow/node.py b/pyiron_workflow/node.py index efcb9f2e7..6e19a7042 100644 --- a/pyiron_workflow/node.py +++ b/pyiron_workflow/node.py @@ -322,6 +322,7 @@ def __init__( @classmethod def parent_type(cls) -> type[Composite]: from pyiron_workflow.nodes.composite import Composite + return Composite def _setup_node(self) -> None: diff --git a/tests/unit/mixin/test_semantics.py b/tests/unit/mixin/test_semantics.py index 9d74da7fe..874928f79 100644 --- a/tests/unit/mixin/test_semantics.py +++ b/tests/unit/mixin/test_semantics.py @@ -6,7 +6,7 @@ from pyiron_workflow.mixin.semantics import ( CyclicPathError, Semantic, - SemanticParent, ParentType, + SemanticParent, ) @@ -53,7 +53,7 @@ def test_label_validity(self): def test_label_delimiter(self): with self.assertRaises( ValueError, - msg=f"Delimiter '{ConcreteSemantic.semantic_delimiter}' not allowed" + msg=f"Delimiter '{ConcreteSemantic.semantic_delimiter}' not allowed", ): ConcreteSemantic(f"invalid{ConcreteSemantic.semantic_delimiter}label") From 48c86c44fb6fc7fcf942795c225c38e7874d04f6 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:46:28 -0800 Subject: [PATCH 19/43] Ruff sort imports Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index ba7289b1c..dca7bea2f 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -23,7 +23,6 @@ from pyiron_workflow.logging import logger from pyiron_workflow.mixin.has_interface_mixins import HasLabel, UsesState - ParentType = TypeVar("ParentType", bound="SemanticParent") From 35c5a5880536d7c8539940d632002c4cbcb1d2fc Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:54:15 -0800 Subject: [PATCH 20/43] Remove unused import Signed-off-by: liamhuber --- pyiron_workflow/mixin/has_interface_mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyiron_workflow/mixin/has_interface_mixins.py b/pyiron_workflow/mixin/has_interface_mixins.py index 7602b454a..729431761 100644 --- a/pyiron_workflow/mixin/has_interface_mixins.py +++ b/pyiron_workflow/mixin/has_interface_mixins.py @@ -11,7 +11,7 @@ from __future__ import annotations from abc import ABC, abstractmethod -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING if TYPE_CHECKING: from pyiron_workflow.channels import Channel From 72b87c4bef9da59d563c9072337aa1fe344927d2 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:59:28 -0800 Subject: [PATCH 21/43] Update docstrings Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index dca7bea2f..8c1ef6d4a 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -2,10 +2,11 @@ Classes for "semantic" reasoning. The motivation here is to be able to provide the object with a unique identifier -in the context of other semantic objects. Each object may have exactly one parent -and an arbitrary number of children, and each child's name must be unique in the -scope of that parent. In this way, the path from the parent-most object to any -child is completely unique. The typical filesystem on a computer is an excellent +in the context of other semantic objects. Each object may have at most one parent, +while semantic parents may have an arbitrary number of children, and each child's name +must be unique in the scope of that parent. In this way, when semantic parents are also +themselves semantic, we can build a path from the parent-most object to any child that +is completely unique. The typical filesystem on a computer is an excellent example and fulfills our requirements, the only reason we depart from it is so that we are free to have objects stored in different locations (possibly even on totally different drives or machines) belong to the same semantic group. @@ -180,7 +181,7 @@ class CyclicPathError(ValueError): class SemanticParent(Generic[ChildType], ABC): """ - A semantic object with a collection of uniquely-named semantic children. + An with a collection of uniquely-named semantic children. Children should be added or removed via the :meth:`add_child` and :meth:`remove_child` methods and _not_ by direct manipulation of the From a633dbfde6b63d6b6d39fd74106b1cef577d2f83 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 15 Jan 2025 12:32:39 -0800 Subject: [PATCH 22/43] Guarantee that semantic parents have a label Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 8c1ef6d4a..90cc57f0d 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -179,9 +179,9 @@ class CyclicPathError(ValueError): ChildType = TypeVar("ChildType", bound=Semantic) -class SemanticParent(Generic[ChildType], ABC): +class SemanticParent(HasLabel, Generic[ChildType], ABC): """ - An with a collection of uniquely-named semantic children. + A labeled object with a collection of uniquely-named semantic children. Children should be added or removed via the :meth:`add_child` and :meth:`remove_child` methods and _not_ by direct manipulation of the From 1d20d080d19b662e7481c44238fa0846c79965b4 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 15 Jan 2025 12:33:48 -0800 Subject: [PATCH 23/43] :bug: don't assume parents have semantic_path But we can now safely assume they have a label Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 90cc57f0d..952c0ff25 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -104,12 +104,16 @@ def semantic_path(self) -> str: The path of node labels from the graph root (parent-most node) down to this node. """ + prefix: str if self.parent is None and self.detached_parent_path is None: prefix = "" elif self.parent is None and self.detached_parent_path is not None: prefix = self.detached_parent_path elif self.parent is not None and self.detached_parent_path is None: - prefix = self.parent.semantic_path + if isinstance(self.parent, Semantic): + prefix = self.parent.semantic_path + else: + prefix = self.semantic_delimiter + self.parent.label else: raise ValueError( f"The parent and detached path should not be able to take non-None " From 799ccb242e785615ef2e7f736f965a75878501e0 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 15 Jan 2025 13:00:11 -0800 Subject: [PATCH 24/43] Pull label default up into Semantic This way it is allowed to be a keyword argument everywhere, except for Workflow which makes it positional and adjusts its `super().__init__` call accordingly. Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 13 +++++++++---- pyiron_workflow/node.py | 5 +---- pyiron_workflow/nodes/composite.py | 2 +- tests/unit/mixin/test_semantics.py | 14 +++++++------- 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 952c0ff25..db0d1a313 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -37,11 +37,17 @@ class Semantic(UsesState, HasLabel, Generic[ParentType], ABC): semantic_delimiter: str = "/" - def __init__(self, label: str, *args, parent: ParentType | None = None, **kwargs): + def __init__( + self, + *args, + label: str | None = None, + parent: ParentType | None = None, + **kwargs + ): self._label = "" self._parent = None self._detached_parent_path = None - self.label = label + self.label = self.__class__.__name__ if label is None else label self.parent = parent super().__init__(*args, **kwargs) @@ -202,14 +208,13 @@ class SemanticParent(HasLabel, Generic[ChildType], ABC): def __init__( self, - label: str | None, # Vestigial while the label order is broken *args, strict_naming: bool = True, **kwargs, ): self._children: bidict[str, ChildType] = bidict() self.strict_naming = strict_naming - super().__init__(*args, label=label, **kwargs) + super().__init__(*args, **kwargs) @classmethod @abstractmethod diff --git a/pyiron_workflow/node.py b/pyiron_workflow/node.py index 6e19a7042..ead473f74 100644 --- a/pyiron_workflow/node.py +++ b/pyiron_workflow/node.py @@ -297,10 +297,7 @@ def __init__( **kwargs: Interpreted as node input data, with keys corresponding to channel labels. """ - super().__init__( - label=self.__class__.__name__ if label is None else label, - parent=parent, - ) + super().__init__(label=label, parent=parent) self.checkpoint = checkpoint self.recovery: Literal["pickle"] | StorageInterface | None = "pickle" self._serialize_result = False # Advertised, but private to indicate diff --git a/pyiron_workflow/nodes/composite.py b/pyiron_workflow/nodes/composite.py index e5e05ed4c..1689da924 100644 --- a/pyiron_workflow/nodes/composite.py +++ b/pyiron_workflow/nodes/composite.py @@ -143,8 +143,8 @@ def __init__( # empty but the running_children list is not super().__init__( - label, *args, + label=label, parent=parent, delete_existing_savefiles=delete_existing_savefiles, autoload=autoload, diff --git a/tests/unit/mixin/test_semantics.py b/tests/unit/mixin/test_semantics.py index 874928f79..2ba39c4f2 100644 --- a/tests/unit/mixin/test_semantics.py +++ b/tests/unit/mixin/test_semantics.py @@ -24,11 +24,11 @@ def child_type(cls) -> type[ConcreteSemantic]: class TestSemantics(unittest.TestCase): def setUp(self): - self.root = ConcreteParent("root") - self.child1 = ConcreteSemantic("child1", parent=self.root) - self.middle1 = ConcreteParent("middle", parent=self.root) - self.middle2 = ConcreteParent("middle_sub", parent=self.middle1) - self.child2 = ConcreteSemantic("child2", parent=self.middle2) + self.root = ConcreteParent(label="root") + self.child1 = ConcreteSemantic(label="child1", parent=self.root) + self.middle1 = ConcreteParent(label="middle", parent=self.root) + self.middle2 = ConcreteParent(label="middle_sub", parent=self.middle1) + self.child2 = ConcreteSemantic(label="child2", parent=self.middle2) def test_getattr(self): with self.assertRaises(AttributeError) as context: @@ -55,7 +55,7 @@ def test_label_delimiter(self): ValueError, msg=f"Delimiter '{ConcreteSemantic.semantic_delimiter}' not allowed", ): - ConcreteSemantic(f"invalid{ConcreteSemantic.semantic_delimiter}label") + ConcreteSemantic(label=f"invalid{ConcreteSemantic.semantic_delimiter}label") def test_semantic_delimiter(self): self.assertEqual( @@ -114,7 +114,7 @@ def test_as_path(self): ) def test_detached_parent_path(self): - orphan = ConcreteSemantic("orphan") + orphan = ConcreteSemantic(label="orphan") orphan.__setstate__(self.child2.__getstate__()) self.assertIsNone( orphan.parent, msg="We still should not explicitly have a parent" From 7ed0a3843863f61a6a09e8857a3bdbb7beb402b1 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 15 Jan 2025 13:37:42 -0800 Subject: [PATCH 25/43] Refactor: label validity check Pull it up from semantic into an extensible method on the mixin class Signed-off-by: liamhuber --- pyiron_workflow/channels.py | 4 ---- pyiron_workflow/mixin/has_interface_mixins.py | 15 ++++++++++++++- pyiron_workflow/mixin/semantics.py | 11 ++--------- tests/unit/mixin/test_run.py | 4 +--- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/pyiron_workflow/channels.py b/pyiron_workflow/channels.py index 829708785..153932fd5 100644 --- a/pyiron_workflow/channels.py +++ b/pyiron_workflow/channels.py @@ -92,10 +92,6 @@ def __init__( self.owner: HasIO = owner self.connections: list[ConjugateType] = [] - @property - def label(self) -> str: - return self._label - @abstractmethod def __str__(self): pass diff --git a/pyiron_workflow/mixin/has_interface_mixins.py b/pyiron_workflow/mixin/has_interface_mixins.py index 729431761..0b0aad36d 100644 --- a/pyiron_workflow/mixin/has_interface_mixins.py +++ b/pyiron_workflow/mixin/has_interface_mixins.py @@ -38,11 +38,24 @@ class HasLabel(ABC): """ A mixin to guarantee the label interface exists. """ + _label: str @property - @abstractmethod def label(self) -> str: """A label for the object.""" + return self._label + + @label.setter + def label(self, new_label: str): + self._check_label(new_label) + self._label = new_label + + def _check_label(self, new_label: str) -> None: + """ + Extensible checking routine for label validity. + """ + if not isinstance(new_label, str): + raise TypeError(f"Expected a string label but got {new_label}") @property def full_label(self) -> str: diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index db0d1a313..98523f14d 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -56,17 +56,10 @@ def __init__( def parent_type(cls) -> type[ParentType]: pass - @property - def label(self) -> str: - return self._label - - @label.setter - def label(self, new_label: str) -> None: - if not isinstance(new_label, str): - raise TypeError(f"Expected a string label but got {new_label}") + def _check_label(self, new_label: str) -> None: + super()._check_label(new_label) if self.semantic_delimiter in new_label: raise ValueError(f"{self.semantic_delimiter} cannot be in the label") - self._label = new_label @property def parent(self) -> ParentType | None: diff --git a/tests/unit/mixin/test_run.py b/tests/unit/mixin/test_run.py index 009e03b55..bfa32ece4 100644 --- a/tests/unit/mixin/test_run.py +++ b/tests/unit/mixin/test_run.py @@ -8,9 +8,7 @@ class ConcreteRunnable(Runnable): - @property - def label(self) -> str: - return "child_class_with_all_methods_implemented" + _label = "child_class_with_all_methods_implemented" def on_run(self, **kwargs): return kwargs From 28b8a3295d24e33a61836a73c18414493325d918 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 15 Jan 2025 13:41:05 -0800 Subject: [PATCH 26/43] Refactor: rename class Signed-off-by: liamhuber --- tests/unit/mixin/test_semantics.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/unit/mixin/test_semantics.py b/tests/unit/mixin/test_semantics.py index 2ba39c4f2..d1a8a204a 100644 --- a/tests/unit/mixin/test_semantics.py +++ b/tests/unit/mixin/test_semantics.py @@ -12,11 +12,11 @@ class ConcreteSemantic(Semantic["ConcreteParent"]): @classmethod - def parent_type(cls) -> type[ConcreteParent]: - return ConcreteParent + def parent_type(cls) -> type[ConcreteSemanticParent]: + return ConcreteSemanticParent -class ConcreteParent(SemanticParent[ConcreteSemantic], ConcreteSemantic): +class ConcreteSemanticParent(SemanticParent[ConcreteSemantic], ConcreteSemantic): @classmethod def child_type(cls) -> type[ConcreteSemantic]: return ConcreteSemantic @@ -24,10 +24,10 @@ def child_type(cls) -> type[ConcreteSemantic]: class TestSemantics(unittest.TestCase): def setUp(self): - self.root = ConcreteParent(label="root") + self.root = ConcreteSemanticParent(label="root") self.child1 = ConcreteSemantic(label="child1", parent=self.root) - self.middle1 = ConcreteParent(label="middle", parent=self.root) - self.middle2 = ConcreteParent(label="middle_sub", parent=self.middle1) + self.middle1 = ConcreteSemanticParent(label="middle", parent=self.root) + self.middle2 = ConcreteSemanticParent(label="middle_sub", parent=self.middle1) self.child2 = ConcreteSemantic(label="child2", parent=self.middle2) def test_getattr(self): From 62c1da711d0044cd4b6d0aee449cdc9d5b55de23 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 15 Jan 2025 13:46:26 -0800 Subject: [PATCH 27/43] Add label restrictions To semantic parent based on its child type's semantic delimiter Signed-off-by: liamhuber --- pyiron_workflow/mixin/has_interface_mixins.py | 1 + pyiron_workflow/mixin/semantics.py | 23 +++++++++++++------ tests/unit/mixin/test_semantics.py | 15 +++++++++++- 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/pyiron_workflow/mixin/has_interface_mixins.py b/pyiron_workflow/mixin/has_interface_mixins.py index 0b0aad36d..5183c83e0 100644 --- a/pyiron_workflow/mixin/has_interface_mixins.py +++ b/pyiron_workflow/mixin/has_interface_mixins.py @@ -38,6 +38,7 @@ class HasLabel(ABC): """ A mixin to guarantee the label interface exists. """ + _label: str @property diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 98523f14d..eb25c682f 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -17,7 +17,7 @@ from abc import ABC, abstractmethod from difflib import get_close_matches from pathlib import Path -from typing import Generic, TypeVar +from typing import ClassVar, Generic, TypeVar from bidict import bidict @@ -35,14 +35,14 @@ class Semantic(UsesState, HasLabel, Generic[ParentType], ABC): accessible. """ - semantic_delimiter: str = "/" + semantic_delimiter: ClassVar[str] = "/" def __init__( - self, - *args, - label: str | None = None, - parent: ParentType | None = None, - **kwargs + self, + *args, + label: str | None = None, + parent: ParentType | None = None, + **kwargs, ): self._label = "" self._parent = None @@ -227,6 +227,15 @@ def children(self) -> bidict[str, ChildType]: def child_labels(self) -> tuple[str]: return tuple(child.label for child in self) + def _check_label(self, new_label: str) -> None: + super()._check_label(new_label) + if self.child_type().semantic_delimiter in new_label: + raise ValueError( + f"Child type ({self.child_type()}) semantic delimiter " + f"{self.child_type().semantic_delimiter} cannot be in new label " + f"{new_label}" + ) + def __getattr__(self, key) -> ChildType: try: return self._children[key] diff --git a/tests/unit/mixin/test_semantics.py b/tests/unit/mixin/test_semantics.py index d1a8a204a..fbb9bac47 100644 --- a/tests/unit/mixin/test_semantics.py +++ b/tests/unit/mixin/test_semantics.py @@ -16,12 +16,18 @@ def parent_type(cls) -> type[ConcreteSemanticParent]: return ConcreteSemanticParent -class ConcreteSemanticParent(SemanticParent[ConcreteSemantic], ConcreteSemantic): +class ConcreteParent(SemanticParent[ConcreteSemantic]): + _label = "concrete_parent_default_label" + @classmethod def child_type(cls) -> type[ConcreteSemantic]: return ConcreteSemantic +class ConcreteSemanticParent(ConcreteParent, ConcreteSemantic): + pass + + class TestSemantics(unittest.TestCase): def setUp(self): self.root = ConcreteSemanticParent(label="root") @@ -57,6 +63,13 @@ def test_label_delimiter(self): ): ConcreteSemantic(label=f"invalid{ConcreteSemantic.semantic_delimiter}label") + non_semantic_parent = ConcreteParent() + with self.assertRaises( + ValueError, + msg=f"Delimiter '{ConcreteSemantic.semantic_delimiter}' not allowed", + ): + non_semantic_parent.label = f"contains_{non_semantic_parent.child_type().semantic_delimiter}_delimiter" + def test_semantic_delimiter(self): self.assertEqual( "/", From c577dfe1bdca6613037f1e83b8f36ea5e6e73c68 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 15 Jan 2025 13:47:34 -0800 Subject: [PATCH 28/43] Improve error messages Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index eb25c682f..8b4694d44 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -59,7 +59,10 @@ def parent_type(cls) -> type[ParentType]: def _check_label(self, new_label: str) -> None: super()._check_label(new_label) if self.semantic_delimiter in new_label: - raise ValueError(f"{self.semantic_delimiter} cannot be in the label") + raise ValueError( + f"Semantic delimiter {self.semantic_delimiter} cannot be in new label " + f"{new_label}" + ) @property def parent(self) -> ParentType | None: @@ -117,8 +120,8 @@ def semantic_path(self) -> str: raise ValueError( f"The parent and detached path should not be able to take non-None " f"values simultaneously, but got {self.parent} and " - f"{self.detached_parent_path}, respectively. Please raise an issue on GitHub " - f"outlining how your reached this state." + f"{self.detached_parent_path}, respectively. Please raise an issue on " + f"GitHub outlining how your reached this state." ) return prefix + self.semantic_delimiter + self.label From 0d7b2e46661ae0487aefa5eeb18fce908f14a7a3 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Thu, 9 Jan 2025 16:38:46 -0800 Subject: [PATCH 29/43] Make SemanticParent a Generic Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index e207ab920..9bf9626a7 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -293,7 +293,7 @@ def add_child( return child @staticmethod - def _ensure_path_is_not_cyclic(parent: SemanticParent | None, child: Semantic): + def _ensure_path_is_not_cyclic(parent: SemanticParent | None, child: ChildType): if parent is not None and parent.semantic_path.startswith( child.semantic_path + child.semantic_delimiter ): From 127c2a74be969e5a9bb41fe23869a346e3cf934a Mon Sep 17 00:00:00 2001 From: liamhuber Date: Fri, 10 Jan 2025 11:34:07 -0800 Subject: [PATCH 30/43] Don't use generic in static method Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 9bf9626a7..241871ce9 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -20,6 +20,7 @@ from bidict import bidict +from pyiron_workflow.compatibility import Self from pyiron_workflow.logging import logger from pyiron_workflow.mixin.has_interface_mixins import HasLabel, HasParent, UsesState @@ -293,7 +294,7 @@ def add_child( return child @staticmethod - def _ensure_path_is_not_cyclic(parent: SemanticParent | None, child: ChildType): + def _ensure_path_is_not_cyclic(parent: SemanticParent | None, child: Semantic): if parent is not None and parent.semantic_path.startswith( child.semantic_path + child.semantic_delimiter ): From 6d60ccc00af0e61df387ce5a658ac3db87901641 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Fri, 10 Jan 2025 11:49:03 -0800 Subject: [PATCH 31/43] Remove unused import Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 241871ce9..e207ab920 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -20,7 +20,6 @@ from bidict import bidict -from pyiron_workflow.compatibility import Self from pyiron_workflow.logging import logger from pyiron_workflow.mixin.has_interface_mixins import HasLabel, HasParent, UsesState From e2958b92a9b421b241ef2b7b6f637b7a3546fff8 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Mon, 13 Jan 2025 13:09:53 -0800 Subject: [PATCH 32/43] Remove HasParent An interface class guaranteeing the (Any-typed) attribute is too vague to be super useful, and redundant when it's _only_ used in `Semantic`. Having a `parent` will just be a direct feature of being semantic. Signed-off-by: liamhuber --- pyiron_workflow/mixin/has_interface_mixins.py | 11 ----------- pyiron_workflow/mixin/semantics.py | 4 ++-- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/pyiron_workflow/mixin/has_interface_mixins.py b/pyiron_workflow/mixin/has_interface_mixins.py index 2828ce7ef..7602b454a 100644 --- a/pyiron_workflow/mixin/has_interface_mixins.py +++ b/pyiron_workflow/mixin/has_interface_mixins.py @@ -53,17 +53,6 @@ def full_label(self) -> str: return self.label -class HasParent(ABC): - """ - A mixin to guarantee the parent interface exists. - """ - - @property - @abstractmethod - def parent(self) -> Any: - """A parent for the object.""" - - class HasChannel(ABC): """ A mix-in class for use with the :class:`Channel` class. diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index e207ab920..55a3aedb0 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -21,10 +21,10 @@ from bidict import bidict from pyiron_workflow.logging import logger -from pyiron_workflow.mixin.has_interface_mixins import HasLabel, HasParent, UsesState +from pyiron_workflow.mixin.has_interface_mixins import HasLabel, UsesState -class Semantic(UsesState, HasLabel, HasParent, ABC): +class Semantic(UsesState, HasLabel, ABC): """ An object with a unique semantic path. From e50d37c19c3452d88e15c6d81fea14de52d5d5b0 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 15:33:20 -0800 Subject: [PATCH 33/43] Pull out static method Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 55a3aedb0..b0caaf431 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -271,7 +271,7 @@ def add_child( f"but got {child}" ) - self._ensure_path_is_not_cyclic(self, child) + _ensure_path_is_not_cyclic(self, child) self._ensure_child_has_no_other_parent(child) @@ -292,18 +292,6 @@ def add_child( child.parent = self return child - @staticmethod - def _ensure_path_is_not_cyclic(parent: SemanticParent | None, child: Semantic): - if parent is not None and parent.semantic_path.startswith( - child.semantic_path + child.semantic_delimiter - ): - raise CyclicPathError( - f"{parent.label} cannot be the parent of {child.label}, because its " - f"semantic path is already in {child.label}'s path and cyclic paths " - f"are not allowed. (i.e. {child.semantic_path} is in " - f"{parent.semantic_path})" - ) - def _ensure_child_has_no_other_parent(self, child: Semantic): if child.parent is not None and child.parent is not self: raise ValueError( @@ -375,7 +363,7 @@ def parent(self) -> SemanticParent | None: @parent.setter def parent(self, new_parent: SemanticParent | None) -> None: - self._ensure_path_is_not_cyclic(new_parent, self) + _ensure_path_is_not_cyclic(new_parent, self) self._set_parent(new_parent) def __getstate__(self): @@ -411,3 +399,15 @@ def __setstate__(self, state): # children). So, now return their parent to them: for child in self: child.parent = self + + +def _ensure_path_is_not_cyclic(parent, child: Semantic): + if isinstance(parent, Semantic) and parent.semantic_path.startswith( + child.semantic_path + child.semantic_delimiter + ): + raise CyclicPathError( + f"{parent.label} cannot be the parent of {child.label}, because its " + f"semantic path is already in {child.label}'s path and cyclic paths " + f"are not allowed. (i.e. {child.semantic_path} is in " + f"{parent.semantic_path})" + ) \ No newline at end of file From afa53d480c1c1e42a33f1b8065a5b9c61192d56b Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 15:36:39 -0800 Subject: [PATCH 34/43] Pull cyclicity check up to Semantic Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index b0caaf431..d057339de 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -79,6 +79,8 @@ def _set_parent(self, new_parent: SemanticParent | None): f"{self.label}, but got {new_parent}" ) + _ensure_path_is_not_cyclic(new_parent, self) + if ( self._parent is not None and new_parent is not self._parent @@ -357,15 +359,6 @@ def remove_child(self, child: ChildType | str) -> ChildType: return child - @property - def parent(self) -> SemanticParent | None: - return self._parent - - @parent.setter - def parent(self, new_parent: SemanticParent | None) -> None: - _ensure_path_is_not_cyclic(new_parent, self) - self._set_parent(new_parent) - def __getstate__(self): state = super().__getstate__() From d7e69ad66e58d6b469e31521fb5ced973a875554 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 15:52:41 -0800 Subject: [PATCH 35/43] De-parent SemanticParent from Semantic Because of the label arg vs kwarg problem, there is still a vestigial label arg in the SemanticParent init signature. Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 7 +++---- tests/unit/mixin/test_semantics.py | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index d057339de..2b50d693a 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -170,7 +170,7 @@ class CyclicPathError(ValueError): ChildType = TypeVar("ChildType", bound=Semantic) -class SemanticParent(Semantic, Generic[ChildType], ABC): +class SemanticParent(Generic[ChildType], ABC): """ A semantic object with a collection of uniquely-named semantic children. @@ -189,15 +189,14 @@ class SemanticParent(Semantic, Generic[ChildType], ABC): def __init__( self, - label: str, + label: str | None, # Vestigial while the label order is broken *args, - parent: SemanticParent | None = None, strict_naming: bool = True, **kwargs, ): self._children: bidict[str, ChildType] = bidict() self.strict_naming = strict_naming - super().__init__(*args, label=label, parent=parent, **kwargs) + super().__init__(*args, label=label, **kwargs) @classmethod @abstractmethod diff --git a/tests/unit/mixin/test_semantics.py b/tests/unit/mixin/test_semantics.py index 0b63b94f0..ab6212852 100644 --- a/tests/unit/mixin/test_semantics.py +++ b/tests/unit/mixin/test_semantics.py @@ -8,7 +8,7 @@ ) -class ConcreteParent(SemanticParent[Semantic]): +class ConcreteParent(SemanticParent[Semantic], Semantic): @classmethod def child_type(cls) -> type[Semantic]: return Semantic From 1f32ee21045274fd912d29e84bd2085114976418 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:27:45 -0800 Subject: [PATCH 36/43] Remove redundant type check This is handled in the super class Signed-off-by: liamhuber --- pyiron_workflow/nodes/composite.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pyiron_workflow/nodes/composite.py b/pyiron_workflow/nodes/composite.py index e3a06cab2..e5e05ed4c 100644 --- a/pyiron_workflow/nodes/composite.py +++ b/pyiron_workflow/nodes/composite.py @@ -304,11 +304,6 @@ def add_child( label: str | None = None, strict_naming: bool | None = None, ) -> Node: - if not isinstance(child, Node): - raise TypeError( - f"Only new {Node.__name__} instances may be added, but got " - f"{type(child)}." - ) self._cached_inputs = None # Reset cache after graph change return super().add_child(child, label=label, strict_naming=strict_naming) From 33d3ce747e45c07164e114baa9f6aa8f92812d1e Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:40:17 -0800 Subject: [PATCH 37/43] Give Semantic a generic parent type Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 27 ++++++++++++++++++-------- pyiron_workflow/node.py | 9 +++++++-- tests/unit/mixin/test_semantics.py | 31 +++++++++++++++++++----------- 3 files changed, 46 insertions(+), 21 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 2b50d693a..837cef44d 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -24,7 +24,10 @@ from pyiron_workflow.mixin.has_interface_mixins import HasLabel, UsesState -class Semantic(UsesState, HasLabel, ABC): +ParentType = TypeVar("ParentType", bound="SemanticParent") + + +class Semantic(UsesState, HasLabel, Generic[ParentType], ABC): """ An object with a unique semantic path. @@ -35,7 +38,7 @@ class Semantic(UsesState, HasLabel, ABC): semantic_delimiter: str = "/" def __init__( - self, label: str, *args, parent: SemanticParent | None = None, **kwargs + self, label: str, *args, parent: ParentType | None = None, **kwargs ): self._label = "" self._parent = None @@ -44,6 +47,11 @@ def __init__( self.parent = parent super().__init__(*args, **kwargs) + @classmethod + @abstractmethod + def parent_type(cls) -> type[ParentType]: + pass + @property def label(self) -> str: return self._label @@ -57,14 +65,14 @@ def label(self, new_label: str) -> None: self._label = new_label @property - def parent(self) -> SemanticParent | None: + def parent(self) -> ParentType | None: return self._parent @parent.setter - def parent(self, new_parent: SemanticParent | None) -> None: + def parent(self, new_parent: ParentType | None) -> None: self._set_parent(new_parent) - def _set_parent(self, new_parent: SemanticParent | None): + def _set_parent(self, new_parent: ParentType | None): """ mypy is uncooperative with super calls for setters, so we pull the behaviour out. @@ -73,9 +81,9 @@ def _set_parent(self, new_parent: SemanticParent | None): # Exit early if nothing is changing return - if new_parent is not None and not isinstance(new_parent, SemanticParent): + if new_parent is not None and not isinstance(new_parent, self.parent_type()): raise ValueError( - f"Expected None or a {SemanticParent.__name__} for the parent of " + f"Expected None or a {self.parent_type()} for the parent of " f"{self.label}, but got {new_parent}" ) @@ -136,7 +144,10 @@ def full_label(self) -> str: @property def semantic_root(self) -> Semantic: """The parent-most object in this semantic path; may be self.""" - return self.parent.semantic_root if isinstance(self.parent, Semantic) else self + if isinstance(self.parent, Semantic): + return self.parent.semantic_root + else: + return self def as_path(self, root: Path | str | None = None) -> Path: """ diff --git a/pyiron_workflow/node.py b/pyiron_workflow/node.py index 3b86a5e45..fa8023d1a 100644 --- a/pyiron_workflow/node.py +++ b/pyiron_workflow/node.py @@ -21,7 +21,7 @@ from pyiron_workflow.logging import logger from pyiron_workflow.mixin.injection import HasIOWithInjection from pyiron_workflow.mixin.run import ReadinessError, Runnable -from pyiron_workflow.mixin.semantics import Semantic +from pyiron_workflow.mixin.semantics import Semantic, ParentType from pyiron_workflow.mixin.single_output import ExploitsSingleOutput from pyiron_workflow.storage import StorageInterface, available_backends from pyiron_workflow.topology import ( @@ -41,7 +41,7 @@ class Node( HasIOWithInjection, - Semantic, + Semantic["Composite"], Runnable, ExploitsSingleOutput, ABC, @@ -319,6 +319,11 @@ def __init__( **kwargs, ) + @classmethod + def parent_type(cls) -> type[Composite]: + from pyiron_workflow.nodes.composite import Composite + return Composite + def _setup_node(self) -> None: """ Called _before_ :meth:`Node.__init__` finishes. diff --git a/tests/unit/mixin/test_semantics.py b/tests/unit/mixin/test_semantics.py index ab6212852..9d74da7fe 100644 --- a/tests/unit/mixin/test_semantics.py +++ b/tests/unit/mixin/test_semantics.py @@ -1,26 +1,34 @@ +from __future__ import annotations + import unittest from pathlib import Path from pyiron_workflow.mixin.semantics import ( CyclicPathError, Semantic, - SemanticParent, + SemanticParent, ParentType, ) -class ConcreteParent(SemanticParent[Semantic], Semantic): +class ConcreteSemantic(Semantic["ConcreteParent"]): + @classmethod + def parent_type(cls) -> type[ConcreteParent]: + return ConcreteParent + + +class ConcreteParent(SemanticParent[ConcreteSemantic], ConcreteSemantic): @classmethod - def child_type(cls) -> type[Semantic]: - return Semantic + def child_type(cls) -> type[ConcreteSemantic]: + return ConcreteSemantic class TestSemantics(unittest.TestCase): def setUp(self): self.root = ConcreteParent("root") - self.child1 = Semantic("child1", parent=self.root) + self.child1 = ConcreteSemantic("child1", parent=self.root) self.middle1 = ConcreteParent("middle", parent=self.root) self.middle2 = ConcreteParent("middle_sub", parent=self.middle1) - self.child2 = Semantic("child2", parent=self.middle2) + self.child2 = ConcreteSemantic("child2", parent=self.middle2) def test_getattr(self): with self.assertRaises(AttributeError) as context: @@ -40,18 +48,19 @@ def test_getattr(self): def test_label_validity(self): with self.assertRaises(TypeError, msg="Label must be a string"): - Semantic(label=123) + ConcreteSemantic(label=123) def test_label_delimiter(self): with self.assertRaises( - ValueError, msg=f"Delimiter '{Semantic.semantic_delimiter}' not allowed" + ValueError, + msg=f"Delimiter '{ConcreteSemantic.semantic_delimiter}' not allowed" ): - Semantic(f"invalid{Semantic.semantic_delimiter}label") + ConcreteSemantic(f"invalid{ConcreteSemantic.semantic_delimiter}label") def test_semantic_delimiter(self): self.assertEqual( "/", - Semantic.semantic_delimiter, + ConcreteSemantic.semantic_delimiter, msg="This is just a hard-code to the current value, update it freely so " "the test passes; if it fails it's just a reminder that your change is " "not backwards compatible, and the next release number should reflect " @@ -105,7 +114,7 @@ def test_as_path(self): ) def test_detached_parent_path(self): - orphan = Semantic("orphan") + orphan = ConcreteSemantic("orphan") orphan.__setstate__(self.child2.__getstate__()) self.assertIsNone( orphan.parent, msg="We still should not explicitly have a parent" From a1815e54b1caeef6a56070bb0c8be5c5bf315190 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:45:07 -0800 Subject: [PATCH 38/43] Remove unused import Signed-off-by: liamhuber --- pyiron_workflow/node.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyiron_workflow/node.py b/pyiron_workflow/node.py index fa8023d1a..efcb9f2e7 100644 --- a/pyiron_workflow/node.py +++ b/pyiron_workflow/node.py @@ -21,7 +21,7 @@ from pyiron_workflow.logging import logger from pyiron_workflow.mixin.injection import HasIOWithInjection from pyiron_workflow.mixin.run import ReadinessError, Runnable -from pyiron_workflow.mixin.semantics import Semantic, ParentType +from pyiron_workflow.mixin.semantics import Semantic from pyiron_workflow.mixin.single_output import ExploitsSingleOutput from pyiron_workflow.storage import StorageInterface, available_backends from pyiron_workflow.topology import ( From 9dd113dd5f8ea6982b1f88c12187c6cd1bd15414 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:46:02 -0800 Subject: [PATCH 39/43] Black Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 6 ++---- pyiron_workflow/node.py | 1 + tests/unit/mixin/test_semantics.py | 4 ++-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 837cef44d..ba7289b1c 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -37,9 +37,7 @@ class Semantic(UsesState, HasLabel, Generic[ParentType], ABC): semantic_delimiter: str = "/" - def __init__( - self, label: str, *args, parent: ParentType | None = None, **kwargs - ): + def __init__(self, label: str, *args, parent: ParentType | None = None, **kwargs): self._label = "" self._parent = None self._detached_parent_path = None @@ -413,4 +411,4 @@ def _ensure_path_is_not_cyclic(parent, child: Semantic): f"semantic path is already in {child.label}'s path and cyclic paths " f"are not allowed. (i.e. {child.semantic_path} is in " f"{parent.semantic_path})" - ) \ No newline at end of file + ) diff --git a/pyiron_workflow/node.py b/pyiron_workflow/node.py index efcb9f2e7..6e19a7042 100644 --- a/pyiron_workflow/node.py +++ b/pyiron_workflow/node.py @@ -322,6 +322,7 @@ def __init__( @classmethod def parent_type(cls) -> type[Composite]: from pyiron_workflow.nodes.composite import Composite + return Composite def _setup_node(self) -> None: diff --git a/tests/unit/mixin/test_semantics.py b/tests/unit/mixin/test_semantics.py index 9d74da7fe..874928f79 100644 --- a/tests/unit/mixin/test_semantics.py +++ b/tests/unit/mixin/test_semantics.py @@ -6,7 +6,7 @@ from pyiron_workflow.mixin.semantics import ( CyclicPathError, Semantic, - SemanticParent, ParentType, + SemanticParent, ) @@ -53,7 +53,7 @@ def test_label_validity(self): def test_label_delimiter(self): with self.assertRaises( ValueError, - msg=f"Delimiter '{ConcreteSemantic.semantic_delimiter}' not allowed" + msg=f"Delimiter '{ConcreteSemantic.semantic_delimiter}' not allowed", ): ConcreteSemantic(f"invalid{ConcreteSemantic.semantic_delimiter}label") From 6cb92c020325ba9b0b9d85c4419af1cb2404a9dc Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:46:28 -0800 Subject: [PATCH 40/43] Ruff sort imports Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index ba7289b1c..dca7bea2f 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -23,7 +23,6 @@ from pyiron_workflow.logging import logger from pyiron_workflow.mixin.has_interface_mixins import HasLabel, UsesState - ParentType = TypeVar("ParentType", bound="SemanticParent") From 5cb569926a0cda1735f18bb312a3fadae808f9a7 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:54:15 -0800 Subject: [PATCH 41/43] Remove unused import Signed-off-by: liamhuber --- pyiron_workflow/mixin/has_interface_mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyiron_workflow/mixin/has_interface_mixins.py b/pyiron_workflow/mixin/has_interface_mixins.py index 7602b454a..729431761 100644 --- a/pyiron_workflow/mixin/has_interface_mixins.py +++ b/pyiron_workflow/mixin/has_interface_mixins.py @@ -11,7 +11,7 @@ from __future__ import annotations from abc import ABC, abstractmethod -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING if TYPE_CHECKING: from pyiron_workflow.channels import Channel From f516fe36fd66dd129147695d1c80ddbbd0e97908 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 14 Jan 2025 16:59:28 -0800 Subject: [PATCH 42/43] Update docstrings Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index dca7bea2f..8c1ef6d4a 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -2,10 +2,11 @@ Classes for "semantic" reasoning. The motivation here is to be able to provide the object with a unique identifier -in the context of other semantic objects. Each object may have exactly one parent -and an arbitrary number of children, and each child's name must be unique in the -scope of that parent. In this way, the path from the parent-most object to any -child is completely unique. The typical filesystem on a computer is an excellent +in the context of other semantic objects. Each object may have at most one parent, +while semantic parents may have an arbitrary number of children, and each child's name +must be unique in the scope of that parent. In this way, when semantic parents are also +themselves semantic, we can build a path from the parent-most object to any child that +is completely unique. The typical filesystem on a computer is an excellent example and fulfills our requirements, the only reason we depart from it is so that we are free to have objects stored in different locations (possibly even on totally different drives or machines) belong to the same semantic group. @@ -180,7 +181,7 @@ class CyclicPathError(ValueError): class SemanticParent(Generic[ChildType], ABC): """ - A semantic object with a collection of uniquely-named semantic children. + An with a collection of uniquely-named semantic children. Children should be added or removed via the :meth:`add_child` and :meth:`remove_child` methods and _not_ by direct manipulation of the From 40d9144c7a371437636393546481bca8b6a6bf74 Mon Sep 17 00:00:00 2001 From: Liam Huber Date: Thu, 16 Jan 2025 09:59:43 -0800 Subject: [PATCH 43/43] Annotate some extra returns (#548) Signed-off-by: liamhuber --- pyiron_workflow/mixin/semantics.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pyiron_workflow/mixin/semantics.py b/pyiron_workflow/mixin/semantics.py index 8b4694d44..1ecfd8fb5 100644 --- a/pyiron_workflow/mixin/semantics.py +++ b/pyiron_workflow/mixin/semantics.py @@ -316,7 +316,7 @@ def add_child( child.parent = self return child - def _ensure_child_has_no_other_parent(self, child: Semantic): + def _ensure_child_has_no_other_parent(self, child: Semantic) -> None: if child.parent is not None and child.parent is not self: raise ValueError( f"The child ({child.label}) already belongs to the parent " @@ -324,17 +324,17 @@ def _ensure_child_has_no_other_parent(self, child: Semantic): f"add it to this parent ({self.label})." ) - def _this_child_is_already_at_this_label(self, child: Semantic, label: str): + def _this_child_is_already_at_this_label(self, child: Semantic, label: str) -> bool: return ( label == child.label and label in self.child_labels and self.children[label] is child ) - def _this_child_is_already_at_a_different_label(self, child, label): + def _this_child_is_already_at_a_different_label(self, child, label) -> bool: return child.parent is self and label != child.label - def _get_unique_label(self, label: str, strict_naming: bool): + def _get_unique_label(self, label: str, strict_naming: bool) -> str: if label in self.__dir__(): if label in self.child_labels: if strict_naming: @@ -351,7 +351,7 @@ def _get_unique_label(self, label: str, strict_naming: bool): ) return label - def _add_suffix_to_label(self, label): + def _add_suffix_to_label(self, label: str) -> str: i = 0 new_label = label while new_label in self.__dir__(): @@ -416,7 +416,7 @@ def __setstate__(self, state): child.parent = self -def _ensure_path_is_not_cyclic(parent, child: Semantic): +def _ensure_path_is_not_cyclic(parent, child: Semantic) -> None: if isinstance(parent, Semantic) and parent.semantic_path.startswith( child.semantic_path + child.semantic_delimiter ):