Suggestions and issues for the workflow class

[JNmpi]

@liamhuber, thanks for all the new features and enhancements for the workflow class. In the following, I list some issues and suggestions regarding the workflow class. For the following jupyter notebook I used the main branch. Thus, some of the issues may have been fixed in the latest branch.

from pyiron_contrib.workflow.node import Node, node, fast_node
from pyiron_contrib.workflow.workflow import Workflow

Using the decorator for a single output variable fails

@node('y')
def identity(x: int=0) -> int:
    return x

node_identity = identity(0)
# node_identity.run()  # uncomment this line to see the error message

The following construction without a decorator works as expected

def identity(x: int=0) -> int:
    return x

node_identity = Node(identity, 'y')

node_identity.inputs.x = 10
node_identity.run()
node_identity.outputs.y.value

10

Some suggestions for making working with the nodes more convenient

Return the available input channels (same for outputs):

node_identity.inputs

<pyiron_contrib.workflow.io.Inputs at 0x16b37de10>

Return the output (e.g. node.outputs.to_value_dict()), similar like dask.compute() when performing run:

node_identity.run()

Connecting two nodes

@node("p1", "m1")
def my_mwe_node(
    x: int | float, y: int | float = 1) -> tuple[int | float, int | float]:
    return x+1, y-1

node_instance = my_mwe_node(x=0)
node_instance.inputs.y = node_identity.outputs.y

node_identity.outputs.y.connected

True

The following code should 'see' that the input of the first node has been updated (i.e., should print 5 rather than 10):

node_identity.inputs.x = 5
#node_identity.run()
print (node_instance.inputs.y)
print (node_identity.outputs.y)
node_instance.run()

10
10

To get the expected behavior I have to manually run the first node (which is not what I would like to have for a delayed operation):

node_identity.inputs.x = 5
node_identity.run()
print (node_instance.inputs.y)
print (node_identity.outputs.y)
node_instance.run()

5
5

[liamhuber]

Thanks for the testing and feedback @JNmpi! fixed in the latest branch.

Here's a summary of the TODO's I see until this issue can be closed:

• [x] Raise a cleaner error when nodes created by decoration erroneously receive *output_labels at instantiation
• [x] Return output (or later a futures object) from the run() call
• [x] Add a better display representation to inputs and outputs

from pyiron_contrib.workflow.node import Node, node, fast_node
from pyiron_contrib.workflow.workflow import Workflow

Using the decorator for a single output variable fails

@node('y')
def identity(x: int=0) -> int:
    return x

node_identity = identity(0)
# node_identity.run()  # uncomment this line to see the error message

This needs a clearer error message, but is behaving as expected. The issue is node_identity = identity(0) is passing 0 as the first *arg for instantiating the class; so the value of 0 is winding up being interpreted as an output label and not an initial value for the x input. node_identity = identity(x=0) works just fine.

I'll leave this issue open until I patch in a cleared error message so users immediately know why this breaks, but right now I have no intention to change the behaviour. It might be possible to code it up so the class "intelligently" knows whether to parse *args as initial input values or legitimate class instantiation arguments... but IMO adding such complexity to the infrastructure is too costly for such marginal benefit in UX -- better to keep initial input values specified with **kwargs.

Some suggestions for making working with the nodes more convenient

Return the available input channels (same for outputs): node_identity.inputs <pyiron_contrib.workflow.io.Inputs at 0x16b37de10>

💯 The IO classes can and should have better human-readable representation. This should be straightforward to add.

...

Return the output (e.g. node.outputs.to_value_dict()), similar like dask.compute() when performing run:

Yep, good idea. Given our other recent discussion, I think this can instead return a Futures object when the node is being run by an executor instead of on the main python process, but implementing that part will need to wait a bit as we work on the Executor interface.

Connecting two nodes

...

The following code should 'see' that the input of the first node has been updated (i.e., should print 5 rather than 10):

...

