From 908674438e3c5469a4706357706147368ad9541c Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 23 Apr 2024 09:13:44 -0700 Subject: [PATCH 01/19] Change paradigm to whether or not the node uses __reduced__ and a constructor Instead of "Meta" nodes --- pyiron_workflow/constructed.py | 84 ++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 pyiron_workflow/constructed.py diff --git a/pyiron_workflow/constructed.py b/pyiron_workflow/constructed.py new file mode 100644 index 00000000..d1306f54 --- /dev/null +++ b/pyiron_workflow/constructed.py @@ -0,0 +1,84 @@ +from __future__ import annotations + +from abc import ABC, abstractmethod + + +class Constructed(ABC): + """ + A mixin which overrides `__reduce__` to return a constructor callable and arguments. + + This is useful for classes which are not importable, but who have constructor + functions that _are_, e.g. children of `StaticNode` that are dynamically created so + their flexibility comes from the class. + """ + + @staticmethod + @abstractmethod + def _instance_constructor( + class_factory, + factory_args, + factory_kwargs, + instance_args, + instance_kwargs, + ) -> callable[[...], Constructed]: + """ + A constructor function returning an instance of the class. + + Args: + class_factory (callable): A method returning the class + factory_args (tuple): Args to pass to the factory method. + factory_kwargs (dict): Kwargs to pass to the factory method. + instance_args (tuple): Args to pass to the new instance. + instance_kwargs (dict): Kwargs to pass to the new instance. + + Returns: + (Constructed): A new instance of the factory result mixed with + :class:`Constructed`. + """ + + @property + @abstractmethod + def _instance_constructor_args(self) -> tuple: + """The arguments to pass to :meth:`_instance_constructor""" + + def __reduce__(self): + return ( + self._instance_constructor, + self._instance_constructor_args, + self.__getstate__() + ) + + +def mix_and_construct_instance( + class_factory, factory_args, factory_kwargs, instance_args, instance_kwargs +): + """ + A constructor function that dynamically mixes in :class:`Constructed` inheritance. + + Args: + class_factory (callable): A method returning a new class, i.e. `return type...`. + factory_args (tuple): Args to pass to the factory method. + factory_kwargs (dict): Kwargs to pass to the factory method. + instance_args (tuple): Args to pass to the new instance. + instance_kwargs (dict): Kwargs to pass to the new instance. + + Returns: + (Constructed): A new instance of the factory result mixed with + :class:`Constructed`. + """ + base_class = class_factory(*factory_args, **factory_kwargs) + return type( + Constructed.__name__ + base_class.__name__, + (base_class, Constructed), + { + "_instance_constructor": staticmethod(mix_and_construct_instance), + "_instance_constructor_args": ( + class_factory, + factory_args, + factory_kwargs, + (), + {} + ), + "__module__": base_class.__module__, + } + )(*instance_args, **instance_kwargs) From a628d2ba0473cc828aa82cee8a16bf3127b44691 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Tue, 23 Apr 2024 11:04:39 -0700 Subject: [PATCH 02/19] Allow direct use of Constructed children --- pyiron_workflow/constructed.py | 46 ++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/pyiron_workflow/constructed.py b/pyiron_workflow/constructed.py index d1306f54..8f3f215f 100644 --- a/pyiron_workflow/constructed.py +++ b/pyiron_workflow/constructed.py @@ -10,10 +10,22 @@ class Constructed(ABC): This is useful for classes which are not importable, but who have constructor functions that _are_, e.g. children of `StaticNode` that are dynamically created so their flexibility comes from the class. + + Unlike overriding `__new__`, no changes are needed for the `__init__ """ - @staticmethod + @property @abstractmethod + def _instance_constructor_args(self) -> tuple: + """ + The arguments to pass to :meth:`_instance_constructor`. + + Should be a 3-tuple of the callable class factory, class factory args, and + class factory kwargs. On unpickling, instance args and kwargs get provided + by :meth:`__setstate__` from the pickled state, so these can be ignored. + """ + + @staticmethod def _instance_constructor( class_factory, factory_args, @@ -35,20 +47,40 @@ def _instance_constructor( (Constructed): A new instance of the factory result mixed with :class:`Constructed`. """ - - @property - @abstractmethod - def _instance_constructor_args(self) -> tuple: - """The arguments to pass to :meth:`_instance_constructor""" + return construct_instance( + class_factory, factory_args, factory_kwargs, instance_args, instance_kwargs + ) def __reduce__(self): return ( self._instance_constructor, - self._instance_constructor_args, + (*self._instance_constructor_args, (), {}), self.__getstate__() ) +def construct_instance( + class_factory, factory_args, factory_kwargs, instance_args, instance_kwargs +): + """ + A constructor function for classes that inherit from :class:`Constructed`. + + Args: + class_factory (callable): A method returning a new class, i.e. `return type...`. + factory_args (tuple): Args to pass to the factory method. + factory_kwargs (dict): Kwargs to pass to the factory method. + instance_args (tuple): Args to pass to the new instance. + instance_kwargs (dict): Kwargs to pass to the new instance. + + Returns: + (Constructed): A new instance of the factory result mixed with + :class:`Constructed`. + """ + return class_factory(*factory_args, **factory_kwargs)( + *instance_args, **instance_kwargs + ) + + def mix_and_construct_instance( class_factory, factory_args, factory_kwargs, instance_args, instance_kwargs ): From 0a965637db24e5754f4944e7ff9bfbec1f5c247d Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 24 Apr 2024 13:39:22 -0700 Subject: [PATCH 03/19] Move and update constructed stuff --- pyiron_workflow/constructed.py | 116 -------------------- pyiron_workflow/snippets/constructed.py | 137 ++++++++++++++++++++++++ tests/unit/snippets/test_constructed.py | 106 ++++++++++++++++++ 3 files changed, 243 insertions(+), 116 deletions(-) delete mode 100644 pyiron_workflow/constructed.py create mode 100644 pyiron_workflow/snippets/constructed.py create mode 100644 tests/unit/snippets/test_constructed.py diff --git a/pyiron_workflow/constructed.py b/pyiron_workflow/constructed.py deleted file mode 100644 index 8f3f215f..00000000 --- a/pyiron_workflow/constructed.py +++ /dev/null @@ -1,116 +0,0 @@ -from __future__ import annotations - -from abc import ABC, abstractmethod - - -class Constructed(ABC): - """ - A mixin which overrides `__reduce__` to return a constructor callable and arguments. - - This is useful for classes which are not importable, but who have constructor - functions that _are_, e.g. children of `StaticNode` that are dynamically created so - their flexibility comes from the class. - - Unlike overriding `__new__`, no changes are needed for the `__init__ - """ - - @property - @abstractmethod - def _instance_constructor_args(self) -> tuple: - """ - The arguments to pass to :meth:`_instance_constructor`. - - Should be a 3-tuple of the callable class factory, class factory args, and - class factory kwargs. On unpickling, instance args and kwargs get provided - by :meth:`__setstate__` from the pickled state, so these can be ignored. - """ - - @staticmethod - def _instance_constructor( - class_factory, - factory_args, - factory_kwargs, - instance_args, - instance_kwargs, - ) -> callable[[...], Constructed]: - """ - A constructor function returning an instance of the class. - - Args: - class_factory (callable): A method returning the class - factory_args (tuple): Args to pass to the factory method. - factory_kwargs (dict): Kwargs to pass to the factory method. - instance_args (tuple): Args to pass to the new instance. - instance_kwargs (dict): Kwargs to pass to the new instance. - - Returns: - (Constructed): A new instance of the factory result mixed with - :class:`Constructed`. - """ - return construct_instance( - class_factory, factory_args, factory_kwargs, instance_args, instance_kwargs - ) - - def __reduce__(self): - return ( - self._instance_constructor, - (*self._instance_constructor_args, (), {}), - self.__getstate__() - ) - - -def construct_instance( - class_factory, factory_args, factory_kwargs, instance_args, instance_kwargs -): - """ - A constructor function for classes that inherit from :class:`Constructed`. - - Args: - class_factory (callable): A method returning a new class, i.e. `return type...`. - factory_args (tuple): Args to pass to the factory method. - factory_kwargs (dict): Kwargs to pass to the factory method. - instance_args (tuple): Args to pass to the new instance. - instance_kwargs (dict): Kwargs to pass to the new instance. - - Returns: - (Constructed): A new instance of the factory result mixed with - :class:`Constructed`. - """ - return class_factory(*factory_args, **factory_kwargs)( - *instance_args, **instance_kwargs - ) - - -def mix_and_construct_instance( - class_factory, factory_args, factory_kwargs, instance_args, instance_kwargs -): - """ - A constructor function that dynamically mixes in :class:`Constructed` inheritance. - - Args: - class_factory (callable): A method returning a new class, i.e. `return type...`. - factory_args (tuple): Args to pass to the factory method. - factory_kwargs (dict): Kwargs to pass to the factory method. - instance_args (tuple): Args to pass to the new instance. - instance_kwargs (dict): Kwargs to pass to the new instance. - - Returns: - (Constructed): A new instance of the factory result mixed with - :class:`Constructed`. - """ - base_class = class_factory(*factory_args, **factory_kwargs) - return type( - Constructed.__name__ + base_class.__name__, - (base_class, Constructed), - { - "_instance_constructor": staticmethod(mix_and_construct_instance), - "_instance_constructor_args": ( - class_factory, - factory_args, - factory_kwargs, - (), - {} - ), - "__module__": base_class.__module__, - } - )(*instance_args, **instance_kwargs) diff --git a/pyiron_workflow/snippets/constructed.py b/pyiron_workflow/snippets/constructed.py new file mode 100644 index 00000000..9cc1da01 --- /dev/null +++ b/pyiron_workflow/snippets/constructed.py @@ -0,0 +1,137 @@ +from __future__ import annotations + +from abc import ABC + + +class Constructed(ABC): + """ + A mixin which overrides `__reduce__` to return a constructor callable and arguments. + + This is useful for classes which are not importable, but who have constructor + functions that _are_, e.g. classes dynamically created classes. + + Unlike overriding `__new__`, no changes are needed for the `__init__` signature. + + Child classes must define the :attr:`_class_factory`, as well as + :attr:`_class_factory_args` and :attr:`_class_instance_args` (if either the factory + or returned class's `__init__` take positional arguments), and may optionally + specify :attr:`_class_factory_kwargs` and/or :attr:`_class_instance_kwargs` if + either callable has keyword arguments that should be specified. + In the case that the mixin is being added dynamically at instantiation, subclass + kwargs need to be re-specified in :attr:`_class_subclass_kwargs`. + All of these parameters should be specified at the time of subclassing (via + :meth:`__init_subclass__`) using keywords of the same (but public) names. + """ + + def __init_subclass__( + cls, + /, + class_factory: callable = None, + class_factory_args: tuple = (), + class_factory_kwargs: dict | None = None, + class_instance_args: tuple = (), + class_instance_kwargs: dict | None = None, + class_subclass_kwargs: dict | None = None, + **kwargs + ): + super(Constructed, cls).__init_subclass__(**kwargs) + + cls._class_factory = staticmethod(class_factory) + if class_factory is not None: + # Intermediate abstract classes may not yet specify a factory + cls.__module__ = class_factory.__module__ + + cls._class_factory_args = class_factory_args + cls._class_factory_kwargs = ( + {} if class_factory_kwargs is None else class_factory_kwargs + ) + cls._class_instance_args = class_instance_args + cls._class_instance_kwargs = ( + {} if class_instance_kwargs is None else class_instance_kwargs + ) + cls._class_subclass_kwargs = ( + {} if class_subclass_kwargs is None else class_subclass_kwargs + ) + + @staticmethod + def _instance_constructor( + class_factory, + factory_args, + factory_kwargs, + instance_args, + instance_kwargs, + subclass_kwargs, + ) -> callable[[...], Constructed]: + """ + A constructor function returning an instance of the class. + + Args: + class_factory (callable): A method returning the class + factory_args (tuple): Args to pass to the factory method. + factory_kwargs (dict): Kwargs to pass to the factory method. + instance_args (tuple): Args to pass to the new instance. + instance_kwargs (dict): Kwargs to pass to the new instance. + subclass_kwargs (dict): Kwargs to pass to `type` when this method is + overridden by :func:`mix_and_construct_instance`. + + Returns: + (Constructed): A new instance of the factory result mixed with + :class:`Constructed`. + """ + return class_factory(*factory_args, **factory_kwargs)( + *instance_args, **instance_kwargs + ) + + def __reduce__(self): + return ( + self._instance_constructor, + ( + self._class_factory, + self._class_factory_args, + self._class_factory_kwargs, + self._class_instance_args, # Args may be _necessary_ for __init__ + self._class_instance_kwargs, + self._class_subclass_kwargs, + ), + self.__getstate__() + ) + + + +def mix_and_construct_instance( + class_factory, + factory_args, + factory_kwargs, + instance_args, + instance_kwargs, + subclass_kwargs +): + """ + A constructor function that dynamically mixes in :class:`Constructed` inheritance. + + Args: + class_factory (callable): A method returning a new class, i.e. `return type...`. + factory_args (tuple): Args to pass to the factory method. + factory_kwargs (dict): Kwargs to pass to the factory method. + instance_args (tuple): Args to pass to the new instance. + instance_kwargs (dict): Kwargs to pass to the new instance. + subclass_kwargs (dict): Kwargs to pass to `type` to make sure subclassing + doesn't lose info passed to the factory by getting overwritten by the + superclass's `__init_subclass__` default values. + + Returns: + (Constructed): A new instance of the factory result mixed with + :class:`Constructed`. + """ + base_class = class_factory(*factory_args, **factory_kwargs) + return type( + Constructed.__name__ + base_class.__name__, + (Constructed, base_class), + {"_instance_constructor": staticmethod(mix_and_construct_instance)}, + class_factory=class_factory, + class_factory_args=factory_args, + class_factory_kwargs=factory_kwargs, + class_instance_args=instance_args, + class_subclass_kwargs=subclass_kwargs, + **subclass_kwargs + )(*instance_args, **instance_kwargs) diff --git a/tests/unit/snippets/test_constructed.py b/tests/unit/snippets/test_constructed.py new file mode 100644 index 00000000..58f8d9b5 --- /dev/null +++ b/tests/unit/snippets/test_constructed.py @@ -0,0 +1,106 @@ +from abc import ABC +import pickle +import unittest + +from pyiron_workflow.snippets.constructed import Constructed, mix_and_construct_instance + + +class HasN(ABC): + def __init_subclass__(cls, /, n=0, s="foo", **kwargs): + super(HasN, cls).__init_subclass__(**kwargs) + cls.n = n + cls.s = s + + def __init__(self, x, y=0): + self.x = x + self.y = y + + +def has_n_factory(n, s="bar"): + return type( + f"{HasN.__name__}{n}", + (HasN,), + {"__module__": HasN.__module__}, + n=n, + s=s + ) + + +class Constructable(Constructed, HasN, ABC): + pass + + +def constructable_factory(n, s="baz"): + return type( + f"{Constructable.__name__}{n}", + (Constructable,), + {}, + n=n, + s=s, + class_factory=constructable_factory, + class_factory_args=(n,), + class_factory_kwargs={"s": s}, + class_instance_args=(0,) + ) + + +class TestConstructed(unittest.TestCase): + def test_reduce(self): + by_inheritance = constructable_factory(1)(2) + constructor, constructor_args, state = by_inheritance.__reduce__() + f, f_args, f_kwargs, i_args, i_kwargs, sc_kwargs = constructor_args + + self.assertIs(Constructed._instance_constructor, constructor) + self.assertIs(constructable_factory, f) + self.assertTupleEqual((1,), f_args) + self.assertDictEqual({"s": "baz"}, f_kwargs) # from the factory defaults + self.assertTupleEqual((0,), i_args) # from the factory + self.assertDictEqual({}, i_kwargs) # from Constructed + self.assertDictEqual({}, sc_kwargs) # from Constructed + + def test_inheritance_mixin(self): + by_inheritance = constructable_factory(42)(43, y=44) + self.assertTupleEqual( + (42, "baz", 43, 44), + (by_inheritance.n, by_inheritance.s, by_inheritance.x, by_inheritance.y), + msg="Sanity check." + ) + reloaded = pickle.loads(pickle.dumps(by_inheritance)) + self.assertTupleEqual( + (by_inheritance.n, by_inheritance.s, by_inheritance.x, by_inheritance.y), + (reloaded.n, reloaded.s, reloaded.x, reloaded.y), + msg=f"Children of {Constructed.__name__} should recover both class and " + f"instance state under the (un)pickle cycle." + ) + + def test_instantiation_mixin(self): + dynamic_mixin = mix_and_construct_instance( + has_n_factory, + (42,), + {"s": "baz"}, + (43,), + {"y": 44}, + {"n": 42, "s": "baz"}, # __init_subclass__ kwargs get duplicated here + # This is annoying for users of `mix_and_construct_instance`, but not + # difficult. + ) + self.assertIsInstance( + dynamic_mixin, + Constructed, + msg=f"{mix_and_construct_instance.__name__} should dynamically add " + f"{Constructed.__name__} inheritance." + ) + self.assertTupleEqual( + (42, "baz", 43, 44), + (dynamic_mixin.n, dynamic_mixin.s, dynamic_mixin.x, dynamic_mixin.y), + msg="Sanity check." + ) + + reloaded = pickle.loads(pickle.dumps(dynamic_mixin)) + self.assertTupleEqual( + (dynamic_mixin.n, dynamic_mixin.s, dynamic_mixin.x, dynamic_mixin.y), + (reloaded.n, reloaded.s, reloaded.x, reloaded.y), + msg=f"Children of {Constructed.__name__} should recover both class and " + f"instance state under the (un)pickle cycle." + ) + From 381b296338853a4c8ac6a9298a8131c8689a34b1 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 24 Apr 2024 13:40:16 -0700 Subject: [PATCH 04/19] Add new singleton behaviour so factory-produced classes can pass is-tests --- pyiron_workflow/snippets/singleton.py | 117 +++++++++++++++++++++++--- tests/unit/snippets/test_singleton.py | 100 +++++++++++++++++++++- 2 files changed, 203 insertions(+), 14 deletions(-) diff --git a/pyiron_workflow/snippets/singleton.py b/pyiron_workflow/snippets/singleton.py index 5b8b85d0..3be2abee 100644 --- a/pyiron_workflow/snippets/singleton.py +++ b/pyiron_workflow/snippets/singleton.py @@ -3,21 +3,13 @@ # Distributed under the terms of "New BSD License", see the LICENSE file. """ -Utility functions used in pyiron. -In order to be accessible from anywhere in pyiron, they *must* remain free of any imports from pyiron! +Tools for singleton objects. """ -from abc import ABCMeta -__author__ = "Joerg Neugebauer, Jan Janssen" -__copyright__ = ( - "Copyright 2020, Max-Planck-Institut für Eisenforschung GmbH - " - "Computational Materials Design (CM) Department" -) -__version__ = "1.0" -__maintainer__ = "Jan Janssen" -__email__ = "janssen@mpie.de" -__status__ = "production" -__date__ = "Sep 1, 2017" +from __future__ import annotations + +from abc import ABCMeta +from functools import wraps class Singleton(ABCMeta): @@ -34,3 +26,102 @@ def __call__(cls, *args, **kwargs): if cls not in cls._instances: cls._instances[cls] = super(Singleton, cls).__call__(*args, **kwargs) return cls._instances[cls] + + +class _RegisteredFactory(metaclass=Singleton): + """ + For making dynamically created classes the same class. + + Used under-the-hood in the public facing decorator below. + """ + def __init_subclass__(cls, /, factory_function: callable[..., type]): + cls.factory_function = staticmethod(factory_function) + cls.registry = {} + + def __call__(self, *args, **kwargs): + constructed = self.factory_function(*args, **kwargs) + try: + return self.registry[constructed.__name__] + except KeyError: + self.registry[constructed.__name__] = constructed + return constructed + + def __reduce__(self): + return ( + _FactoryTown().get_registered_factory, + (self.factory_function,), + self.__getstate__(), + ) + + +class _FactoryTown(metaclass=Singleton): + """ + A singleton to hold existing factories, so that if we wrap the same factory + function more than once, we always get back the same factory. + """ + factories = {} + + def get_registered_factory( + self, factory_function: callable[..., type] + ) -> type[_RegisteredFactory]: + try: + return self.factories[id(factory_function)] + except KeyError: + factory_class = type( + f"{_RegisteredFactory.__name__}{factory_function.__name__.title()}", + (_RegisteredFactory,), + {}, + factory_function=factory_function + ) + factory = wraps(factory_function)(factory_class()) + self.factories[id(factory_function)] = factory + return factory + + +def registered_factory(factory_function: callable[..., type]): + """ + A decorator for wrapping class factories. + + Wrapped factories return the _same object_ when they would generate a class with + the same name as they have generated before. I.e. a sort of singleton-class + generator where the classes themselves are singleton, not their instances. + + Args: + factory_function (callabe[..., type]): The class factory to wrap. + + Returns: + (_RegisteredFactory): A factory instance that will return the same class object + whenever the factory method would return a class whose name has been seen + before. + + Example: + >>> from abc import ABC + >>> + >>> from pyiron_workflow.snippets.singleton import registered_factory + >>> + >>> class Foo(ABC): + ... def __init_subclass__(cls, /, n=0, **kwargs): + ... super().__init_subclass__(**kwargs) + ... cls.n = n + >>> + >>> @registered_factory + ... def foo_factory(n): + ... return type( + ... f"{Foo.__name__}{n}", + ... (Foo,), + ... {}, + ... n=n + ... ) + >>> + >>> FooTwo = foo_factory(2) + >>> Foo2 = foo_factory(2) + >>> print(FooTwo.__name__, FooTwo.n) + Foo2 2 + + >>> print(Foo2.__name__, Foo2.n) + Foo2 2 + + >>> print(FooTwo is Foo2) + True + """ + return _FactoryTown().get_registered_factory(factory_function) diff --git a/tests/unit/snippets/test_singleton.py b/tests/unit/snippets/test_singleton.py index 7953445e..2ba85df5 100644 --- a/tests/unit/snippets/test_singleton.py +++ b/tests/unit/snippets/test_singleton.py @@ -1,5 +1,7 @@ +from abc import ABC import unittest -from pyiron_workflow.snippets.singleton import Singleton + +from pyiron_workflow.snippets.singleton import Singleton, registered_factory class TestSingleton(unittest.TestCase): @@ -15,5 +17,101 @@ def __init__(self): self.assertEqual(2, f1.x) +class TestRegisteredFactory(unittest.TestCase): + def test_decorator(self): + class Foo(ABC): + def __init_subclass__(cls, /, n=0, **kwargs): + super().__init_subclass__(**kwargs) + cls.n = n + + def foo_factory(n): + """The foo factory docstring.""" + return type( + f"{Foo.__name__}{n}", + (Foo,), + {}, + n=n + ) + + FooTwo = foo_factory(2) + Foo2 = foo_factory(2) + self.assertEqual( + FooTwo.__name__, + Foo2.__name__, + msg="Sanity check" + ) + self.assertIsNot( + FooTwo, + Foo2, + msg="Sanity check" + ) + self.assertEqual( + 2, + Foo2.n, + msg="Sanity check" + ) + + Foo3 = foo_factory(3) + self.assertIsNot( + Foo3, + Foo2, + msg="Sanity check" + ) + + registered_foo_factory = registered_factory(foo_factory) + FooTwo = registered_foo_factory(2) + Foo2 = registered_foo_factory(2) + self.assertEqual( + FooTwo.__name__, + Foo2.__name__, + msg="Sanity check" + ) + self.assertIs( + FooTwo, + Foo2, + msg="The point of the registration is that dynamically generated classes " + "with the same name from the same generator should wind up being the " + "same class" + ) + self.assertEqual( + "The foo factory docstring.", + registered_foo_factory.__doc__, + msg="The wrapper should preserve the factory's docstring" + ) + + re_registered_foo_factory = registered_factory(foo_factory) + self.assertIs( + registered_foo_factory, + re_registered_foo_factory, + msg="The factories themselves are singletons based on the id of the " + "factory function they use; If you register the same factory function " + "twice, you should get the same factory back." + ) + self.assertIs( + re_registered_foo_factory(2), + Foo2, + msg="From the above, it should hold trivially that building the same class " + "from each factory gives the same class." + ) + + @registered_factory + def different_but_similar(n): + """The foo factory docstring.""" + return foo_factory(n) + + Foo2b = different_but_similar(2) + self.assertEqual( + Foo2b.__name__, + Foo2.__name__, + msg="Sanity check. It's the same factory behaviour after all." + ) + self.assertIsNot( + Foo2b, + Foo2, + msg="These come from two separate registered factories, each of which is " + "maintaining its own internal registration list." + ) + + if __name__ == '__main__': unittest.main() From 2943aff93c37eea5f2ca1c04b93bbceb5eaaa2e9 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 24 Apr 2024 13:44:03 -0700 Subject: [PATCH 05/19] PEP8 newline --- pyiron_workflow/snippets/constructed.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pyiron_workflow/snippets/constructed.py b/pyiron_workflow/snippets/constructed.py index 9cc1da01..a49cce3f 100644 --- a/pyiron_workflow/snippets/constructed.py +++ b/pyiron_workflow/snippets/constructed.py @@ -97,7 +97,6 @@ def __reduce__(self): ) - def mix_and_construct_instance( class_factory, factory_args, From 10dd9ff9c538110659de7697ea3810912973585b Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 24 Apr 2024 14:17:18 -0700 Subject: [PATCH 06/19] Remove unnecessary __getstate__ The object isn't holding instance level state and older versions of python bork here. --- pyiron_workflow/snippets/singleton.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pyiron_workflow/snippets/singleton.py b/pyiron_workflow/snippets/singleton.py index 3be2abee..0b58b314 100644 --- a/pyiron_workflow/snippets/singleton.py +++ b/pyiron_workflow/snippets/singleton.py @@ -50,7 +50,6 @@ def __reduce__(self): return ( _FactoryTown().get_registered_factory, (self.factory_function,), - self.__getstate__(), ) From 03e134eeda332f8f8e5a9201b882045ba7d5d11d Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 24 Apr 2024 14:23:30 -0700 Subject: [PATCH 07/19] Add constructed __*state__ compatibility for older versions --- pyiron_workflow/snippets/constructed.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/pyiron_workflow/snippets/constructed.py b/pyiron_workflow/snippets/constructed.py index a49cce3f..3d566423 100644 --- a/pyiron_workflow/snippets/constructed.py +++ b/pyiron_workflow/snippets/constructed.py @@ -96,6 +96,20 @@ def __reduce__(self): self.__getstate__() ) + def __getstate__(self): + # Backwards compatibility + try: + super().__getstate__() + except AttributeError: + return dict(self.__dict__) + + def __setstate__(self, state): + # Backwards compatibility + try: + super().__setstate__(state) + except AttributeError: + self.__dict__.update(**state) + def mix_and_construct_instance( class_factory, From 0f5ecf1d42f7925d4bc5b3eb33f1b8d266df3999 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Wed, 24 Apr 2024 14:28:16 -0700 Subject: [PATCH 08/19] :bug: add missing `return` --- pyiron_workflow/snippets/constructed.py | 2 +- tests/unit/snippets/test_constructed.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pyiron_workflow/snippets/constructed.py b/pyiron_workflow/snippets/constructed.py index 3d566423..c97d66a4 100644 --- a/pyiron_workflow/snippets/constructed.py +++ b/pyiron_workflow/snippets/constructed.py @@ -99,7 +99,7 @@ def __reduce__(self): def __getstate__(self): # Backwards compatibility try: - super().__getstate__() + return super().__getstate__() except AttributeError: return dict(self.__dict__) diff --git a/tests/unit/snippets/test_constructed.py b/tests/unit/snippets/test_constructed.py index 58f8d9b5..4c7fb8c3 100644 --- a/tests/unit/snippets/test_constructed.py +++ b/tests/unit/snippets/test_constructed.py @@ -60,6 +60,7 @@ def test_reduce(self): def test_inheritance_mixin(self): by_inheritance = constructable_factory(42)(43, y=44) + print(by_inheritance.__getstate__()) self.assertTupleEqual( (42, "baz", 43, 44), (by_inheritance.n, by_inheritance.s, by_inheritance.x, by_inheritance.y), From 1209762facef67f02779926fb6d83cec23413905 Mon Sep 17 00:00:00 2001 From: pyiron-runner Date: Wed, 24 Apr 2024 21:28:46 +0000 Subject: [PATCH 09/19] Format black --- pyiron_workflow/snippets/constructed.py | 8 ++++---- pyiron_workflow/snippets/singleton.py | 4 +++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/pyiron_workflow/snippets/constructed.py b/pyiron_workflow/snippets/constructed.py index c97d66a4..91914f59 100644 --- a/pyiron_workflow/snippets/constructed.py +++ b/pyiron_workflow/snippets/constructed.py @@ -32,7 +32,7 @@ def __init_subclass__( class_instance_args: tuple = (), class_instance_kwargs: dict | None = None, class_subclass_kwargs: dict | None = None, - **kwargs + **kwargs, ): super(Constructed, cls).__init_subclass__(**kwargs) @@ -93,7 +93,7 @@ def __reduce__(self): self._class_instance_kwargs, self._class_subclass_kwargs, ), - self.__getstate__() + self.__getstate__(), ) def __getstate__(self): @@ -117,7 +117,7 @@ def mix_and_construct_instance( factory_kwargs, instance_args, instance_kwargs, - subclass_kwargs + subclass_kwargs, ): """ A constructor function that dynamically mixes in :class:`Constructed` inheritance. @@ -146,5 +146,5 @@ def mix_and_construct_instance( class_factory_kwargs=factory_kwargs, class_instance_args=instance_args, class_subclass_kwargs=subclass_kwargs, - **subclass_kwargs + **subclass_kwargs, )(*instance_args, **instance_kwargs) diff --git a/pyiron_workflow/snippets/singleton.py b/pyiron_workflow/snippets/singleton.py index 0b58b314..a2f5be6b 100644 --- a/pyiron_workflow/snippets/singleton.py +++ b/pyiron_workflow/snippets/singleton.py @@ -34,6 +34,7 @@ class _RegisteredFactory(metaclass=Singleton): Used under-the-hood in the public facing decorator below. """ + def __init_subclass__(cls, /, factory_function: callable[..., type]): cls.factory_function = staticmethod(factory_function) cls.registry = {} @@ -58,6 +59,7 @@ class _FactoryTown(metaclass=Singleton): A singleton to hold existing factories, so that if we wrap the same factory function more than once, we always get back the same factory. """ + factories = {} def get_registered_factory( @@ -70,7 +72,7 @@ def get_registered_factory( f"{_RegisteredFactory.__name__}{factory_function.__name__.title()}", (_RegisteredFactory,), {}, - factory_function=factory_function + factory_function=factory_function, ) factory = wraps(factory_function)(factory_class()) self.factories[id(factory_function)] = factory From 346e939a451a8c19f24ea5c2e81798008222002b Mon Sep 17 00:00:00 2001 From: liamhuber Date: Sat, 27 Apr 2024 10:18:44 -0700 Subject: [PATCH 10/19] Revert singleton --- pyiron_workflow/snippets/singleton.py | 120 +++----------------------- tests/unit/snippets/test_singleton.py | 100 +-------------------- 2 files changed, 15 insertions(+), 205 deletions(-) diff --git a/pyiron_workflow/snippets/singleton.py b/pyiron_workflow/snippets/singleton.py index a2f5be6b..5f073cd3 100644 --- a/pyiron_workflow/snippets/singleton.py +++ b/pyiron_workflow/snippets/singleton.py @@ -3,13 +3,21 @@ # Distributed under the terms of "New BSD License", see the LICENSE file. """ -Tools for singleton objects. +Utility functions used in pyiron. +In order to be accessible from anywhere in pyiron, they *must* remain free of any imports from pyiron! """ - -from __future__ import annotations - from abc import ABCMeta -from functools import wraps + +__author__ = "Joerg Neugebauer, Jan Janssen" +__copyright__ = ( + "Copyright 2020, Max-Planck-Institut für Eisenforschung GmbH - " + "Computational Materials Design (CM) Department" +) +__version__ = "1.0" +__maintainer__ = "Jan Janssen" +__email__ = "janssen@mpie.de" +__status__ = "production" +__date__ = "Sep 1, 2017" class Singleton(ABCMeta): @@ -25,104 +33,4 @@ class Singleton(ABCMeta): def __call__(cls, *args, **kwargs): if cls not in cls._instances: cls._instances[cls] = super(Singleton, cls).__call__(*args, **kwargs) - return cls._instances[cls] - - -class _RegisteredFactory(metaclass=Singleton): - """ - For making dynamically created classes the same class. - - Used under-the-hood in the public facing decorator below. - """ - - def __init_subclass__(cls, /, factory_function: callable[..., type]): - cls.factory_function = staticmethod(factory_function) - cls.registry = {} - - def __call__(self, *args, **kwargs): - constructed = self.factory_function(*args, **kwargs) - try: - return self.registry[constructed.__name__] - except KeyError: - self.registry[constructed.__name__] = constructed - return constructed - - def __reduce__(self): - return ( - _FactoryTown().get_registered_factory, - (self.factory_function,), - ) - - -class _FactoryTown(metaclass=Singleton): - """ - A singleton to hold existing factories, so that if we wrap the same factory - function more than once, we always get back the same factory. - """ - - factories = {} - - def get_registered_factory( - self, factory_function: callable[..., type] - ) -> type[_RegisteredFactory]: - try: - return self.factories[id(factory_function)] - except KeyError: - factory_class = type( - f"{_RegisteredFactory.__name__}{factory_function.__name__.title()}", - (_RegisteredFactory,), - {}, - factory_function=factory_function, - ) - factory = wraps(factory_function)(factory_class()) - self.factories[id(factory_function)] = factory - return factory - - -def registered_factory(factory_function: callable[..., type]): - """ - A decorator for wrapping class factories. - - Wrapped factories return the _same object_ when they would generate a class with - the same name as they have generated before. I.e. a sort of singleton-class - generator where the classes themselves are singleton, not their instances. - - Args: - factory_function (callabe[..., type]): The class factory to wrap. - - Returns: - (_RegisteredFactory): A factory instance that will return the same class object - whenever the factory method would return a class whose name has been seen - before. - - Example: - >>> from abc import ABC - >>> - >>> from pyiron_workflow.snippets.singleton import registered_factory - >>> - >>> class Foo(ABC): - ... def __init_subclass__(cls, /, n=0, **kwargs): - ... super().__init_subclass__(**kwargs) - ... cls.n = n - >>> - >>> @registered_factory - ... def foo_factory(n): - ... return type( - ... f"{Foo.__name__}{n}", - ... (Foo,), - ... {}, - ... n=n - ... ) - >>> - >>> FooTwo = foo_factory(2) - >>> Foo2 = foo_factory(2) - >>> print(FooTwo.__name__, FooTwo.n) - Foo2 2 - - >>> print(Foo2.__name__, Foo2.n) - Foo2 2 - - >>> print(FooTwo is Foo2) - True - """ - return _FactoryTown().get_registered_factory(factory_function) + return cls._instances[cls] \ No newline at end of file diff --git a/tests/unit/snippets/test_singleton.py b/tests/unit/snippets/test_singleton.py index 2ba85df5..7953445e 100644 --- a/tests/unit/snippets/test_singleton.py +++ b/tests/unit/snippets/test_singleton.py @@ -1,7 +1,5 @@ -from abc import ABC import unittest - -from pyiron_workflow.snippets.singleton import Singleton, registered_factory +from pyiron_workflow.snippets.singleton import Singleton class TestSingleton(unittest.TestCase): @@ -17,101 +15,5 @@ def __init__(self): self.assertEqual(2, f1.x) -class TestRegisteredFactory(unittest.TestCase): - def test_decorator(self): - class Foo(ABC): - def __init_subclass__(cls, /, n=0, **kwargs): - super().__init_subclass__(**kwargs) - cls.n = n - - def foo_factory(n): - """The foo factory docstring.""" - return type( - f"{Foo.__name__}{n}", - (Foo,), - {}, - n=n - ) - - FooTwo = foo_factory(2) - Foo2 = foo_factory(2) - self.assertEqual( - FooTwo.__name__, - Foo2.__name__, - msg="Sanity check" - ) - self.assertIsNot( - FooTwo, - Foo2, - msg="Sanity check" - ) - self.assertEqual( - 2, - Foo2.n, - msg="Sanity check" - ) - - Foo3 = foo_factory(3) - self.assertIsNot( - Foo3, - Foo2, - msg="Sanity check" - ) - - registered_foo_factory = registered_factory(foo_factory) - FooTwo = registered_foo_factory(2) - Foo2 = registered_foo_factory(2) - self.assertEqual( - FooTwo.__name__, - Foo2.__name__, - msg="Sanity check" - ) - self.assertIs( - FooTwo, - Foo2, - msg="The point of the registration is that dynamically generated classes " - "with the same name from the same generator should wind up being the " - "same class" - ) - self.assertEqual( - "The foo factory docstring.", - registered_foo_factory.__doc__, - msg="The wrapper should preserve the factory's docstring" - ) - - re_registered_foo_factory = registered_factory(foo_factory) - self.assertIs( - registered_foo_factory, - re_registered_foo_factory, - msg="The factories themselves are singletons based on the id of the " - "factory function they use; If you register the same factory function " - "twice, you should get the same factory back." - ) - self.assertIs( - re_registered_foo_factory(2), - Foo2, - msg="From the above, it should hold trivially that building the same class " - "from each factory gives the same class." - ) - - @registered_factory - def different_but_similar(n): - """The foo factory docstring.""" - return foo_factory(n) - - Foo2b = different_but_similar(2) - self.assertEqual( - Foo2b.__name__, - Foo2.__name__, - msg="Sanity check. It's the same factory behaviour after all." - ) - self.assertIsNot( - Foo2b, - Foo2, - msg="These come from two separate registered factories, each of which is " - "maintaining its own internal registration list." - ) - - if __name__ == '__main__': unittest.main() From 1adec064caa916e052978ecef37538984a22e5c1 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Sat, 27 Apr 2024 10:29:55 -0700 Subject: [PATCH 11/19] Remove constructed It's superceded by the snippets.factory stuff --- pyiron_workflow/snippets/constructed.py | 150 ------------------------ tests/unit/snippets/test_constructed.py | 107 ----------------- 2 files changed, 257 deletions(-) delete mode 100644 pyiron_workflow/snippets/constructed.py delete mode 100644 tests/unit/snippets/test_constructed.py diff --git a/pyiron_workflow/snippets/constructed.py b/pyiron_workflow/snippets/constructed.py deleted file mode 100644 index 91914f59..00000000 --- a/pyiron_workflow/snippets/constructed.py +++ /dev/null @@ -1,150 +0,0 @@ -from __future__ import annotations - -from abc import ABC - - -class Constructed(ABC): - """ - A mixin which overrides `__reduce__` to return a constructor callable and arguments. - - This is useful for classes which are not importable, but who have constructor - functions that _are_, e.g. classes dynamically created classes. - - Unlike overriding `__new__`, no changes are needed for the `__init__` signature. - - Child classes must define the :attr:`_class_factory`, as well as - :attr:`_class_factory_args` and :attr:`_class_instance_args` (if either the factory - or returned class's `__init__` take positional arguments), and may optionally - specify :attr:`_class_factory_kwargs` and/or :attr:`_class_instance_kwargs` if - either callable has keyword arguments that should be specified. - In the case that the mixin is being added dynamically at instantiation, subclass - kwargs need to be re-specified in :attr:`_class_subclass_kwargs`. - All of these parameters should be specified at the time of subclassing (via - :meth:`__init_subclass__`) using keywords of the same (but public) names. - """ - - def __init_subclass__( - cls, - /, - class_factory: callable = None, - class_factory_args: tuple = (), - class_factory_kwargs: dict | None = None, - class_instance_args: tuple = (), - class_instance_kwargs: dict | None = None, - class_subclass_kwargs: dict | None = None, - **kwargs, - ): - super(Constructed, cls).__init_subclass__(**kwargs) - - cls._class_factory = staticmethod(class_factory) - if class_factory is not None: - # Intermediate abstract classes may not yet specify a factory - cls.__module__ = class_factory.__module__ - - cls._class_factory_args = class_factory_args - cls._class_factory_kwargs = ( - {} if class_factory_kwargs is None else class_factory_kwargs - ) - cls._class_instance_args = class_instance_args - cls._class_instance_kwargs = ( - {} if class_instance_kwargs is None else class_instance_kwargs - ) - cls._class_subclass_kwargs = ( - {} if class_subclass_kwargs is None else class_subclass_kwargs - ) - - @staticmethod - def _instance_constructor( - class_factory, - factory_args, - factory_kwargs, - instance_args, - instance_kwargs, - subclass_kwargs, - ) -> callable[[...], Constructed]: - """ - A constructor function returning an instance of the class. - - Args: - class_factory (callable): A method returning the class - factory_args (tuple): Args to pass to the factory method. - factory_kwargs (dict): Kwargs to pass to the factory method. - instance_args (tuple): Args to pass to the new instance. - instance_kwargs (dict): Kwargs to pass to the new instance. - subclass_kwargs (dict): Kwargs to pass to `type` when this method is - overridden by :func:`mix_and_construct_instance`. - - Returns: - (Constructed): A new instance of the factory result mixed with - :class:`Constructed`. - """ - return class_factory(*factory_args, **factory_kwargs)( - *instance_args, **instance_kwargs - ) - - def __reduce__(self): - return ( - self._instance_constructor, - ( - self._class_factory, - self._class_factory_args, - self._class_factory_kwargs, - self._class_instance_args, # Args may be _necessary_ for __init__ - self._class_instance_kwargs, - self._class_subclass_kwargs, - ), - self.__getstate__(), - ) - - def __getstate__(self): - # Backwards compatibility - try: - return super().__getstate__() - except AttributeError: - return dict(self.__dict__) - - def __setstate__(self, state): - # Backwards compatibility - try: - super().__setstate__(state) - except AttributeError: - self.__dict__.update(**state) - - -def mix_and_construct_instance( - class_factory, - factory_args, - factory_kwargs, - instance_args, - instance_kwargs, - subclass_kwargs, -): - """ - A constructor function that dynamically mixes in :class:`Constructed` inheritance. - - Args: - class_factory (callable): A method returning a new class, i.e. `return type...`. - factory_args (tuple): Args to pass to the factory method. - factory_kwargs (dict): Kwargs to pass to the factory method. - instance_args (tuple): Args to pass to the new instance. - instance_kwargs (dict): Kwargs to pass to the new instance. - subclass_kwargs (dict): Kwargs to pass to `type` to make sure subclassing - doesn't lose info passed to the factory by getting overwritten by the - superclass's `__init_subclass__` default values. - - Returns: - (Constructed): A new instance of the factory result mixed with - :class:`Constructed`. - """ - base_class = class_factory(*factory_args, **factory_kwargs) - return type( - Constructed.__name__ + base_class.__name__, - (Constructed, base_class), - {"_instance_constructor": staticmethod(mix_and_construct_instance)}, - class_factory=class_factory, - class_factory_args=factory_args, - class_factory_kwargs=factory_kwargs, - class_instance_args=instance_args, - class_subclass_kwargs=subclass_kwargs, - **subclass_kwargs, - )(*instance_args, **instance_kwargs) diff --git a/tests/unit/snippets/test_constructed.py b/tests/unit/snippets/test_constructed.py deleted file mode 100644 index 4c7fb8c3..00000000 --- a/tests/unit/snippets/test_constructed.py +++ /dev/null @@ -1,107 +0,0 @@ -from abc import ABC -import pickle -import unittest - -from pyiron_workflow.snippets.constructed import Constructed, mix_and_construct_instance - - -class HasN(ABC): - def __init_subclass__(cls, /, n=0, s="foo", **kwargs): - super(HasN, cls).__init_subclass__(**kwargs) - cls.n = n - cls.s = s - - def __init__(self, x, y=0): - self.x = x - self.y = y - - -def has_n_factory(n, s="bar"): - return type( - f"{HasN.__name__}{n}", - (HasN,), - {"__module__": HasN.__module__}, - n=n, - s=s - ) - - -class Constructable(Constructed, HasN, ABC): - pass - - -def constructable_factory(n, s="baz"): - return type( - f"{Constructable.__name__}{n}", - (Constructable,), - {}, - n=n, - s=s, - class_factory=constructable_factory, - class_factory_args=(n,), - class_factory_kwargs={"s": s}, - class_instance_args=(0,) - ) - - -class TestConstructed(unittest.TestCase): - def test_reduce(self): - by_inheritance = constructable_factory(1)(2) - constructor, constructor_args, state = by_inheritance.__reduce__() - f, f_args, f_kwargs, i_args, i_kwargs, sc_kwargs = constructor_args - - self.assertIs(Constructed._instance_constructor, constructor) - self.assertIs(constructable_factory, f) - self.assertTupleEqual((1,), f_args) - self.assertDictEqual({"s": "baz"}, f_kwargs) # from the factory defaults - self.assertTupleEqual((0,), i_args) # from the factory - self.assertDictEqual({}, i_kwargs) # from Constructed - self.assertDictEqual({}, sc_kwargs) # from Constructed - - def test_inheritance_mixin(self): - by_inheritance = constructable_factory(42)(43, y=44) - print(by_inheritance.__getstate__()) - self.assertTupleEqual( - (42, "baz", 43, 44), - (by_inheritance.n, by_inheritance.s, by_inheritance.x, by_inheritance.y), - msg="Sanity check." - ) - reloaded = pickle.loads(pickle.dumps(by_inheritance)) - self.assertTupleEqual( - (by_inheritance.n, by_inheritance.s, by_inheritance.x, by_inheritance.y), - (reloaded.n, reloaded.s, reloaded.x, reloaded.y), - msg=f"Children of {Constructed.__name__} should recover both class and " - f"instance state under the (un)pickle cycle." - ) - - def test_instantiation_mixin(self): - dynamic_mixin = mix_and_construct_instance( - has_n_factory, - (42,), - {"s": "baz"}, - (43,), - {"y": 44}, - {"n": 42, "s": "baz"}, # __init_subclass__ kwargs get duplicated here - # This is annoying for users of `mix_and_construct_instance`, but not - # difficult. - ) - self.assertIsInstance( - dynamic_mixin, - Constructed, - msg=f"{mix_and_construct_instance.__name__} should dynamically add " - f"{Constructed.__name__} inheritance." - ) - self.assertTupleEqual( - (42, "baz", 43, 44), - (dynamic_mixin.n, dynamic_mixin.s, dynamic_mixin.x, dynamic_mixin.y), - msg="Sanity check." - ) - - reloaded = pickle.loads(pickle.dumps(dynamic_mixin)) - self.assertTupleEqual( - (dynamic_mixin.n, dynamic_mixin.s, dynamic_mixin.x, dynamic_mixin.y), - (reloaded.n, reloaded.s, reloaded.x, reloaded.y), - msg=f"Children of {Constructed.__name__} should recover both class and " - f"instance state under the (un)pickle cycle." - ) - From 9c68c2ecce90b5e9d9443ec73b71f47b77413714 Mon Sep 17 00:00:00 2001 From: pyiron-runner Date: Sat, 27 Apr 2024 17:24:03 +0000 Subject: [PATCH 12/19] Format black --- pyiron_workflow/snippets/singleton.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyiron_workflow/snippets/singleton.py b/pyiron_workflow/snippets/singleton.py index 5f073cd3..5b8b85d0 100644 --- a/pyiron_workflow/snippets/singleton.py +++ b/pyiron_workflow/snippets/singleton.py @@ -33,4 +33,4 @@ class Singleton(ABCMeta): def __call__(cls, *args, **kwargs): if cls not in cls._instances: cls._instances[cls] = super(Singleton, cls).__call__(*args, **kwargs) - return cls._instances[cls] \ No newline at end of file + return cls._instances[cls] From 0e87a0ca4966c582d28fbc55263d3426d9b99899 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Sun, 28 Apr 2024 14:19:35 -0700 Subject: [PATCH 13/19] Let the factory clear method take specific names --- pyiron_workflow/snippets/factory.py | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/pyiron_workflow/snippets/factory.py b/pyiron_workflow/snippets/factory.py index 69c8797d..e1005175 100644 --- a/pyiron_workflow/snippets/factory.py +++ b/pyiron_workflow/snippets/factory.py @@ -160,14 +160,31 @@ def __call__(self, *args) -> type[_FactoryMade]: return factory_made @classmethod - def clear(cls): + def clear(cls, *class_names, skip_missing=True): """ - Remove constructed classes. + Remove constructed class(es). Can be useful if you've updated the constructor and want to remove old instances. + + Args: + *class_names (str): The names of classes to remove. Removes all of them + when empty. + skip_missing (bool): Whether to pass over key errors when a name is + requested that is not currently in the class registry. (Default is + True, let missing names pass silently.) """ - cls.class_registry = {} + if len(class_names) == 0: + cls.class_registry = {} + else: + for name in class_names: + try: + cls.class_registry.pop(name) + except KeyError as e: + if skip_missing: + continue + else: + raise KeyError(f"Could not find class {name}") def _build_class( self, name, bases, class_dict, sc_init_kwargs, class_factory_args From 6f1880cf4ae773e12dddd355e657bd458f6b5769 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Sun, 28 Apr 2024 21:37:58 -0700 Subject: [PATCH 14/19] Don't override __module__ to the factory function If it was explicitly set downstream, leave that. But if the user left it empty, still default it back to the factory function's module --- pyiron_workflow/snippets/factory.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pyiron_workflow/snippets/factory.py b/pyiron_workflow/snippets/factory.py index e1005175..475954d9 100644 --- a/pyiron_workflow/snippets/factory.py +++ b/pyiron_workflow/snippets/factory.py @@ -190,7 +190,8 @@ def _build_class( self, name, bases, class_dict, sc_init_kwargs, class_factory_args ) -> type[_FactoryMade]: - class_dict["__module__"] = self.factory_function.__module__ + if "__module__" not in class_dict.keys(): + class_dict["__module__"] = self.factory_function.__module__ sc_init_kwargs["class_factory"] = self sc_init_kwargs["class_factory_args"] = class_factory_args From 42181e0db412882250e4a23a47600dd3c28b0286 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Mon, 29 Apr 2024 13:16:52 -0700 Subject: [PATCH 15/19] Clean up storage if job tests fail --- tests/unit/test_job.py | 119 +++++++++++++++++++++++------------------ 1 file changed, 66 insertions(+), 53 deletions(-) diff --git a/tests/unit/test_job.py b/tests/unit/test_job.py index b1a93a63..af82a2f5 100644 --- a/tests/unit/test_job.py +++ b/tests/unit/test_job.py @@ -181,12 +181,18 @@ def test_clean_failure(self): def test_node(self): node = Workflow.create.standard.UserInput(42) nj = self.make_a_job_from_node(node) - nj.run() - self.assertEqual( - 42, - nj.node.outputs.user_input.value, - msg="A single node should run just as well as a workflow" - ) + try: + nj.run() + self.assertEqual( + 42, + nj.node.outputs.user_input.value, + msg="A single node should run just as well as a workflow" + ) + finally: + try: + node.storage.delete() + except FileNotFoundError: + pass @unittest.skipIf(sys.version_info < (3, 11), "Storage will only work in 3.11+") def test_modal(self): @@ -195,29 +201,32 @@ def test_modal(self): modal_wf.out = modal_wf.create.standard.UserInput(modal_wf.sleep) nj = self.make_a_job_from_node(modal_wf) - nj.run() - self.assertTrue( - nj.status.finished, - msg="The interpreter should not release until the job is done" - ) - self.assertEqual( - 0, - nj.node.outputs.out__user_input.value, - msg="The node should have run, and since it's modal there's no need to " - "update the instance" - ) - - lj = self.pr.load(nj.job_name) - self.assertIsNot( - lj, - nj, - msg="The loaded job should be a new instance." - ) - self.assertEqual( - nj.node.outputs.out__user_input.value, - lj.node.outputs.out__user_input.value, - msg="The loaded job should still have all the same values" - ) + try: + nj.run() + self.assertTrue( + nj.status.finished, + msg="The interpreter should not release until the job is done" + ) + self.assertEqual( + 0, + nj.node.outputs.out__user_input.value, + msg="The node should have run, and since it's modal there's no need to " + "update the instance" + ) + + lj = self.pr.load(nj.job_name) + self.assertIsNot( + lj, + nj, + msg="The loaded job should be a new instance." + ) + self.assertEqual( + nj.node.outputs.out__user_input.value, + lj.node.outputs.out__user_input.value, + msg="The loaded job should still have all the same values" + ) + finally: + modal_wf.storage.delete() @unittest.skipIf(sys.version_info < (3, 11), "Storage will only work in 3.11+") def test_nonmodal(self): @@ -225,31 +234,35 @@ def test_nonmodal(self): nonmodal_node.out = Workflow.create.standard.UserInput(42) nj = self.make_a_job_from_node(nonmodal_node) - nj.run(run_mode="non_modal") - self.assertFalse( - nj.status.finished, - msg=f"The local process should released immediately per non-modal " - f"style, but got status {nj.status}" - ) - while not nj.status.finished: - sleep(0.1) - self.assertTrue( - nj.status.finished, - msg="The job status should update on completion" - ) - self.assertIs( - nj.node.outputs.out__user_input.value, - NOT_DATA, - msg="As usual with remote processes, we expect to require a data read " - "before the local instance reflects its new state." - ) - lj = self.pr.load(nj.job_name) - self.assertEqual( - 42, - lj.node.outputs.out__user_input.value, - msg="The loaded job should have the finished values" - ) + try: + nj.run(run_mode="non_modal") + self.assertFalse( + nj.status.finished, + msg=f"The local process should released immediately per non-modal " + f"style, but got status {nj.status}" + ) + while not nj.status.finished: + sleep(0.1) + self.assertTrue( + nj.status.finished, + msg="The job status should update on completion" + ) + self.assertIs( + nj.node.outputs.out__user_input.value, + NOT_DATA, + msg="As usual with remote processes, we expect to require a data read " + "before the local instance reflects its new state." + ) + + lj = self.pr.load(nj.job_name) + self.assertEqual( + 42, + lj.node.outputs.out__user_input.value, + msg="The loaded job should have the finished values" + ) + finally: + nonmodal_node.storage.delete() @unittest.skipIf(sys.version_info < (3, 11), "Storage will only work in 3.11+") def test_bad_workflow(self): From 4779ba7382ec28b72ccc7199b981d62540735730 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Mon, 29 Apr 2024 13:40:59 -0700 Subject: [PATCH 16/19] Make tinybase the default storage backend --- pyiron_workflow/node_library/standard.py | 8 ++++++++ pyiron_workflow/storage.py | 8 ++++---- tests/unit/test_job.py | 10 ++-------- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/pyiron_workflow/node_library/standard.py b/pyiron_workflow/node_library/standard.py index d091b8fe..8a3ab85c 100644 --- a/pyiron_workflow/node_library/standard.py +++ b/pyiron_workflow/node_library/standard.py @@ -5,6 +5,7 @@ from __future__ import annotations import random +from time import sleep from pyiron_workflow.channels import NOT_DATA, OutputSignal from pyiron_workflow.function import Function, as_function_node @@ -52,6 +53,12 @@ def RandomFloat(): return random.random() +@as_function_node("time") +def Sleep(t): + sleep(t) + return t + + @as_function_node("slice") def Slice(start=None, stop=NOT_DATA, step=None): if start is None: @@ -291,6 +298,7 @@ def Round(obj): RandomFloat, RightMultiply, Round, + Sleep, Slice, String, Subtract, diff --git a/pyiron_workflow/storage.py b/pyiron_workflow/storage.py index 95a6a30c..bb93f227 100644 --- a/pyiron_workflow/storage.py +++ b/pyiron_workflow/storage.py @@ -372,10 +372,6 @@ def _storage_interfaces(cls): interfaces["h5io"] = H5ioStorage return interfaces - @classmethod - def default_backend(cls): - return "h5io" - class HasTinybaseStorage(HasStorage, ABC): @classmethod @@ -391,3 +387,7 @@ def to_storage(self, storage: TinybaseStorage): @abstractmethod def from_storage(self, storage: TinybaseStorage): pass + + @classmethod + def default_backend(cls): + return "tinybase" diff --git a/tests/unit/test_job.py b/tests/unit/test_job.py index af82a2f5..068ba027 100644 --- a/tests/unit/test_job.py +++ b/tests/unit/test_job.py @@ -9,12 +9,6 @@ import pyiron_workflow.job # To get the job classes registered -@Workflow.wrap.as_function_node("t") -def Sleep(t): - sleep(t) - return t - - class _WithAJob(unittest.TestCase, ABC): @abstractmethod def make_a_job_from_node(self, node): @@ -58,7 +52,7 @@ def test_node(self): @unittest.skipIf(sys.version_info < (3, 11), "Storage will only work in 3.11+") def test_modal(self): modal_wf = Workflow("modal_wf") - modal_wf.sleep = Sleep(0) + modal_wf.sleep = Workflow.create.standard.Sleep(0) modal_wf.out = modal_wf.create.standard.UserInput(modal_wf.sleep) nj = self.make_a_job_from_node(modal_wf) @@ -197,7 +191,7 @@ def test_node(self): @unittest.skipIf(sys.version_info < (3, 11), "Storage will only work in 3.11+") def test_modal(self): modal_wf = Workflow("modal_wf") - modal_wf.sleep = Sleep(0) + modal_wf.sleep = Workflow.create.standard.Sleep(0) modal_wf.out = modal_wf.create.standard.UserInput(modal_wf.sleep) nj = self.make_a_job_from_node(modal_wf) From 6d064dbb0f571dab26c7a322cee55d0e1be6d3a1 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Mon, 29 Apr 2024 14:28:08 -0700 Subject: [PATCH 17/19] Switch Function and Macro over to using classfactory With this, everything is pickleable (unless you slap something unpickleable on top, or define it in a place that can't be reached by pickle like inside a local function scope). The big downside is that `h5io` storage is now basically useless, since all our nodes come from custom reconstructors. Similarly, for the node job `DataContainer` can no longer store the input node. The `tinybase` backend is still working ok, so I made it the default, and I got the node job working again by forcing it to cloudpickle the input node on saving. These are some ugly hacks, but since storage is an alpha feature right now anyhow, I'd prefer to push ahead with pickleability. --- pyiron_workflow/function.py | 100 ++++++++----------- pyiron_workflow/io_preview.py | 10 +- pyiron_workflow/job.py | 33 ++++++- pyiron_workflow/macro.py | 175 +++++++++++++++++++++------------- pyiron_workflow/node.py | 28 +++--- tests/unit/test_function.py | 11 +++ tests/unit/test_job.py | 31 ++++-- tests/unit/test_macro.py | 114 ++++++++++++++-------- tests/unit/test_workflow.py | 92 ++++++++++++------ 9 files changed, 377 insertions(+), 217 deletions(-) diff --git a/pyiron_workflow/function.py b/pyiron_workflow/function.py index cc3295ce..6f749bb3 100644 --- a/pyiron_workflow/function.py +++ b/pyiron_workflow/function.py @@ -1,16 +1,14 @@ from __future__ import annotations from abc import ABC, abstractmethod -from typing import Any, Literal, Optional, TYPE_CHECKING +from typing import Any -from pyiron_workflow.io_preview import DecoratedNode, decorated_node_decorator_factory +from pyiron_workflow.io_preview import StaticNode, ScrapesIO from pyiron_workflow.snippets.colors import SeabornColors +from pyiron_workflow.snippets.factory import classfactory -if TYPE_CHECKING: - from pyiron_workflow.composite import Composite - -class Function(DecoratedNode, ABC): +class Function(StaticNode, ScrapesIO, ABC): """ Function nodes wrap an arbitrary python function. @@ -347,64 +345,50 @@ def color(self) -> str: return SeabornColors.green -as_function_node = decorated_node_decorator_factory(Function, Function.node_function) - - -def function_node( - node_function: callable, - *args, - label: Optional[str] = None, - parent: Optional[Composite] = None, - overwrite_save: bool = False, - run_after_init: bool = False, - storage_backend: Optional[Literal["h5io", "tinybase"]] = None, - save_after_run: bool = False, - output_labels: Optional[str | tuple[str]] = None, - validate_output_labels: bool = True, - **kwargs, +@classfactory +def function_node_factory( + node_function: callable, validate_output_labels: bool, /, *output_labels ): - """ - Dynamically creates a new child of :class:`Function` using the - provided :func:`node_function` and returns an instance of that. - - Beyond the standard :class:`Function`, initialization allows the args... - - Args: - node_function (callable): The function determining the behaviour of the node. - output_labels (Optional[str | list[str] | tuple[str]]): A name for each return - value of the node function OR a single label. (Default is None, which - scrapes output labels automatically from the source code of the wrapped - function.) This can be useful when returned values are not well named, e.g. - to make the output channel dot-accessible if it would otherwise have a label - that requires item-string-based access. Additionally, specifying a _single_ - label for a wrapped function that returns a tuple of values ensures that a - _single_ output channel (holding the tuple) is created, instead of one - channel for each return value. The default approach of extracting labels - from the function source code also requires that the function body contain - _at most_ one `return` expression, so providing explicit labels can be used - to circumvent this (at your own risk), or to circumvent un-inspectable - source code (e.g. a function that exists only in memory). - """ + return ( + node_function.__name__, + (Function,), # Define parentage + { + "node_function": staticmethod(node_function), + "__module__": node_function.__module__, + "_output_labels": None if len(output_labels) == 0 else output_labels, + "_validate_output_labels": validate_output_labels, + }, + {}, + ) - if not callable(node_function): - raise AttributeError( - f"Expected `node_function` to be callable but got {node_function}" + +def as_function_node(*output_labels, validate_output_labels=True): + def decorator(node_function): + function_node_factory.clear(node_function.__name__) # Force a fresh class + factory_made = function_node_factory( + node_function, validate_output_labels, *output_labels ) + factory_made._class_returns_from_decorated_function = node_function + factory_made.preview_io() + return factory_made + return decorator + +def function_node( + node_function, + *node_args, + output_labels=None, + validate_output_labels=True, + **node_kwargs +): if output_labels is None: output_labels = () elif isinstance(output_labels, str): output_labels = (output_labels,) - - return as_function_node( - *output_labels, validate_output_labels=validate_output_labels - )(node_function)( - *args, - label=label, - parent=parent, - overwrite_save=overwrite_save, - run_after_init=run_after_init, - storage_backend=storage_backend, - save_after_run=save_after_run, - **kwargs, + function_node_factory.clear(node_function.__name__) # Force a fresh class + factory_made = function_node_factory( + node_function, validate_output_labels, *output_labels ) + factory_made.preview_io() + return factory_made(*node_args, **node_kwargs) + diff --git a/pyiron_workflow/io_preview.py b/pyiron_workflow/io_preview.py index bc8a85e8..46a4e323 100644 --- a/pyiron_workflow/io_preview.py +++ b/pyiron_workflow/io_preview.py @@ -18,7 +18,9 @@ from functools import lru_cache, wraps from textwrap import dedent from types import FunctionType -from typing import Any, get_args, get_type_hints, Literal, Optional, TYPE_CHECKING +from typing import ( + Any, ClassVar, get_args, get_type_hints, Literal, Optional, TYPE_CHECKING +) from pyiron_workflow.channels import InputData, NOT_DATA from pyiron_workflow.injection import OutputDataWithInjection, OutputsWithInjection @@ -130,9 +132,9 @@ class ScrapesIO(HasIOPreview, ABC): def _io_defining_function(cls) -> callable: """Must return a static class method.""" - _output_labels: tuple[str] | None = None # None: scrape them - _validate_output_labels: bool = True # True: validate against source code - _io_defining_function_uses_self: bool = False # False: use entire signature + _output_labels: ClassVar[tuple[str] | None] = None # None: scrape them + _validate_output_labels: ClassVar[bool] = True # True: validate against source code + _io_defining_function_uses_self: ClassVar[bool] = False # False: use entire signature @classmethod def _build_inputs_preview(cls): diff --git a/pyiron_workflow/job.py b/pyiron_workflow/job.py index 8a220d86..1b825dd2 100644 --- a/pyiron_workflow/job.py +++ b/pyiron_workflow/job.py @@ -20,10 +20,13 @@ from __future__ import annotations +import base64 import inspect import os import sys +import cloudpickle + from pyiron_base import TemplateJob, JOB_CLASS_DICT from pyiron_base.jobs.flex.pythonfunctioncontainer import ( PythonFunctionContainerJob, @@ -103,25 +106,51 @@ def validate_ready_to_run(self): f"Node not ready:{nl}{self.input['node'].readiness_report}" ) + def save(self): + # DataContainer can't handle custom reconstructors, so convert the node to + # bytestream + self.input["node"] = base64.b64encode( + cloudpickle.dumps(self.input["node"]) + ).decode("utf-8") + super().save() + def run_static(self): # Overrides the parent method # Copy and paste except for the output update, which makes sure the output is # flat and not tested beneath "result" + + # Unpack the node + input_dict = self.input.to_builtin() + input_dict["node"] = cloudpickle.loads(base64.b64decode(self.input["node"])) + if ( self._executor_type is not None and "executor" in inspect.signature(self._function).parameters.keys() ): - input_dict = self.input.to_builtin() del input_dict["executor"] output = self._function( **input_dict, executor=self._get_executor(max_workers=self.server.cores) ) else: - output = self._function(**self.input.to_builtin()) + output = self._function(**input_dict) self.output.update(output) # DIFFERS FROM PARENT METHOD self.to_hdf() self.status.finished = True + def get_input_node(self): + """ + On saving, we turn the input node into a bytestream so that the DataContainer + can store it. You might want to look at it again though, so you can use this + to unpack it + + Returns: + (Node): The input node as a node again + """ + if isinstance(self.input["node"], Node): + return self.input["node"] + else: + return cloudpickle.loads(base64.b64decode(self.input["node"])) + JOB_CLASS_DICT[NodeOutputJob.__name__] = NodeOutputJob.__module__ diff --git a/pyiron_workflow/macro.py b/pyiron_workflow/macro.py index e1798f7d..fbbdabfa 100644 --- a/pyiron_workflow/macro.py +++ b/pyiron_workflow/macro.py @@ -12,13 +12,14 @@ from pyiron_workflow.composite import Composite from pyiron_workflow.has_interface_mixins import HasChannel from pyiron_workflow.io import Outputs, Inputs -from pyiron_workflow.io_preview import DecoratedNode, decorated_node_decorator_factory +from pyiron_workflow.io_preview import StaticNode, ScrapesIO +from pyiron_workflow.snippets.factory import classfactory if TYPE_CHECKING: from pyiron_workflow.channels import Channel -class Macro(Composite, DecoratedNode, ABC): +class Macro(Composite, StaticNode, ScrapesIO, ABC): """ A macro is a composite node that holds a graph with a fixed interface, like a pre-populated workflow that is the same every time you instantiate it. @@ -470,74 +471,120 @@ def __setstate__(self, state): self.children[child].outputs[child_out].value_receiver = self.outputs[out] -as_macro_node = decorated_node_decorator_factory( - Macro, - Macro.graph_creator, - decorator_docstring_additions="The first argument in the wrapped function is " - "`self`-like and will receive the macro instance " - "itself, and thus is ignored in the IO.", -) +@classfactory +def macro_node_factory(graph_creator: callable, validate_output_labels: bool, /, *output_labels +): + return ( + graph_creator.__name__, + (Macro,), # Define parentage + { + "graph_creator": staticmethod(graph_creator), + "__module__": graph_creator.__module__, + "_output_labels": None if len(output_labels) == 0 else output_labels, + "_validate_output_labels": validate_output_labels, + }, + {}, + ) -def macro_node( - graph_creator, - label: Optional[str] = None, - parent: Optional[Composite] = None, - overwrite_save: bool = False, - run_after_init: bool = False, - storage_backend: Optional[Literal["h5io", "tinybase"]] = None, - save_after_run: bool = False, - strict_naming: bool = True, - output_labels: Optional[str | list[str] | tuple[str]] = None, - validate_output_labels: bool = True, - **kwargs, -): - """ - Creates a new child of :class:`Macro` using the provided - :func:`graph_creator` and returns an instance of that. - - Quacks like a :class:`Composite` for the sake of creating and registering nodes. - - Beyond the standard :class:`Macro`, initialization allows the args... - - Args: - graph_creator (callable): The function defining macro's graph. - output_labels (Optional[str | list[str] | tuple[str]]): A name for each return - value of the node function OR a single label. (Default is None, which - scrapes output labels automatically from the source code of the wrapped - function.) This can be useful when returned values are not well named, e.g. - to make the output channel dot-accessible if it would otherwise have a label - that requires item-string-based access. Additionally, specifying a _single_ - label for a wrapped function that returns a tuple of values ensures that a - _single_ output channel (holding the tuple) is created, instead of one - channel for each return value. The default approach of extracting labels - from the function source code also requires that the function body contain - _at most_ one `return` expression, so providing explicit labels can be used - to circumvent this (at your own risk), or to circumvent un-inspectable - source code (e.g. a function that exists only in memory). - """ - if not callable(graph_creator): - # `function_node` quacks like a class, even though it's a function and - # dynamically creates children of `Macro` by providing the necessary - # callable to the decorator - raise AttributeError( - f"Expected `graph_creator` to be callable but got {graph_creator}" +def as_macro_node(*output_labels, validate_output_labels=True): + def decorator(node_function): + macro_node_factory.clear(node_function.__name__) # Force a fresh class + factory_made = macro_node_factory( + node_function, validate_output_labels, *output_labels ) + factory_made._class_returns_from_decorated_function = node_function + factory_made.preview_io() + return factory_made + return decorator + +def macro_node( + node_function, + *node_args, + output_labels=None, + validate_output_labels=True, + **node_kwargs +): if output_labels is None: output_labels = () elif isinstance(output_labels, str): output_labels = (output_labels,) - - return as_macro_node(*output_labels, validate_output_labels=validate_output_labels)( - graph_creator - )( - label=label, - parent=parent, - overwrite_save=overwrite_save, - run_after_init=run_after_init, - storage_backend=storage_backend, - save_after_run=save_after_run, - strict_naming=strict_naming, - **kwargs, + macro_node_factory.clear(node_function.__name__) # Force a fresh class + factory_made = macro_node_factory( + node_function, validate_output_labels, *output_labels ) + factory_made.preview_io() + return factory_made(*node_args, **node_kwargs) + +# as_macro_node = decorated_node_decorator_factory( +# Macro, +# Macro.graph_creator, +# decorator_docstring_additions="The first argument in the wrapped function is " +# "`self`-like and will receive the macro instance " +# "itself, and thus is ignored in the IO.", +# ) +# +# +# def macro_node( +# graph_creator, +# label: Optional[str] = None, +# parent: Optional[Composite] = None, +# overwrite_save: bool = False, +# run_after_init: bool = False, +# storage_backend: Optional[Literal["h5io", "tinybase"]] = None, +# save_after_run: bool = False, +# strict_naming: bool = True, +# output_labels: Optional[str | list[str] | tuple[str]] = None, +# validate_output_labels: bool = True, +# **kwargs, +# ): +# """ +# Creates a new child of :class:`Macro` using the provided +# :func:`graph_creator` and returns an instance of that. +# +# Quacks like a :class:`Composite` for the sake of creating and registering nodes. +# +# Beyond the standard :class:`Macro`, initialization allows the args... +# +# Args: +# graph_creator (callable): The function defining macro's graph. +# output_labels (Optional[str | list[str] | tuple[str]]): A name for each return +# value of the node function OR a single label. (Default is None, which +# scrapes output labels automatically from the source code of the wrapped +# function.) This can be useful when returned values are not well named, e.g. +# to make the output channel dot-accessible if it would otherwise have a label +# that requires item-string-based access. Additionally, specifying a _single_ +# label for a wrapped function that returns a tuple of values ensures that a +# _single_ output channel (holding the tuple) is created, instead of one +# channel for each return value. The default approach of extracting labels +# from the function source code also requires that the function body contain +# _at most_ one `return` expression, so providing explicit labels can be used +# to circumvent this (at your own risk), or to circumvent un-inspectable +# source code (e.g. a function that exists only in memory). +# """ +# if not callable(graph_creator): +# # `function_node` quacks like a class, even though it's a function and +# # dynamically creates children of `Macro` by providing the necessary +# # callable to the decorator +# raise AttributeError( +# f"Expected `graph_creator` to be callable but got {graph_creator}" +# ) +# +# if output_labels is None: +# output_labels = () +# elif isinstance(output_labels, str): +# output_labels = (output_labels,) +# +# return as_macro_node(*output_labels, validate_output_labels=validate_output_labels)( +# graph_creator +# )( +# label=label, +# parent=parent, +# overwrite_save=overwrite_save, +# run_after_init=run_after_init, +# storage_backend=storage_backend, +# save_after_run=save_after_run, +# strict_naming=strict_naming, +# **kwargs, +# ) diff --git a/pyiron_workflow/node.py b/pyiron_workflow/node.py index 3faba287..453fcb18 100644 --- a/pyiron_workflow/node.py +++ b/pyiron_workflow/node.py @@ -126,18 +126,19 @@ class Node( - Nodes created from a registered package store their package identifier as a class attribute. - [ALPHA FEATURE] Nodes can be saved to and loaded from file if python >= 3.11. + - As long as you haven't put anything unpickleable on them, or defined them in + an unpicklable place (e.g. in the `` of another function), you can + simple (un)pickle nodes. There is no save/load interface for this right + now, just import pickle and do it. - Saving is triggered manually, or by setting a flag to save after the nodes runs. - - On instantiation, nodes will load automatically if they find saved content. + - At the end of instantiation, nodes will load automatically if they find saved + content. - Discovered content can instead be deleted with a kwarg. - You can't load saved content _and_ run after instantiation at once. - - The nodes must be somewhere importable, and the imported object must match - the type of the node being saved. This basically just rules out one edge - case where a node class is defined like - `SomeFunctionNode = Workflow.wrap.as_function_node()(some_function)`, since - then the new class gets the name `some_function`, which when imported is - the _function_ "some_function" and not the desired class "SomeFunctionNode". - This is checked for at save-time and will cause a nice early failure. + - The nodes must be defined somewhere importable, i.e. in a module, `__main__`, + and as a class property are all fine, but, e.g., inside the `` of + another function is not. - [ALPHA ISSUE] If the source code (cells, `.py` files...) for a saved graph is altered between saving and loading the graph, there are no guarantees about the loaded state; depending on the nature of the changes everything may @@ -152,9 +153,14 @@ class Node( the entire graph may be saved at once. - [ALPHA ISSUE] There are two possible back-ends for saving: one leaning on `tinybase.storage.GenericStorage` (in practice, - `H5ioStorage(GenericStorage)`), and the other, default back-end that uses - the `h5io` module directly. The backend used is always the one on the graph - root. + `H5ioStorage(GenericStorage)`), that is the default, and the other that + uses the `h5io` module directly. The backend used is always the one on the + graph root. + - [ALPHA ISSUE] The `h5io` backend is deprecated -- it can't handle custom + reconstructors (i.e. when `__reduce__` returns a tuple with some + non-standard callable as its first entry), and basically all our nodes do + that now! `tinybase` gets around this by falling back on `cloudpickle` when + its own interactions with `h5io` fail. - [ALPHA ISSUE] Restrictions on data: - For the `h5io` backend: Most data that can be pickled will be fine, but some classes will hit an edge case and throw an exception from `h5io` diff --git a/tests/unit/test_function.py b/tests/unit/test_function.py index 1891b224..7d922d1f 100644 --- a/tests/unit/test_function.py +++ b/tests/unit/test_function.py @@ -1,3 +1,4 @@ +import pickle from typing import Optional, Union import unittest @@ -505,6 +506,16 @@ def NoReturn(x): # Honestly, functions with no return should probably be made illegal to # encourage functional setups... + def test_pickle(self): + n = function_node(plus_one, 5, output_labels="p1") + n() + reloaded = pickle.loads(pickle.dumps(n)) + self.assertListEqual(n.outputs.labels, reloaded.outputs.labels) + self.assertDictEqual( + n.outputs.to_value_dict(), + reloaded.outputs.to_value_dict() + ) + if __name__ == '__main__': unittest.main() diff --git a/tests/unit/test_job.py b/tests/unit/test_job.py index 068ba027..9baffa0c 100644 --- a/tests/unit/test_job.py +++ b/tests/unit/test_job.py @@ -7,6 +7,7 @@ from pyiron_workflow import Workflow from pyiron_workflow.channels import NOT_DATA import pyiron_workflow.job # To get the job classes registered +from pyiron_workflow.node import Node class _WithAJob(unittest.TestCase, ABC): @@ -42,6 +43,9 @@ def test_clean_failure(self): def test_node(self): node = Workflow.create.standard.UserInput(42) nj = self.make_a_job_from_node(node) + + self.assertIsInstance(nj.get_input_node(), Node, msg="Sanity check") + nj.run() self.assertEqual( 42, @@ -49,6 +53,19 @@ def test_node(self): msg="A single node should run just as well as a workflow" ) + self.assertIsInstance( + nj.input["node"], + str, + msg="On saving, we convert the input to a bytestream so DataContainer can " + "handle storing it." + ) + self.assertIsInstance( + nj.get_input_node(), + Node, + msg="But we might want to look at it again, so make sure this convenience " + "method works." + ) + @unittest.skipIf(sys.version_info < (3, 11), "Storage will only work in 3.11+") def test_modal(self): modal_wf = Workflow("modal_wf") @@ -134,19 +151,13 @@ def not_importable_directy_from_module(x): return x + 1 nj = self.make_a_job_from_node(not_importable_directy_from_module(42)) - nj.run() - self.assertEqual( - 43, - nj.output.y, - msg="Things should run fine locally" - ) with self.assertRaises( AttributeError, - msg="We have promised that you'll hit trouble if you try to load a job " - "whose nodes are not all importable directly from their module" - # h5io also has this limitation, so I suspect that may be the source + msg="On saving we cloudpickle the node, then at run time we try to " + "recreate the dynamic class, but this doesn't work from the " + "scope, i.e. when the function is nested inside another function." ): - self.pr.load(nj.job_name) + nj.run() @unittest.skipIf(sys.version_info < (3, 11), "Storage will only work in 3.11+") def test_shorter_name(self): diff --git a/tests/unit/test_macro.py b/tests/unit/test_macro.py index 27956ee7..d594e1d1 100644 --- a/tests/unit/test_macro.py +++ b/tests/unit/test_macro.py @@ -1,10 +1,9 @@ -import sys from concurrent.futures import Future - +import pickle +import sys from time import sleep import unittest - from pyiron_workflow._tests import ensure_tests_in_python_path from pyiron_workflow.channels import NOT_DATA from pyiron_workflow.function import function_node @@ -492,49 +491,48 @@ def test_storage_for_modified_macros(self): modified_result = macro() - macro.save() - reloaded = Macro.create.demo.AddThree( - label="m", storage_backend=backend - ) - self.assertDictEqual( - modified_result, - reloaded.outputs.to_value_dict(), - msg="Updated IO should have been (de)serialized" - ) - self.assertSetEqual( - set(macro.children.keys()), - set(reloaded.children.keys()), - msg="All nodes should have been (de)serialized." - ) # Note that this snags the _new_ one in the case of h5io! - self.assertEqual( - Macro.create.demo.AddThree.__name__, - reloaded.__class__.__name__, - msg=f"LOOK OUT! This all (de)serialized nicely, but what we " - f"loaded is _falsely_ claiming to be an " - f"{Macro.create.demo.AddThree.__name__}. This is " - f"not any sort of technical error -- what other class name " - f"would we load? -- but is a deeper problem with saving " - f"modified objects that we need ot figure out some better " - f"solution for later." - ) - rerun = reloaded() - if backend == "h5io": + with self.assertRaises( + TypeError, msg="h5io can't handle custom reconstructors" + ): + macro.save() + else: + macro.save() + reloaded = Macro.create.demo.AddThree( + label="m", storage_backend=backend + ) self.assertDictEqual( modified_result, - rerun, - msg="Rerunning should re-execute the _modified_ " - "functionality" + reloaded.outputs.to_value_dict(), + msg="Updated IO should have been (de)serialized" ) - elif backend == "tinybase": - self.assertDictEqual( - original_result, - rerun, - msg="Rerunning should re-execute the _original_ " - "functionality" + self.assertSetEqual( + set(macro.children.keys()), + set(reloaded.children.keys()), + msg="All nodes should have been (de)serialized." + ) # Note that this snags the _new_ one in the case of h5io! + self.assertEqual( + Macro.create.demo.AddThree.__name__, + reloaded.__class__.__name__, + msg=f"LOOK OUT! This all (de)serialized nicely, but what we " + f"loaded is _falsely_ claiming to be an " + f"{Macro.create.demo.AddThree.__name__}. This is " + f"not any sort of technical error -- what other class name " + f"would we load? -- but is a deeper problem with saving " + f"modified objects that we need ot figure out some better " + f"solution for later." ) - else: - raise ValueError(f"Unexpected backend {backend}?") + rerun = reloaded() + + if backend == "tinybase": + self.assertDictEqual( + original_result, + rerun, + msg="Rerunning should re-execute the _original_ " + "functionality" + ) + else: + raise ValueError(f"Unexpected backend {backend}?") finally: macro.storage.delete() @@ -561,6 +559,40 @@ def ReturnHasDot(macro): macro.foo = macro.create.standard.UserInput() return macro.foo.outputs.user_input + def test_pickle(self): + m = macro_node(add_three_macro) + m(1) + reloaded_m = pickle.loads(pickle.dumps(m)) + self.assertTupleEqual( + m.child_labels, + reloaded_m.child_labels, + msg="Spot check values are getting reloaded correctly" + ) + self.assertDictEqual( + m.outputs.to_value_dict(), + reloaded_m.outputs.to_value_dict(), + msg="Spot check values are getting reloaded correctly" + ) + self.assertTrue( + reloaded_m.two.connected, + msg="The macro should reload with all its child connections" + ) + + self.assertTrue(m.two.connected, msg="Sanity check") + reloaded_two = pickle.loads(pickle.dumps(m.two)) + self.assertFalse( + reloaded_two.connected, + msg="Children are expected to be de-parenting on serialization, so that if " + "we ship them off to another process, they don't drag their whole " + "graph with them" + ) + self.assertEqual( + m.two.outputs.to_value_dict(), + reloaded_two.outputs.to_value_dict(), + msg="The remainder of the child node state should be recovering just " + "fine on (de)serialization, this is a spot-check" + ) + if __name__ == '__main__': unittest.main() diff --git a/tests/unit/test_workflow.py b/tests/unit/test_workflow.py index 6a66d9db..91337964 100644 --- a/tests/unit/test_workflow.py +++ b/tests/unit/test_workflow.py @@ -1,4 +1,5 @@ from concurrent.futures import Future +import pickle import sys from time import sleep import unittest @@ -441,27 +442,34 @@ def add_three_macro(self, one__x): def test_storage_values(self): for backend in Workflow.allowed_backends(): with self.subTest(backend): - wf = Workflow("wf", storage_backend=backend) try: + print("Trying", backend) + wf = Workflow("wf", storage_backend=backend) wf.register("static.demo_nodes", domain="demo") wf.inp = wf.create.demo.AddThree(x=0) wf.out = wf.inp.outputs.add_three + 1 wf_out = wf() three_result = wf.inp.three.outputs.add.value - wf.save() - - reloaded = Workflow("wf", storage_backend=backend) - self.assertEqual( - wf_out.out__add, - reloaded.outputs.out__add.value, - msg="Workflow-level data should get reloaded" - ) - self.assertEqual( - three_result, - reloaded.inp.three.value, - msg="Child data arbitrarily deep should get reloaded" - ) + if backend == "h5io": + with self.assertRaises( + TypeError, + msg="h5io can't handle custom reconstructors" + ): + wf.save() + else: + wf.save() + reloaded = Workflow("wf", storage_backend=backend) + self.assertEqual( + wf_out.out__add, + reloaded.outputs.out__add.value, + msg="Workflow-level data should get reloaded" + ) + self.assertEqual( + three_result, + reloaded.inp.three.value, + msg="Child data arbitrarily deep should get reloaded" + ) finally: # Clean up after ourselves wf.storage.delete() @@ -479,9 +487,20 @@ def test_storage_scopes(self): for backend in Workflow.allowed_backends(): with self.subTest(backend): try: - wf.storage_backend = backend - wf.save() - Workflow(wf.label, storage_backend=backend) + for backend in Workflow.allowed_backends(): + if backend == "h5io": + with self.subTest(backend): + with self.assertRaises( + TypeError, + msg="h5io can't handle custom reconstructors" + ): + wf.storage_backend = backend + wf.save() + else: + with self.subTest(backend): + wf.storage_backend = backend + wf.save() + Workflow(wf.label, storage_backend=backend) finally: wf.storage.delete() @@ -499,24 +518,30 @@ def test_storage_scopes(self): wf.save() finally: wf.remove_child(wf.import_type_mismatch) + wf.storage.delete() if "h5io" in Workflow.allowed_backends(): wf.add_child(PlusOne(label="local_but_importable")) try: - wf.storage_backend = "h5io" - wf.save() - Workflow(wf.label, storage_backend="h5io") + with self.assertRaises( + TypeError, msg="h5io can't handle custom reconstructors" + ): + wf.storage_backend = "h5io" + wf.save() finally: wf.storage.delete() if "tinybase" in Workflow.allowed_backends(): - with self.assertRaises( - NotImplementedError, - msg="Storage docs for tinybase claim all children must be registered " - "nodes" - ): - wf.storage_backend = "tinybase" - wf.save() + try: + with self.assertRaises( + NotImplementedError, + msg="Storage docs for tinybase claim all children must be registered " + "nodes" + ): + wf.storage_backend = "tinybase" + wf.save() + finally: + wf.storage.delete() if "h5io" in Workflow.allowed_backends(): with self.subTest("Instanced node"): @@ -553,6 +578,19 @@ def UnimportableScope(x): wf.remove_child(wf.unimportable_scope) wf.storage.delete() + def test_pickle(self): + wf = Workflow("wf") + wf.register("static.demo_nodes", domain="demo") + wf.inp = wf.create.demo.AddThree(x=0) + wf.out = wf.inp.outputs.add_three + 1 + wf_out = wf() + reloaded = pickle.loads(pickle.dumps(wf)) + self.assertDictEqual( + wf_out, + reloaded.outputs.to_value_dict(), + msg="Pickling should work" + ) + if __name__ == '__main__': unittest.main() From d620c7be55b2656d1e9ce677b67d9a9cde04ca31 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Mon, 29 Apr 2024 14:50:11 -0700 Subject: [PATCH 18/19] Remove unused decorator And reformat tests in the vein of usage in Function and Macro --- pyiron_workflow/io_preview.py | 113 +--------------------------------- tests/unit/test_io_preview.py | 60 +++++++++++++----- 2 files changed, 44 insertions(+), 129 deletions(-) diff --git a/pyiron_workflow/io_preview.py b/pyiron_workflow/io_preview.py index 46a4e323..c6f21773 100644 --- a/pyiron_workflow/io_preview.py +++ b/pyiron_workflow/io_preview.py @@ -130,7 +130,7 @@ class ScrapesIO(HasIOPreview, ABC): @classmethod @abstractmethod def _io_defining_function(cls) -> callable: - """Must return a static class method.""" + """Must return a static method.""" _output_labels: ClassVar[tuple[str] | None] = None # None: scrape them _validate_output_labels: ClassVar[bool] = True # True: validate against source code @@ -356,114 +356,3 @@ def inputs(self) -> Inputs: @property def outputs(self) -> OutputsWithInjection: return self._outputs - - -class DecoratedNode(StaticNode, ScrapesIO, ABC): - """ - A static node whose IO is defined by a function's information (and maybe output - labels). - """ - - -def decorated_node_decorator_factory( - parent_class: type[DecoratedNode], - io_static_method: callable, - decorator_docstring_additions: str = "", - **parent_class_attr_overrides, -): - """ - A decorator factory for building decorators to dynamically create new subclasses - of some subclass of :class:`DecoratedNode` using the function they decorate. - - New classes get their class name and module set using the decorated function's - name and module. - - Args: - parent_class (type[DecoratedNode]): The base class for the new node class. - io_static_method: The static method on the :param:`parent_class` which will - store the io-defining function the resulting decorator will decorate. - :param:`parent_class` must override :meth:`_io_defining_function` inherited - from :class:`DecoratedNode` to return this method. This allows - :param:`parent_class` classes to have unique names for their io-defining - functions. - decorator_docstring_additions (str): Any extra text to add between the main - body of the docstring and the arguments. - **parent_class_attr_overrides: Any additional attributes to pass to the new, - dynamically created class created by the resulting decorator. - - Returns: - (callable): A decorator that takes creates a new subclass of - :param:`parent_class` that uses the wrapped function as the return value of - :meth:`_io_defining_function` for the :class:`DecoratedNode` mixin. - """ - if getattr(parent_class, io_static_method.__name__) is not io_static_method: - raise ValueError( - f"{io_static_method.__name__} is not a method on {parent_class}" - ) - if not isinstance(io_static_method, FunctionType): - raise TypeError(f"{io_static_method.__name__} should be a static method") - - def as_decorated_node_decorator( - *output_labels: str, - validate_output_labels: bool = True, - ): - output_labels = None if len(output_labels) == 0 else output_labels - - @builds_class_io - def as_decorated_node(io_defining_function: callable): - if not callable(io_defining_function): - raise AttributeError( - f"Tried to create a new child class of {parent_class.__name__}, " - f"but got {io_defining_function} instead of a callable." - ) - - return type( - io_defining_function.__name__, - (parent_class,), # Define parentage - { - io_static_method.__name__: staticmethod(io_defining_function), - "__module__": io_defining_function.__module__, - "_output_labels": output_labels, - "_validate_output_labels": validate_output_labels, - **parent_class_attr_overrides, - }, - ) - - return as_decorated_node - - as_decorated_node_decorator.__doc__ = dedent( - f""" - A decorator for dynamically creating `{parent_class.__name__}` sub-classes by - wrapping a function as the `{io_static_method.__name__}`. - - The returned subclass uses the wrapped function (and optionally any provided - :param:`output_labels`) to specify its IO. - - {decorator_docstring_additions} - - Args: - *output_labels (str): A name for each return value of the graph creating - function. When empty, scrapes output labels automatically from the - source code of the wrapped function. This can be useful when returned - values are not well named, e.g. to make the output channel - dot-accessible if it would otherwise have a label that requires - item-string-based access. Additionally, specifying a _single_ label for - a wrapped function that returns a tuple of values ensures that a - _single_ output channel (holding the tuple) is created, instead of one - channel for each return value. The default approach of extracting - labels from the function source code also requires that the function - body contain _at most_ one `return` expression, so providing explicit - labels can be used to circumvent this (at your own risk). (Default is - empty, try to scrape labels from the source code of the wrapped - function.) - validate_output_labels (bool): Whether to compare the provided output labels - (if any) against the source code (if available). (Default is True.) - - Returns: - (callable[[callable], type[{parent_class.__name__}]]): A decorator that - transforms a function into a child class of `{parent_class.__name__}` - using the decorated function as - `{parent_class.__name__}.{io_static_method.__name__}`. - """ - ) - return as_decorated_node_decorator diff --git a/tests/unit/test_io_preview.py b/tests/unit/test_io_preview.py index fa366028..d7cdddae 100644 --- a/tests/unit/test_io_preview.py +++ b/tests/unit/test_io_preview.py @@ -3,26 +3,52 @@ import unittest from pyiron_workflow.channels import NOT_DATA -from pyiron_workflow.io_preview import ( - ScrapesIO, decorated_node_decorator_factory, OutputLabelsNotValidated -) +from pyiron_workflow.io_preview import ScrapesIO, OutputLabelsNotValidated +from pyiron_workflow.snippets.factory import classfactory -class ScraperParent(ScrapesIO, ABC): - - @staticmethod - @abstractmethod - def io_function(*args, **kwargs): - pass - +class ScrapesFromDecorated(ScrapesIO): @classmethod - def _io_defining_function(cls): - return cls.io_function - - -as_scraper = decorated_node_decorator_factory( - ScraperParent, ScraperParent.io_function -) + def _io_defining_function(cls) -> callable: + return cls._decorated_function + + +@classfactory +def scraper_factory( + io_defining_function, + validate_output_labels, + io_defining_function_uses_self, + /, + *output_labels, +): + return ( + io_defining_function.__name__, + (ScrapesFromDecorated,), # Define parentage + { + "_decorated_function": staticmethod(io_defining_function), + "__module__": io_defining_function.__module__, + "_output_labels": None if len(output_labels) == 0 else output_labels, + "_validate_output_labels": validate_output_labels, + "_io_defining_function_uses_self": io_defining_function_uses_self + }, + {}, + ) + + +def as_scraper( + *output_labels, + validate_output_labels=True, + io_defining_function_uses_self=False, +): + def scraper_decorator(fnc): + scraper_factory.clear(fnc.__name__) # Force a fresh class + factory_made = scraper_factory( + fnc, validate_output_labels, io_defining_function_uses_self, *output_labels + ) + factory_made._class_returns_from_decorated_function = fnc + factory_made.preview_io() + return factory_made + return scraper_decorator class TestIOPreview(unittest.TestCase): From 9f0955da21f5db0ddc69343da5310d4d655ab7c2 Mon Sep 17 00:00:00 2001 From: pyiron-runner Date: Mon, 29 Apr 2024 22:10:44 +0000 Subject: [PATCH 19/19] Format black --- pyiron_workflow/function.py | 6 +++--- pyiron_workflow/io_preview.py | 12 ++++++++++-- pyiron_workflow/macro.py | 9 ++++++--- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/pyiron_workflow/function.py b/pyiron_workflow/function.py index 6f749bb3..18cc5198 100644 --- a/pyiron_workflow/function.py +++ b/pyiron_workflow/function.py @@ -371,6 +371,7 @@ def decorator(node_function): factory_made._class_returns_from_decorated_function = node_function factory_made.preview_io() return factory_made + return decorator @@ -379,16 +380,15 @@ def function_node( *node_args, output_labels=None, validate_output_labels=True, - **node_kwargs + **node_kwargs, ): if output_labels is None: output_labels = () elif isinstance(output_labels, str): output_labels = (output_labels,) - function_node_factory.clear(node_function.__name__) # Force a fresh class + function_node_factory.clear(node_function.__name__) # Force a fresh class factory_made = function_node_factory( node_function, validate_output_labels, *output_labels ) factory_made.preview_io() return factory_made(*node_args, **node_kwargs) - diff --git a/pyiron_workflow/io_preview.py b/pyiron_workflow/io_preview.py index c6f21773..6f692077 100644 --- a/pyiron_workflow/io_preview.py +++ b/pyiron_workflow/io_preview.py @@ -19,7 +19,13 @@ from textwrap import dedent from types import FunctionType from typing import ( - Any, ClassVar, get_args, get_type_hints, Literal, Optional, TYPE_CHECKING + Any, + ClassVar, + get_args, + get_type_hints, + Literal, + Optional, + TYPE_CHECKING, ) from pyiron_workflow.channels import InputData, NOT_DATA @@ -134,7 +140,9 @@ def _io_defining_function(cls) -> callable: _output_labels: ClassVar[tuple[str] | None] = None # None: scrape them _validate_output_labels: ClassVar[bool] = True # True: validate against source code - _io_defining_function_uses_self: ClassVar[bool] = False # False: use entire signature + _io_defining_function_uses_self: ClassVar[bool] = ( + False # False: use entire signature + ) @classmethod def _build_inputs_preview(cls): diff --git a/pyiron_workflow/macro.py b/pyiron_workflow/macro.py index fbbdabfa..4c3727ba 100644 --- a/pyiron_workflow/macro.py +++ b/pyiron_workflow/macro.py @@ -472,7 +472,8 @@ def __setstate__(self, state): @classfactory -def macro_node_factory(graph_creator: callable, validate_output_labels: bool, /, *output_labels +def macro_node_factory( + graph_creator: callable, validate_output_labels: bool, /, *output_labels ): return ( graph_creator.__name__, @@ -496,6 +497,7 @@ def decorator(node_function): factory_made._class_returns_from_decorated_function = node_function factory_made.preview_io() return factory_made + return decorator @@ -504,19 +506,20 @@ def macro_node( *node_args, output_labels=None, validate_output_labels=True, - **node_kwargs + **node_kwargs, ): if output_labels is None: output_labels = () elif isinstance(output_labels, str): output_labels = (output_labels,) - macro_node_factory.clear(node_function.__name__) # Force a fresh class + macro_node_factory.clear(node_function.__name__) # Force a fresh class factory_made = macro_node_factory( node_function, validate_output_labels, *output_labels ) factory_made.preview_io() return factory_made(*node_args, **node_kwargs) + # as_macro_node = decorated_node_decorator_factory( # Macro, # Macro.graph_creator,