From ec177f99e154ce311533f9d432de8809555e0f7a Mon Sep 17 00:00:00 2001 From: liamhuber Date: Thu, 22 Jun 2023 12:30:41 -0700 Subject: [PATCH 1/9] Fix base class docstring --- pyiron_contrib/workflow/io.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/pyiron_contrib/workflow/io.py b/pyiron_contrib/workflow/io.py index 093f31053..c77dd871c 100644 --- a/pyiron_contrib/workflow/io.py +++ b/pyiron_contrib/workflow/io.py @@ -1,3 +1,7 @@ +""" +Collections of channel objects. +""" + from __future__ import annotations from abc import ABC, abstractmethod @@ -19,24 +23,24 @@ class IO(HasToDict, ABC): """ IO is a convenience layer for holding and accessing multiple input/output channels. - It allows key and dot-based access to the underlying channels based on their name. + It allows key and dot-based access to the underlying channels. Channels can also be iterated over, and there are a number of helper functions to alter the properties of or check the status of all the channels at once. - A new channel can be assigned as an attribute of an IO collection, as long as the - attribute name matches the channel's label and type (i.e. `OutputChannel` for - `Outputs` and `InputChannel` for `Inputs`). - - New channels can also be added using the `add` method, which must be implemented in - child classes to add channels of the correct type. + A new channel can be assigned as an attribute of an IO collection, as long as it + matches the channel's type (e.g. `OutputChannel` for `Outputs`, `InputChannel` + for `Inputs`, etc...). When assigning something to an attribute holding an existing channel, if the - assigned object is a `Channel`, then it is treated like a `connection`, otherwise - it is treated like a value `update`. I.e. + assigned object is a `Channel`, then an attempt is made to make a `connection` + between the two channels, otherwise we fall back on a value assignment that must + be defined in child classes under `_assign_value_to_existing_channel`, i.e. >>> some_io.some_existing_channel = 5 is equivalent to - >>> some_io.some_existing_channel.update(5) + >>> some_io._assign_value_to_existing_channel( + ... some_io["some_existing_channel"], 5 + ... ) and >>> some_io.some_existing_channel = some_other_channel From a698291b0fcdc4e05db73abed80fe006804650b1 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Thu, 22 Jun 2023 12:33:58 -0700 Subject: [PATCH 2/9] Allow IO panels to store channels under a different name than the label This will allow Workflows to have io with labels that combine the node and channel labels, without modifying the underlying channel label --- pyiron_contrib/workflow/io.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pyiron_contrib/workflow/io.py b/pyiron_contrib/workflow/io.py index c77dd871c..f4a59a65e 100644 --- a/pyiron_contrib/workflow/io.py +++ b/pyiron_contrib/workflow/io.py @@ -5,6 +5,7 @@ from __future__ import annotations from abc import ABC, abstractmethod +from warnings import warn from pyiron_contrib.workflow.channels import ( Channel, @@ -76,9 +77,9 @@ def __setattr__(self, key, value): self._assign_value_to_existing_channel(self.channel_dict[key], value) elif isinstance(value, self._channel_class): if key != value.label: - raise ValueError( - f"Channels can only be assigned to attributes matching their label," - f"but just tried to assign the channel {value.label} to {key}" + warn( + f"Assigning a channel with the label {value.label} to the io key " + f"{key}" ) self.channel_dict[key] = value else: From d97b87f5132327abe63071fcb88f657e5a0302ed Mon Sep 17 00:00:00 2001 From: liamhuber Date: Thu, 22 Jun 2023 12:38:46 -0700 Subject: [PATCH 3/9] Add docstrings to IO classes --- pyiron_contrib/workflow/io.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pyiron_contrib/workflow/io.py b/pyiron_contrib/workflow/io.py index f4a59a65e..8c6e21e02 100644 --- a/pyiron_contrib/workflow/io.py +++ b/pyiron_contrib/workflow/io.py @@ -135,6 +135,10 @@ def to_dict(self): class DataIO(IO, ABC): + """ + Extends the base IO class with helper methods relevant to data channels. + """ + def _assign_a_non_channel_value(self, channel: DataChannel, value) -> None: channel.update(value) @@ -195,6 +199,13 @@ def _channel_class(self) -> type(OutputSignal): class Signals: + """ + A meta-container for input and output signal IO containers. + + Attributes: + input (InputSignals): An empty input signals IO container. + output (OutputSignals): An empty input signals IO container. + """ def __init__(self): self.input = InputSignals() self.output = OutputSignals() From 927cc40c566dae132a761c8b72fbb07203468755 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Thu, 22 Jun 2023 12:39:07 -0700 Subject: [PATCH 4/9] Remove unused import --- pyiron_contrib/workflow/channels.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pyiron_contrib/workflow/channels.py b/pyiron_contrib/workflow/channels.py index 1eca94cb3..486948c4d 100644 --- a/pyiron_contrib/workflow/channels.py +++ b/pyiron_contrib/workflow/channels.py @@ -2,7 +2,6 @@ import typing from abc import ABC, abstractmethod -from json import dumps from warnings import warn from pyiron_contrib.workflow.has_channel import HasChannel From e173d21e0b1ab931619ffd5e9f12f8a294a8b334 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Thu, 22 Jun 2023 13:18:59 -0700 Subject: [PATCH 5/9] Expand and update channel docs --- pyiron_contrib/workflow/channels.py | 173 +++++++++++++++++++++++++--- 1 file changed, 155 insertions(+), 18 deletions(-) diff --git a/pyiron_contrib/workflow/channels.py b/pyiron_contrib/workflow/channels.py index 486948c4d..de556917e 100644 --- a/pyiron_contrib/workflow/channels.py +++ b/pyiron_contrib/workflow/channels.py @@ -1,3 +1,23 @@ +""" +Channels are access points for information to flow into and out of nodes. + +Data channels carry, unsurprisingly, data. +Input data channels force an update on their owning node when they are updated with new +data, and output data channels update all the input data channels to which they are +connected. +In this way, data channels facilitate forward propagation of data through a graph. +They hold data persistently. + +Signal channels are tools for procedurally exposing functionality on nodes. +Input signal channels are connected to a callback function which gets invoked when the +channel is updated. +Output signal channels must be accessed by the owning node directly, and then trigger +all the input signal channels to which they are connected. +In this way, signal channels can force behaviour (node method calls) to propagate +forwards through a graph. +They do not hold any data, but rather fire for an effect. +""" + from __future__ import annotations import typing @@ -19,10 +39,20 @@ class Channel(HasChannel, HasToDict, ABC): """ Channels facilitate the flow of information (data or control signals) into and out of nodes. - They have a label and belong to a node. + They must have a label and belong to a node. Input/output channels can be (dis)connected from other output/input channels, and store all of their current connections in a list. + This connection information is duplicated in that it is stored on _both_ channels + that form the connection. + + Child classes must define a string representation, `__str__`, and what to do on an + attempted connection, `connect`. + + Attributes: + label (str): The name of the channel. + node (pyiron_contrib.workflow.node.Node): The node to which the channel belongs. + connections (list[Channel]): Other channels to which this channel is connected. """ def __init__( @@ -30,9 +60,17 @@ def __init__( label: str, node: Node, ): - self.label = label - self.node = node - self.connections = [] + """ + Make a new channel. + + Args: + label (str): A name for the channel. + node (pyiron_contrib.workflow.node.Node): The node to which the channel + belongs. + """ + self.label: str = label + self.node: Node = node + self.connections: list[Channel] = [] @abstractmethod def __str__(self): @@ -40,19 +78,38 @@ def __str__(self): @abstractmethod def connect(self, *others: Channel): + """ + How to handle connections to other channels. + + Args: + *others (Channel): The other channel objects to attempt to connect with. + """ pass def disconnect(self, *others: Channel): + """ + If currently connected to any others, removes this and the other from eachothers + respective connections lists. + + Args: + *others (Channel): The other channels to disconnect from. + """ for other in others: if other in self.connections: self.connections.remove(other) other.disconnect(self) def disconnect_all(self): + """ + Disconnect from all other channels currently in the connections list. + """ self.disconnect(*self.connections) @property def connected(self): + """ + Has at least one connection. + """ return len(self.connections) > 0 def _already_connected(self, other: Channel): @@ -82,16 +139,13 @@ class DataChannel(Channel, ABC): They store this data in a `value` attribute. They may optionally have a type hint. They have a `ready` attribute which tells whether their value matches their type - hint. + hint (if one is provided, else `True`). They may optionally have a storage priority (but this doesn't do anything yet). (In the future they may optionally have an ontological type.) The `value` held by a channel can be manually assigned, but should normally be set by the `update` method. In neither case is the type hint strictly enforced. - Input channels will then propagate their value along to their owning node. - Output channels with then propagate their value to all the input channels they're - connected to. Type hinting is strictly enforced in one situation: when making connections to other channels and at least one data channel has a non-None value for its type hint. @@ -121,11 +175,6 @@ class DataChannel(Channel, ABC): hint a tuple with a mixture of fixed elements of fixed type, followed by an arbitrary elements of arbitrary type. This and other complex scenarios are not yet included in our test suite and behaviour is not guaranteed. - - TODO: - In direct relation to the above warning, it may be nice to add a flag to - channels to turn on/off the strict enforcement of type hints when making - connections. """ def __init__( @@ -144,23 +193,53 @@ def __init__( @property def ready(self): + """ + Check if the currently stored value satisfies the channel's type hint. + + Returns: + (bool): Whether the value matches the type hint. + """ if self.type_hint is not None: return valid_value(self.value, self.type_hint) else: return True def update(self, value): + """ + Store a new value and trigger before- and after-update routines. + + Args: + value: The value to store. + """ self._before_update() self.value = value self._after_update() def _before_update(self): + """ + A tool for child classes to do things before the value changed during an update. + """ pass def _after_update(self): + """ + A tool for child classes to do things after the value changed during an update. + """ pass def connect(self, *others: DataChannel): + """ + For all others for which the connection is valid (one input, one output, both + data channels), adds this to the other's list of connections and the other to + this list of connections. + Then the input channel gets updated with the output channel's current value. + + Args: + *others (DataChannel): + + Raises: + TypeError: When one of others is not a `DataChannel` + """ for other in others: if self._valid_connection(other): self.connections.append(other) @@ -216,12 +295,18 @@ def to_dict(self): class InputData(DataChannel): """ - `InputData` channels may be set to `wait_for_update()`, and they are only `ready` - when they are not `waiting_for_update`. Their parent node can be told to always set - them to wait for an update after the node runs using - `require_update_after_node_runs()`. + On `update`, Input channels will then propagate their value along to their owning + node by invoking its `update` method. - They may also set their `strict_connections` to `False` (`True` -- default) at + `InputData` channels may be set to `wait_for_update()`, and they are only `ready` + when they are not `waiting_for_update`. + Their parent node can be told to always set them to wait for an update after the + node runs using `require_update_after_node_runs()`. + This allows nodes to complete the update of multiple channels before running again. + + The `strict_connections` parameter controls whether connections are subject to + type checking requirements. + I.e., they may set `strict_connections` to `False` (`True` -- default) at instantiation or later with `(de)activate_strict_connections()` to prevent (enable) data type checking when making connections with `OutputData` channels. """ @@ -246,10 +331,23 @@ def __init__( self.waiting_for_update = False def wait_for_update(self): + """ + Sets `waiting_for_update` to `True`, which prevents `ready` from returning + `True` until `update` is called. + """ self.waiting_for_update = True @property def ready(self): + """ + Extends the parent class check for whether the value matches the type hint with + a check for whether the channel has been told to wait for an update (and not + been updated since then). + + Returns: + (bool): True when the stored value matches the type hint and the channel + has not been told to wait for an update. + """ return not self.waiting_for_update and super().ready def _before_update(self): @@ -264,6 +362,14 @@ def _after_update(self): self.node.update() def require_update_after_node_runs(self, wait_now=False): + """ + Registers this channel with its owning node as one that should have + `wait_for_update()` applied after each time the node runs. + + Args: + wait_now (bool): Also call `wait_for_update()` right now. (Default is + False.) + """ if self.label not in self.node.channels_requiring_update_after_run: self.node.channels_requiring_update_after_run.append(self.label) if wait_now: @@ -277,6 +383,10 @@ def deactivate_strict_connections(self): class OutputData(DataChannel): + """ + On `update`, Output channels propagate their value to all the input channels to + which they are connected by invoking their `update` method. + """ def _after_update(self): for inp in self.connections: inp.update(self.value) @@ -296,6 +406,17 @@ def __call__(self): pass def connect(self, *others: Channel): + """ + For all others for which the connection is valid (one input, one output, both + data channels), adds this to the other's list of connections and the other to + this list of connections. + + Args: + *others (SignalChannel): The other channels to attempt a connection to + + Raises: + TypeError: When one of others is not a `SignalChannel` + """ for other in others: if self._valid_connection(other): self.connections.append(other) @@ -322,12 +443,25 @@ def _is_IO_pair(self, other) -> bool: class InputSignal(SignalChannel): + """ + Invokes a callback when called. + """ def __init__( self, label: str, node: Node, callback: callable, ): + """ + Make a new input signal channel. + + Args: + label (str): A name for the channel. + node (pyiron_contrib.workflow.node.Node): The node to which the channel + belongs. + callback (callable): An argument-free callback to invoke when calling this + object. + """ super().__init__(label=label, node=node) self.callback: callable = callback @@ -344,6 +478,9 @@ def to_dict(self): class OutputSignal(SignalChannel): + """ + Calls all the input signal objects in its connections list when called. + """ def __call__(self): for c in self.connections: c() From 233623e1054a1d18b7c9920c22aebeec567452d5 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Thu, 22 Jun 2023 13:28:54 -0700 Subject: [PATCH 6/9] Expand and update type hinting --- pyiron_contrib/workflow/channels.py | 56 ++++++++++++++--------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/pyiron_contrib/workflow/channels.py b/pyiron_contrib/workflow/channels.py index de556917e..64d760567 100644 --- a/pyiron_contrib/workflow/channels.py +++ b/pyiron_contrib/workflow/channels.py @@ -77,7 +77,7 @@ def __str__(self): pass @abstractmethod - def connect(self, *others: Channel): + def connect(self, *others: Channel) -> None: """ How to handle connections to other channels. @@ -86,7 +86,7 @@ def connect(self, *others: Channel): """ pass - def disconnect(self, *others: Channel): + def disconnect(self, *others: Channel) -> None: """ If currently connected to any others, removes this and the other from eachothers respective connections lists. @@ -99,20 +99,20 @@ def disconnect(self, *others: Channel): self.connections.remove(other) other.disconnect(self) - def disconnect_all(self): + def disconnect_all(self) -> None: """ Disconnect from all other channels currently in the connections list. """ self.disconnect(*self.connections) @property - def connected(self): + def connected(self) -> bool: """ Has at least one connection. """ return len(self.connections) > 0 - def _already_connected(self, other: Channel): + def _already_connected(self, other: Channel) -> bool: return other in self.connections def __iter__(self): @@ -125,7 +125,7 @@ def __len__(self): def channel(self) -> Channel: return self - def to_dict(self): + def to_dict(self) -> dict: return { "label": self.label, "connected": self.connected, @@ -192,7 +192,7 @@ def __init__( self.storage_priority = storage_priority @property - def ready(self): + def ready(self) -> bool: """ Check if the currently stored value satisfies the channel's type hint. @@ -204,7 +204,7 @@ def ready(self): else: return True - def update(self, value): + def update(self, value) -> None: """ Store a new value and trigger before- and after-update routines. @@ -215,19 +215,19 @@ def update(self, value): self.value = value self._after_update() - def _before_update(self): + def _before_update(self) -> None: """ A tool for child classes to do things before the value changed during an update. """ pass - def _after_update(self): + def _after_update(self) -> None: """ A tool for child classes to do things after the value changed during an update. """ pass - def connect(self, *others: DataChannel): + def connect(self, *others: DataChannel) -> None: """ For all others for which the connection is valid (one input, one output, both data channels), adds this to the other's list of connections and the other to @@ -258,7 +258,7 @@ def connect(self, *others: DataChannel): f"({self.__class__.__name__}) got a {other} ({type(other)})" ) - def _valid_connection(self, other): + def _valid_connection(self, other) -> bool: if self._is_IO_pair(other) and not self._already_connected(other): if self._both_typed(other): out, inp = self._figure_out_who_is_who(other) @@ -274,10 +274,10 @@ def _valid_connection(self, other): else: return False - def _is_IO_pair(self, other: DataChannel): + def _is_IO_pair(self, other: DataChannel) -> bool: return isinstance(other, DataChannel) and not isinstance(other, self.__class__) - def _both_typed(self, other: DataChannel): + def _both_typed(self, other: DataChannel) -> bool: return self.type_hint is not None and other.type_hint is not None def _figure_out_who_is_who(self, other: DataChannel) -> (OutputData, InputData): @@ -286,7 +286,7 @@ def _figure_out_who_is_who(self, other: DataChannel) -> (OutputData, InputData): def __str__(self): return str(self.value) - def to_dict(self): + def to_dict(self) -> dict: d = super().to_dict() d["value"] = repr(self.value) d["ready"] = self.ready @@ -330,7 +330,7 @@ def __init__( self.strict_connections = strict_connections self.waiting_for_update = False - def wait_for_update(self): + def wait_for_update(self) -> None: """ Sets `waiting_for_update` to `True`, which prevents `ready` from returning `True` until `update` is called. @@ -338,7 +338,7 @@ def wait_for_update(self): self.waiting_for_update = True @property - def ready(self): + def ready(self) -> bool: """ Extends the parent class check for whether the value matches the type hint with a check for whether the channel has been told to wait for an update (and not @@ -350,18 +350,18 @@ def ready(self): """ return not self.waiting_for_update and super().ready - def _before_update(self): + def _before_update(self) -> None: if self.node.running: raise RuntimeError( f"Parent node {self.node.label} of {self.label} is running, so value " f"cannot be updated." ) - def _after_update(self): + def _after_update(self) -> None: self.waiting_for_update = False self.node.update() - def require_update_after_node_runs(self, wait_now=False): + def require_update_after_node_runs(self, wait_now=False) -> None: """ Registers this channel with its owning node as one that should have `wait_for_update()` applied after each time the node runs. @@ -375,10 +375,10 @@ def require_update_after_node_runs(self, wait_now=False): if wait_now: self.wait_for_update() - def activate_strict_connections(self): + def activate_strict_connections(self) -> None: self.strict_connections = True - def deactivate_strict_connections(self): + def deactivate_strict_connections(self) -> None: self.strict_connections = False @@ -387,7 +387,7 @@ class OutputData(DataChannel): On `update`, Output channels propagate their value to all the input channels to which they are connected by invoking their `update` method. """ - def _after_update(self): + def _after_update(self) -> None: for inp in self.connections: inp.update(self.value) @@ -402,10 +402,10 @@ class SignalChannel(Channel, ABC): """ @abstractmethod - def __call__(self): + def __call__(self) -> None: pass - def connect(self, *others: Channel): + def connect(self, *others: SignalChannel) -> None: """ For all others for which the connection is valid (one input, one output, both data channels), adds this to the other's list of connections and the other to @@ -465,13 +465,13 @@ def __init__( super().__init__(label=label, node=node) self.callback: callable = callback - def __call__(self): + def __call__(self) -> None: self.callback() def __str__(self): return f"{self.label} runs {self.callback.__name__}" - def to_dict(self): + def to_dict(self) -> dict: d = super().to_dict() d["callback"] = self.callback.__name__ return d @@ -481,7 +481,7 @@ class OutputSignal(SignalChannel): """ Calls all the input signal objects in its connections list when called. """ - def __call__(self): + def __call__(self) -> None: for c in self.connections: c() From 77931c8282a9c3bfbbc7d82c5544448e8db7d756 Mon Sep 17 00:00:00 2001 From: liamhuber Date: Thu, 22 Jun 2023 13:48:24 -0700 Subject: [PATCH 7/9] Fix tests to match spec --- tests/unit/workflow/test_io.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/unit/workflow/test_io.py b/tests/unit/workflow/test_io.py index 7a75df862..950f4d778 100644 --- a/tests/unit/workflow/test_io.py +++ b/tests/unit/workflow/test_io.py @@ -40,8 +40,20 @@ def test_assignment(self): with self.assertRaises(TypeError): self.input.foo = "not an input channel" - with self.assertRaises(ValueError): + with self.subTest("Can assign to a key that is not the label"): + label_before_assignment = self.post_facto_output.label self.output.not_this_channels_name = self.post_facto_output + self.assertIs( + self.output.not_this_channels_name, + self.post_facto_output, + msg="Expected channel to get assigned" + ) + self.assertEqual( + self.post_facto_output.label, + label_before_assignment, + msg="Labels should not get updated on assignment of channels to IO " + "collections" + ) with self.assertRaises(TypeError): # Right label, and a channel, but wrong type of channel From cbc8fbce1489b4ef1bb7b8c21d1690200d9a325f Mon Sep 17 00:00:00 2001 From: liamhuber Date: Thu, 22 Jun 2023 13:49:03 -0700 Subject: [PATCH 8/9] Refactor: slide Start by testing things that are forbidden, and move to successful stuff --- tests/unit/workflow/test_io.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/unit/workflow/test_io.py b/tests/unit/workflow/test_io.py index 950f4d778..36e15a22f 100644 --- a/tests/unit/workflow/test_io.py +++ b/tests/unit/workflow/test_io.py @@ -40,6 +40,13 @@ def test_assignment(self): with self.assertRaises(TypeError): self.input.foo = "not an input channel" + with self.assertRaises(TypeError): + # Right label, and a channel, but wrong type of channel + self.input.b = self.post_facto_output + + with self.subTest("Successful channel assignment"): + self.output.b = self.post_facto_output + with self.subTest("Can assign to a key that is not the label"): label_before_assignment = self.post_facto_output.label self.output.not_this_channels_name = self.post_facto_output @@ -55,13 +62,6 @@ def test_assignment(self): "collections" ) - with self.assertRaises(TypeError): - # Right label, and a channel, but wrong type of channel - self.input.b = self.post_facto_output - - with self.subTest("Successful channel assignment"): - self.output.b = self.post_facto_output - def test_connection(self): self.input.x = self.input.y self.assertEqual( From 45048d49c6afb96788d99c2ee8b10b5a3cf751d3 Mon Sep 17 00:00:00 2001 From: pyiron-runner Date: Thu, 22 Jun 2023 20:56:42 +0000 Subject: [PATCH 9/9] Format black --- pyiron_contrib/workflow/channels.py | 3 +++ pyiron_contrib/workflow/io.py | 1 + 2 files changed, 4 insertions(+) diff --git a/pyiron_contrib/workflow/channels.py b/pyiron_contrib/workflow/channels.py index 64d760567..8bf6be53b 100644 --- a/pyiron_contrib/workflow/channels.py +++ b/pyiron_contrib/workflow/channels.py @@ -387,6 +387,7 @@ class OutputData(DataChannel): On `update`, Output channels propagate their value to all the input channels to which they are connected by invoking their `update` method. """ + def _after_update(self) -> None: for inp in self.connections: inp.update(self.value) @@ -446,6 +447,7 @@ class InputSignal(SignalChannel): """ Invokes a callback when called. """ + def __init__( self, label: str, @@ -481,6 +483,7 @@ class OutputSignal(SignalChannel): """ Calls all the input signal objects in its connections list when called. """ + def __call__(self) -> None: for c in self.connections: c() diff --git a/pyiron_contrib/workflow/io.py b/pyiron_contrib/workflow/io.py index 8c6e21e02..c5bc278c0 100644 --- a/pyiron_contrib/workflow/io.py +++ b/pyiron_contrib/workflow/io.py @@ -206,6 +206,7 @@ class Signals: input (InputSignals): An empty input signals IO container. output (OutputSignals): An empty input signals IO container. """ + def __init__(self): self.input = InputSignals() self.output = OutputSignals()