On main:HEAD we currently have the behaviour you want in FastNode. Standard nodes set run_on_updates and update_on_instantiation both to False by default; that means that they do not attempt to run() the node when the input gets updated (e.g. by forming a new connection), nor do they update at the end of instantiation -- They only run() when run() is explicitly invoked (by calling the method or pinging the node.signals.input.run input signal).

We want such behaviour available for nodes that are super expensive (think VASP calculations), where we don't necessarily want the node re-running when updating the structure, again on kpoints, etc.

However, I totally agree that the "fast" behaviour is what is intuitively expected! Over in #729 we reverse things so that the standard Function(Node) behaves "fast" and a new Slow(Function) class is introduced that is identical to Function but with "slow" default values for the two bool flags above.

EDIT: I missed the IO formatting request on my first pass. It's absolutely a good idea though.

[JNmpi]

Thanks, @Liam for the fast response and the detailed explanations. Below are some thoughts and comments on the various issues:

• Using the decorator for a single output variable fails: Thanks to your explanation I understand that I have to use kwargs rather than args. I am still not sure it is why it is needed. Could you give an example of a class instantiation? My idea was to make the node functions as similar to a normal python function as possible. Thus, writing 0 rather than x=0 is likely to occur. Even if there is an error message it feels like an exception from conventional python. I would therefore really like, if possible, to avoid this behavior.
• Connecting two nodes: The functionality I would like to have is not so much pushing, but pulling. Running a node which requires input from other (not yet executed nodes) it should execute them and once finished execute the last node. This is a very nice feature of dask delayed. I see your point that to know when the nodes below are finished they have to push this info to the upper lying node. A possible solution would be to define all nodes as delayed (i.e., they do not update if new input is received) but once the top node is called the status of the underying node instances is changed to run if new input arives. To easily build workflows without having to execute each single node individually this would be a nice and important feature. Thus, calling run on a workflow or a top node it is clear that I want to update all lower lying nodes, even if they are expensive.

[liamhuber]

@JNmpi, and thanks for getting back to me so fast! I like asychronous stuff, but fast-paced async is even better 😄

Initial input as args

Could you give an example of a class instantiation? My idea was to make the node functions as similar to a normal python function as possible.

Yeah, I totally agree with the sentiment. So the issue is that for, e.g., the base node class that wraps functions the initialization signature looks like this:

def __init__(
        self,
        node_function: callable,
        *output_labels: str,
        label: Optional[str] = None,
        run_on_updates: bool = True,
        update_on_instantiation: bool = True,
        channels_requiring_update_after_run: Optional[list[str]] = None,
        parent: Optional[Composite] = None,
        **kwargs,
    ):

Here, the node_function to be wrapped and the output_labels are both required arguments of the node, then there are a bunch of optional arguments, and finally we accept **kwargs and will try to interpret them as initial input values. So the *args are already in use and not available to be interpreted as input values.

Of course, we could take our "mandatory" input, make it a keyword argument, e.g. node_function: callable = None, output_labels: list[str] = None and then just manually raise an exception if they're not explicitly provided. This would free up the positional arguments to be interpreted as input values, but is the sort of "unpythonic" complication to the implementation I expressed concern about, since now we're taking an existing feature -- mandatory input -- and spoofing it.

