mOutputNode confusing definition causes issue in Scheduler

Required prerequisites

• Make sure you've read the documentation. Your issue may be addressed there.
• Search the issue tracker and discussions to verify that this hasn't already been reported. +1 or comment there if it has.

What commit version of aidge do you use

• aidge_core: 0.3.0dev

Problem description

GraphView::mOutputNodes attribute used to be any Node within the graph that has no chidren at all or in the graph for at least one output.

This definition has been updated and users can manually add the output node they want. By cancelling the automatic definition of output nodes, the list is not automaticcally updated if two Nodes are connected to each other and are not an output for the graph any more. If Nodes are linked after they have been alread added to the graph, their newly formed connection will not affect the mOutputNode attribute.

Because of this, the provided list of output nodes cannot be trusted. It can be the source of errors like here !203 (diffs) in the Scheduler where a Producer can make its way to the consumer list which later throws an error during the forward() call.

Reproducible example code

You just need to link the nodes after adding them.

import aidge_core as ai

relu_node_0 = ai.ReLU(name="ReLU#1")
relu_node_1 = ai.ReLU(name="ReLU#2")

g = ai.GraphView()
g.add(relu_node_0)
g.add(relu_node_1)

print(g.get_ordered_outputs())
# [
#   (Node(name='ReLU#1'),
#   (Node(name='ReLU#2')
# ]

relu_node_0.add_child(relu_node_1)

print(g.get_ordered_outputs())
# [
#   (Node(name='ReLU#1'), should not be an output node anymore...
#   (Node(name='ReLU#2')
# ]

changed the description

Suggested solution

The real issue is the GraphView::mOutputNodes definition which tries to fit two different roles:

1. Actual outputs of the graph, like stated in the old definition. It can automatically be updated, whether it is when a link is changed between Nodes or everytime it is called
2. Watched outputs of the graph. These outputs are those manually stated by the user or the system

The GraphView structure could be updated by seperating these two definitions into two attributes. Here would be the main additions to the class:

• mOutputNodes actual outputs of the graph
• mWhatchedOutputs manually managed outputs
• outputNodes() return mOutputNodes
• outputs() return mOutputNodes + mWatchedOutputs

What do you think @olivierbichler?

the Scheduler class would use the mOutputNodes list whereas users would fetch mOutputNodes + mWatchedOutputs

Indeed, you're right, there are 2 separate thinks here.

Imho, for the first point (maintainance of mOutputNodes), it should indeed be done on new/removed connections. For the second point (maintainance of user's list of ordered inputs/outputs) the current behavior is fine.

My 2 cents:

Before introducing a separate member mWatchedOutputs, I think your first point should be addressed first, maintaining graph view integrity (including inputs/outputs) it is independent as you said. Either:

1. it's the responsibility of the user to "re-add" nodes with new connections (the current behavior), or
2. or it'as automatic and add_child()/... connections methods should update the related graph views automatically.

I would be in favor of 2 because it's more user friendly, though 1. is documented, hence we can live with this I suppose. If, as you state the decision has been already made at some point for 1., can it be discussed again (does it seem feasible now, complexity, maintenance, etc...)?

For your second point, adding a mWatchedOutputs, actually I'm not sure it would solve the management of user outputs definitions simply. Indeed, even if we introduce this, we will have to maintain it on new/removed nodes, new/removed connections. Hence the work is the same as for mOutputNodes.

Actually, from the user perspective, either:

1. setting inputs/outputs nodes becomes mandatory, and this makes sense (but should be invalidated as soon as the graphview changes),
2. or it is not mandatory and the default inputs/outputs list are maintained as currently and when user has a schedulable graph view he will optionally force inputs/outputs for the ones he's interested in.

The point 2. is the current behavior. imho I think it's fine after playing with in different ways. When importing graphs from external sources (onnx for instance), or creating graph with multiple outputs/inputs, it is natural to first create the graphview and then declare the inputs/outputs at the end added.

Hence, there is no a real advantage to provide the user with 2 lists of outputs in the interface, and, unless it simplifies the implementation, there is no really need to separate the two lists.

As a side note, I do not have a definite opinion on this points, though it seems to me that the current implementation of GraphView is overly complicated (i.e. more than I would expect), if it can be simplified by some way I'm +1. On the contrary any addition internal state to maintain should be added with care, unless if it simplifies the rest of course.

For instance, it's another discussion, but more radically:

• get rid of mRootNode which is not really defined unless for graph matchers, in which case it should be a matcher property
• simplify mostly everything (deterministic orders, including default inputs/outputs orders, graph merges semantic) by maintaining an insertion order in the graphView instead of stucking with sets.

For reference, please check discussion in !53 (merged).

I am a bit surprised by the exampe in the issue, because it still should work. This seems to be a bug for me!

I think I've seen it mentionned somewhere in docstrings or comments, but adding a connection requires to add again the node (at least the parent node of the connection) in order to update correctly the graphview.

Oh, yes, you are right! In the example given in this issue, this behavior has always been so. Because the connection is added directly from node to node, the GraphView is not updated. There is even a function for that: GraphView::updateInputsOutputs().

@pineapple But the automatic definition of output nodes has not been cancelled! It is still there! If you replace relu_node_0.add_child(relu_node_1) with g.add_child(relu_node_1) it should work.

It remains that there could be an enhancement to the interface to allow additional connections while maintaining the integrity of graph views. Actually a node already maintain it's graph view set, hence I suppose it could be feasible to enforce this.

I agree, this should be feasible.

Done in !405 (merged).

For now, I will change the test https://gitlab.eclipse.org/eclipse/aidge/aidge_backend_cpu/-/blob/dev/aidge_backend_cpu/unit_tests/test_scheduler.py?ref_type=heads#L7 that brought this issue to light to allow for other MR to be merged.

changed health status to needs attention

added Bug 🐛 PriorityHigh labels

mentioned in merge request !224 (merged)

changed the description

@pineapple I am not convinced by the mOutputNodes + mWatchedOutputs split. The user will either use one or the other mechanism, and he will have two mechanisms that both have drawbacks without any new advantage... If he relies on mWatchedOutputs, he will be unable to benefit from the whole automatic mOutputNodes process and will have to add each node manually. Conversely, if he uses mOutputNodes, he will not be able to reorganize or add outputs...

It looks like fixing auto updating of GraphViews in the example you provide would be the best solution.

If he relies on mWatchedOutputs, he will be unable to benefit from the whole automatic mOutputNodes process

The goal is for the user to get mOutputNodes + mWatchedOutputs, not mWatchedNodes alone. I am thinking about models where user would need to fetch the output of a node that is already connected to another node. It would also ease debugging

This is already possible with setOrderedOutputs(), although maybe not so convenient, because the user has to list all the outputs, instead of just adding a new output to "watch". But an additional interface could be provided for that without changing the underlying mechanism.

I continue to think that this can be handled well with mOutputNodes alone. What happens if a mWatchedNodes is removed from the graph? What if it gets also added automatically to mOutputNodes upon removing a node? How are mWatchedNodes outputs ordered w.r.t. mOutputNodes? Again, I don't see a clear benefit from having an additional separate list, that would just add more complexity.

Done in !405 (merged): added a addOrderedOutputs() method to just add new output to "watch".

changed milestone to %aidge v0.5.0

added EnhancementStructure 🏗 StatusDelayed TopicGraphView labels

changed milestone to %aidge v0.6.0

mentioned in merge request !405 (merged)

!405 (merged) should close this issue.

closed