This is all further complicated when we think about the nodes created by decorators. There, we accept the output labels and node function kwargs as arguments to the decorator, and return a dynamically defined class. When we instantiate that class, we override some of its arguments, but additional positional arguments are still interpreted as output labels (hence the error message from your example where the code complains that the number of output labels doesn't match the number of returned values). Here's the decorator code for convenience to see in detail the implementation I'm talking about:

def function_node(*output_labels: str, **node_class_kwargs):
    """
    A decorator for dynamically creating node classes from functions.

    Decorates a function.
    Takes an output label for each returned value of the function.
    Returns a `Function` subclass whose name is the camel-case version of the function node,
    and whose signature is modified to exclude the node function and output labels
    (which are explicitly defined in the process of using the decorator).
    """

    def as_node(node_function: callable):
        return type(
            node_function.__name__.title().replace("_", ""),  # fnc_name to CamelCase
            (Function,),  # Define parentage
            {
                "__init__": partialmethod(
                    Function.__init__,
                    node_function,
                    *output_labels,
                    **node_class_kwargs,
                )
            },
        )

    return as_node

I do really like your idea of making input positional-arg-accessible, it's just that at this exact moment the implementation cost seems very high. @samwaseda had a really nice idea for automatically extracting output labels; if/when we get this implemented, it makes much more sense to move *output_labels over to a list kwarg, which is one step towards making implementation of your idea cleaner. So I think we'll be able to do this eventually, but would prefer to wait for now.

Push vs pull workflows

The functionality I would like to have is not so much pushing, but pulling. Running a node which requires input from other (not yet executed nodes) it should execute them and once finished execute the last node. This is a very nice feature of dask delayed. I see your point that to know when the nodes below are finished they have to push this info to the upper lying node.

Ahhh, ok! Unfortunately, to the best of my understanding, this is incompatible with cyclic graphs. We could imagine supporting both modes, with "pull" only being eligible for DAGs -- ryven supports both modes, although few or perhaps no others do. IMO, it's not worth the extra implementation headache to support both.

I have zero expectation that you've read this -- it's not even merged yet! -- but in the spec for workflows, I've claimed that something one of our differentiating features is ground-up support for cyclic graphs. Dask graphs, in comparison, offer "directed acyclic graph of tasks with data dependencies".

With the disclaimer that I may simply not be sufficiently imaginative or clever, I believe that writing cycling workflows, e.g. with a "while loop", is not only impossible in a pull-mode, but also requires execution dependencies and not just data dependencies -- and I don't understand how one would implement a pull-mode for execution dependencies.

To make it concrete, suppose we wanted a graph that encodes "generate a random number between 0 and 20; while this number is <10, keep generating new random numbers; finally, take the square root of this number". Below is a minimal push-based, "execution dependency", pyiron_contrib.workflows-like graph for accomplishing this. With full humility that I'm using "proof by lack of imagination", I really don't see how to encode such a routine with only pull-based data dependencies. Obviously this example is a silly toy, but it maps directly onto realistic while loops like "change cell shape until pressure is below a threshold" and "run thermodynamic integration until statistical error is below a threshold" that are relevant to us.

So I really don't have any intrinsic objection to a pull-mode, but I absolutely want to support cyclic graphs and am not smart enough to see a way to both at once. I'm open to conditional dual-support, but wouldn't want to implement that until we're really happy with the cyclic-supporting push-mode.

A possible solution would be to define all nodes as delayed (i.e., they do not update if new input is received) but once the top node is called the status of the underying node instances is changed to run if new input arives. To easily build workflows without having to execute each single node individually this would be a nice and important feature. Thus, calling run on a workflow or a top node it is clear that I want to update all lower lying nodes, even if they are expensive.

I 100% agree we want some sort of single-button "run it all" access exposed to users. There is definitely some tension here, as simply making all nodes delayed by default conflicts with your user frustration in #756. Ultimately there may be some element of "no free lunch", but -- unlike the cyclic-pull-graphs -- I don't see any deep and fundamental barrier here. We just need to play around and come up with more and more clever UI tricks and interfaces. Certainly the current implementation falls flat on its face here, as Workflow.run() raises a NotImplementedError 🤣 But we're on the same page for the objective and we will just need to chip away at it.

[niklassiemer]

Thank you very much for all these asynchronous discussions in a very well expressed form! I enjoy reading these discussions and I get an idea of what is happening! :)

Certainly the current implementation falls flat on its face here, as Workflow.run() raises a NotImplementedError 🤣 But we're on the same page for the objective and we will just need to chip away at it.

That sounds really like the way to go for me, populate a workflow object with all the nodes. In the best case I would like to support even something like (Sorry for not being able to dig out the correct/available syntax here!)

wf = Workflow()

@node('y')
def identity(x: int=0) -> int:
    return x

node_identity = identity(0)
@node("p1", "m1")
def my_mwe_node(
    x: int | float, y: int | float) -> tuple[int | float, int | float]:
    return x+1, y-1

node_instance = my_mwe_node(x=0) 
node_instance.inputs.y = node_identity.outputs.y

wf = Workflow([node_identity,node_instance])

wf.run(1, 2)

to populate the missing input fields (x of node_identity and y of my_mwe_node?

[liamhuber]

Thank you very much for all these asynchronous discussions in a very well expressed form! I enjoy reading these discussions and I get an idea of what is happening! :)

🥳

...

...
wf.run(1, 2)

to populate the missing input fields (x of node_identity and y of my_mwe_node?

I hadn't thought of (optionally!) passing input data to the run call -- I like it! Off the top of my head, something like that should by parsing input as a first step of the run call and then proceeding with the rest of the call "as usual".

However, at this stage it gets reeeaaally hard to do it with *args and not **kwargs. Currently, the nodes are stored as a dictionary, so there is no sense of their order. Even if we changed that, having the arg order be dependent on the order nodes were added to the graph seems super hard to use; so I'd push in the direction of run(**kwargs) but otherwise like it and think it's doable!

[niklassiemer]

Yes, I thought similar, however would not a kwarg a la node_name.input_label be needed in that case? I mean the nodes could and even probably would have the same input names?

[liamhuber]

Indeed, Workflow.inputs has keys that automatically take the form {node.label}_{channel.label}. There are guarantees that two nodes belonging to the same workflow don't have the same label (I don't think we're so explicit for input channels, but since these are generated from function args and kwargs you get the same guarantee implicitly), so there is an injective mapping between channel labels at the workflow level and actual channel objects.

[JNmpi]

Thanks, @liamhuber and @niklassiemer. A quick thought regarding the last item. The syntax {node.label}_{channel.label} may be a bit dangerous since we use reularly an underscore '' in our names/labels. Thus, the underscore is not a good separator to extract/know what part of the full name belongs to the node/channel etc. A very pragmatic suggestion would be to change it to `{node.label}{channel.label}and check that the labels do not contain a double underscore (if they do we could raise an error). The translation from label name to object call could be then done fully automatically and unambigously, e.g. the string{node.label}__{channel.label}would internally translated to{node.label}.{channel.label}`

[JNmpi]

Some (super programmatic) thoughts regarding push vs pull workflows. The reason we want to have delayed execution is to avoid for time-consuming executions such as DFT runs that changing the input is going to immediately start execution, even if the changes are not complete. If we call run on a top node or the workflow we know that all input changes have been finalised and that the computation should run with exactly these input parameters. My suggestion would be therefore to temporarily change the status of all nodes below the top node is changed from delayed to running instantaneously. This approach should also work for cyclic workflows since in the running mode each node is getting this status. Once the workflow calculation is finished the status is reverted to its original stage. Of course, to prevent undesired side effects the workflow/nodes should be locked, i.e., they should raise an error when trying to modify its/their input. Since we know that the present workflow implementation works when being in push mode, introducing such a temporary mode switch should work right out of the box.

[JNmpi]

Also some thoughts regarding the issue Initial input as args:

A possible solution may be to rewrite the decorator to convert the function into a class. This is what I did in my code example when discussing the conversion of nodes into files and vice versa. You may have a look into it. Once I have a class I can introduce my own call definition and I should be free to use the various arguments from the node and the decorator in whatever way we want. Using this freedom I expect that it becomes possible to realize the desired behavior.

[liamhuber]

Some notes following a quick synchronous discussion between me and @JNmpi:

The syntax {node.label}_{channel.label} may be a bit dangerous since we use reularly an underscore '_' in our names/labels. ...

I share this concern. Something like using double underscores as suggested is indeed a good first-step to solving it.

Some (super programmatic) thoughts regarding push vs pull workflows. The reason we want to have delayed execution is to avoid for time-consuming executions such as DFT runs that changing the input is going to immediately start execution, even if the changes are not complete....

We have a couple tools for this:

• signals which allow us to manually control
• Forcing (groups of) input to be updated before they are considered ready again (thus preventing updates from proceeding to run until all of the group is updated)

We talked through the "while" schematic above, as well as a hypothetical macro where a DFT node gets its structure input once, then runs cyclically with encut and kpoints getting updated. In such a case it's not enough for a workflow or macro parent to convert all children to run_on_updates, because then the DFT would double-run on encut and kpoint updates, respectively. It's also not enough for the workflow to say run_on_updates, but wait for all of your (connected!) input to get updated after each run -- because then it would hang forever since structure is connected but not updated. Ideally, the parent would be able to do topological analysis to see which inputs are cyclically connected and then group only these as requiring update before running again...but I don't know enough graph math to program this promptly. So, in the meantime, users can instantiate expensive nodes as Slow, then manually set their update policies. We should just make the tools for doing this as easy as possible until such time as we come up with a robust way to automate it!

I'll try to get the schematic example running before Monday -- it just requires defining an if signal node and a simple >= data node -- but I've got some other stuff on my plate and it might not happen until next week.

Also some thoughts regarding the issue Initial input as args:

This was a misunderstanding on my part. I was confused because our decorators do return a class! Joerg clarified his code returns a class instance. He's spot-on that we can simply modify __call__ and get to accepting *args as input even before we have @samwaseda's automatic labeling implemented. With the caveat that I haven't sat down and tried to implement it yet, I think we can even include the __call__ modifications right on the base Node class and just refactor out a set_collective_input method and apply it in both __init__ and __call__.

[liamhuber]

@JNmpi I made a demo that implements the sketch in the comment above. It runs on the latest pyiron_contrib.workflow stuff, i.e. on this draft branch

The setup looks like this:

# Node definitions
# This is pretty verbose, but just because I want print statements and
# because there are no flow-control classes implemented in the library yet
# So in real cases these would most be imports from node packages
# I'm glad we've got a big enough toolbox to do this type of development
# right in the notebook though!

@Workflow.wrap_as.single_value_node("rand")
def numpy_randint(low=0, high=20):
    rand = np.random.randint(low=low, high=high)
    print(f"Generating random number between {low} and {high}...{rand}!")
    return rand

class GreaterThanLimitSwitch(Function):
    """
    A switch class for sending signal output depending on a '>' check
    applied to input
    """

    def __init__(self, **kwargs):
        super().__init__(self.greater_than, "value_gt_limit", **kwargs)
        self.signals.output.true = OutputSignal("true", self)
        self.signals.output.false = OutputSignal("false", self)

    @staticmethod
    def greater_than(value, limit=10):
        return value > limit

    def process_run_result(self, function_output):
        """
        Process the output as usual, then fire signals accordingly.
        """
        super().process_run_result(function_output)

        if self.outputs.value_gt_limit.value:
            print(f"{self.inputs.value.value} > {self.inputs.limit.value}")
            self.signals.output.true()
        else:
            print(f"{self.inputs.value.value} <= {self.inputs.limit.value}")
            self.signals.output.false()

@Workflow.wrap_as.single_value_node("sqrt")
def numpy_sqrt(value=0):
    sqrt = np.sqrt(value)
    print(f"sqrt({value}) = {sqrt}")
    return sqrt

# Graph definition
# Including data flow
# To handle execution control for cyclic flows we turn off some of the
# automated running
# I also stop the headmost node from running at instantiation,
# but that's for output prettiness, not functionality

wf = Workflow("rand_until_big_then_sqrt")

wf.rand = numpy_randint(update_on_instantiation=False)

wf.gt_switch = GreaterThanLimitSwitch(run_on_updates=False)
wf.gt_switch.inputs.value = wf.rand

wf.sqrt = numpy_sqrt(run_on_updates=False)
wf.sqrt.inputs.value = wf.rand

# Finally, define the flow control
# To avoid a race condition, we let rand push out its data, 
# _then_ trigger the switch with signal control
wf.gt_switch.signals.input.run = wf.rand.signals.output.ran
wf.sqrt.signals.input.run = wf.gt_switch.signals.output.true
wf.rand.signals.input.run = wf.gt_switch.signals.output.false

This is a bit long, but without the comments and if you assume the individual nodes are part of a node package somewhere and compress it down, it's just seven lines:

wf = Workflow("rand_until_big_then_sqrt")

wf.rand = numpy_randint(update_on_instantiation=False)
wf.gt_switch = GreaterThanLimitSwitch(run_on_updates=False, value=wf.rand)
wf.sqrt = numpy_sqrt(run_on_updates=False, value=wf.rand)

wf.gt_switch.signals.input.run = wf.rand.signals.output.ran
wf.sqrt.signals.input.run = wf.gt_switch.signals.output.true
wf.rand.signals.input.run = wf.gt_switch.signals.output.false

Ideally we want to call wf.run(), which should run the headmost node(s) on the graph. This isn't implemented yet, so we'll just invoke our starting node manually:

wf.rand.update()
print(wf.sqrt)

We can repeat those two lines as many times as we like to get output like:

Generating random number between 0 and 20...14!
14 > 10
sqrt(14) = 3.7416573867739413
sqrt (NumpySqrt) output single-value: 3.7416573867739413

Generating random number between 0 and 20...0!
0 <= 10
Generating random number between 0 and 20...3!
3 <= 10
Generating random number between 0 and 20...15!
15 > 10
sqrt(15) = 3.872983346207417
sqrt (NumpySqrt) output single-value: 3.872983346207417

Generating random number between 0 and 20...2!
2 <= 10
Generating random number between 0 and 20...15!
15 > 10
sqrt(15) = 3.872983346207417
sqrt (NumpySqrt) output single-value: 3.872983346207417

I'm a little sad that we need to think about race conditions, but by setting it in #761 so that data gets pushed first, then signals get fired, we've got all the necessary tools to make sure everyone has the data they need before they run. There's lots of room for improvement, but the idea is working!

[JNmpi]

Thanks, @Liam, for implementing the example so quickly. Inspired by it I played a bit with workflow concepts and came up with a solution that does not require users to explicitly set signals. It is completely based on connecting data input and output. It is only a simple demonstrator (no decorators, shortcomings in the syntax etc.) but should be sufficient to highlight the concept. I have a couple of ideas to extend it but to start the discussion I attach it below. I provide two examples: A generic one and one which reproduces your workflow above.

Class definition

class Node:
    def __init__(self, name, dependencies=None):
        self.name = name
        self.dependencies = dependencies if dependencies else []
        self.result = None

    def add_dependency(self, dependency):
        self.dependencies.append(dependency)

    def process(self):
        # Override this method in derived classes
        return self.result

    def pull(self):
        self.process()
        return 
        # delete above two lines to get back to original code
        if self.result is None:
            for dependency in self.dependencies:
                dependency.pull()
            self.process()

class Workflow:
    def __init__(self):
        self.nodes = []

    def add_node(self, node):
        self.nodes.append(node)

    def run(self):
        # TODO: use user input or graph analysis to set output 
        # The following is a very pragmatic and simple solution        
        top_node_index = -1
        top_node = self.nodes[top_node_index]
        top_node.pull()
        # for node in self.nodes:
        #     node.pull()
        self.output = top_node.result 
        return self.output

    def visualize(self):
        from graphviz import Digraph
        from IPython.display import display      

        dot = Digraph(comment='Workflow')

        # Add nodes to the graph
        for node in self.nodes:
            dot.node(node.name, node.name)

        # Add edges for dependencies
        for node in self.nodes:
            for dependency in node.dependencies:
                dot.edge(dependency.name, node.name)

        # Display the graph
        display(dot)             

Example: 1

# Example usage:
class AddNode(Node):
    def __init__(self, name, a, b):
        super().__init__(name, dependencies=[a, b])
        self.a = a
        self.b = b

    def process(self):
        self.result = self.a.process() + self.b.process()
        return self.result

class MultiplyNode(Node):
    def __init__(self, name, a, b):
        super().__init__(name, dependencies=[a, b])
        self.a = a
        self.b = b

    def process(self):
        self.result = self.a.process() * self.b.process()
        return self.result

# Creating nodes
a = Node("a")
b = Node("b")
add_node = AddNode("add", a, b)
multiply_node = MultiplyNode("multiply", add_node, b)

# Setting initial values
a.result = 3
b.result = 4

# Creating a workflow and adding nodes
workflow = Workflow()
workflow.add_node(a)
workflow.add_node(b)
workflow.add_node(add_node)
workflow.add_node(multiply_node)

# Running the workflow
workflow.run()

28

workflow.visualize()

Liam's example

# Liam's example:
class Random(Node):
    def __init__(self, name, low=0, high=20):
        super().__init__(name, dependencies=[])
        self.low = low
        self.high = high

    def process(self):
        import numpy as np

        rand = np.random.randint(low=self.low, high=self.high)
        print(f"Generating random number between {self.low} and {self.high}...{rand}!")
        self.result = rand
        return self.result

class GreaterThanLimitSwitch(Node):
    def __init__(self, name, rand, threshold=10, max_attempts=100):
        super().__init__(name, dependencies=[rand])
        self.rand = rand
        self.threshold = threshold
        self.max_attempts = max_attempts

    def process(self):
        print (f'compute {self.name}')
        for i in range(self.max_attempts):
            r = self.rand.process()
            if r > self.threshold:
                self.result = r
                return self.result

        raise ValueError(f'Threshold={self.threshold} not exceeded within max_attempts={self.max_attempts} steps')  

class SquareRoot(Node):
    def __init__(self, name, arg):
        super().__init__(name, dependencies=[arg])
        self.arg = arg

    def process(self):
        import numpy as np

        print (f'compute {self.name}')
        arg = self.arg.process()
        self.result = np.sqrt(arg)
        print(f"sqrt({arg}) = {self.result}")
        return self.result

rand_node = Random('rand')
switch_node = GreaterThanLimitSwitch('switch', rand_node)
sqrt_node = SquareRoot('sqrt', switch_node)

# Creating a workflow and adding nodes
workflow = Workflow()
workflow.add_node(rand_node)
workflow.add_node(switch_node)
workflow.add_node(sqrt_node)

workflow.visualize()

workflow.run()

compute sqrt
compute switch
Generating random number between 0 and 20...10!
Generating random number between 0 and 20...2!
Generating random number between 0 and 20...12!
sqrt(12) = 3.4641016151377544

3.4641016151377544

[liamhuber]

@JNmpi, nice, I like defining a "dependencies" list for pull modes.

However, I don't think it's the pull mode that is making the it possible to avoid using signals. Rather the two big differences I see are that (a) one of the nodes takes a node instance as input -- I think we eventually want to allow this type of behavior, but I think it represents a more complex concept of a "meta node"; certainly it is certainly harder to make a GUI for than "regular" -- and (b) is that there is explicitly a for loop inside one of the nodes -- with signals we get for/while behaviour without ever have to write for other while.

I believe we can easily have a similar "data only" push-mode approach with the existing architecture if we allow the same features (node instance as input and internal for loop). I agree that going forward we will want to allow for such meta nodes that allow other nodes as input, but I also really like that it's possible to avoid this with the signals architecture.

[JNmpi]

@liamhuber, thanks for your super-fast reply.

It would be good to have a more in-depth discussion via Zoom. Just a few quick thoughts/ideas regarding your comments:

• Taking a node as input: This is the case only in the present implementation. I was actually thinking to add to each output object a link to the parent node. This would allow to use as input the output of a node as well as to make a pull on the output object.
• explicit for loop in node: It is interesting that you describe it as an undesired feature. I actually liked it since it provides (at least for me) a very intuitive syntax both for the node definition and its application. Thus, from my perspective, I am happy to replace signals by an internal for loop. I guess I have to better understand where your objection comes from.

[liamhuber]

Hi @JNmpi,

We can absolutely talk about it in more detail on Monday. I think I can offer a quick explanation here to get that ball rolling though:

My serious concern is how this implementation destroys idempotency. In this particular example it's totally benign, because rand_node has fixed inputs, it's output doesn't get used anywhere else, and the graph is anyhow executing all in serial. However, I can imagine that switch_node (or more generally "the accepting node") modifies the input of rand_node (or more generally "the passed node"), and that the output of the passed node is also used somewhere else in the graph. Since the loop and manipulation of the passed node are hidden inside the accepting node, it becomes very difficult to know what data other nodes using the passed node's output wind up seeing. If any of these are executing non-modaly, the problem becomes much worse, as internal execution calls from the passed node may interfere/interact with execution calls at the graph level.

I think there is room for this type of behaviour, where the accepting node either makes a copy or just accepts a class instead of an instance to begin with -- then we can internally have for loops that are either creating new sub-graphs (e.g. for parallel computation) or doing something extremely similar to here, but now on totally internally owned data (copies of the passed node) such that we don't worry about modification of externally used data. It's also possible we may find a solution that uses a raw node instance as here, but somehow manages to otherwise control and constrain the side-effects of being non-idempotent.

I also didn't mean to imply that we should never have for-loops inside node functions, just that I was very happy to be able to make a loop over a node purely at the graph level, without having to write a for loop.

[JNmpi]

Thanks, @Liam. We can talk, e.g., tomorrow after the pyiron meeting.

I fully agree with your concern regarding idempotency. I had thought about it and see a couple of solutions, which we can discuss tomorrow. While I am fully open and happy to use explicit signals to realize constructs like for-loops, I see the potential pitfalls of the proposed concept but also how things can get much simpler. The concept reminds me a lot on the yield statement in Python, which cleanly separates the generation of new list items from the execution of the for-loop. This is one of the features I really like in Python. Let's talk one of the next few days more about these issues. I am looking already forward to it.

[liam]

Please stop using @liam. It doesn't inform the person you want.

On Sun, 9 Jul 2023, 19:22 JNmpi, @.***> wrote:

Thanks, @liam https://github.com/liam. We can talk, e.g., tomorrow after the pyiron meeting.

I fully agree with your concern regarding idempotency. I had thought about it and see a couple of solutions, which we can discuss tomorrow. While I am fully open and happy to use explicit signals to realize constructs like for-loops, I see the potential pitfalls of the proposed concept but also how things can get much simpler. The concept reminds me a lot on the yield statement in Python, which cleanly separates the generation of new list items from the execution of the for-loop. This is one of the features I really like in Python. Let's talk one of the next few days more about these issues. I am looking already forward to it.

— Reply to this email directly, view it on GitHub https://github.com/pyiron/pyiron_contrib/issues/756#issuecomment-1627789139, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAGRFBF6U45FDBJBDKL7RDXPLZFHANCNFSM6AAAAAAZ2IUODE . You are receiving this because you were mentioned.Message ID: @.***>

[JNmpi]

@liamhuber, do you have time for a Zoom meeting now? (my other meeting is already over)

[liamhuber]

Yep, getting a coffee refill and brt

[JNmpi]

Shall we take the pyiron Zoom link?