Major Issue: Nodes with multiple inputs/outputs cannot be represented in the current format about nir

[DylanMuir]

Hi all, I hope it's ok that I jump in!

The graph representation in Rockpool addresses these issues, and so could serve as an example of one possible approach to solve the problem (in a battle-tested form).

You can get the overview from the Rockpool docs here: https://rockpool.ai/advanced/graph_overview.html

Basically, we have GraphModule objects (equivalent to NIR Nodes, I believe), within which we enumerate the input and output channels individually. GraphModules are connected together using "graph edge" objects (called GraphNodes in Rockpool), which represent connectivity only and no computation.

The result is we have full flexibility on connectivity, including splits, joins, skips, partial connectivity, etc. We always use the graph edge objects between GraphModules, so all the graph traversal machinery knows how to handle them.

If you would like me to clarify any of the design on a call, I'm very happy to do so.

[Jegp]

Thanks for the explanations. I think it made it clearer to me, although I'm still not convinced of a good way to go about this. I think it'd be tremendously helpful if you could tell us your thinking about the Rockpool graphs @DylanMuir! And please, by all means jump in as often as you can!

Would it be helpful to schedule a call for those interested to (a) fully map the problem, and (b) discuss the Rockpool graphs as a solution? If so, throw a 👍 and I'll send out a Doodle.

[Jegp]

I'm not sure I understand the premise. Why would the current graph representation assume a single input/output for a node?

Are you referring to the connection from/to a NIRGraph? In that case, I agree it's a problem and that there are currently no strong way to map which input/output goes where. I, personally, like the subscript approach where any connections into a graph would be aware of which "wire" they plug into. However, that doesn't resolve the situation where the outermost node is a NIRGraph. How would we decide how to plug into the graph if it has, say, two input nodes?

[matjobst]

I also like the subscript approach, but since we reference the nodes with their name when defining the edges, I would propose a fully string-based version that looks more object-oriented, so one edge would be for example ("in", "nodeB.in1"). That would signify an edge from the toplevel graph input "in" to the input "in1" of the subgraph "nodeB".

This is an example of how this could look like in practice:

 inner_model = nir.NIRGraph(
    nodes={
        "in1": nir.Input(np.array([4, 5, 2])),
        "in2": nir.Input(np.array([4, 5, 2])),
        "flat": nir.Flatten(0),
        "out1": nir.Output(np.array([20, 2])),
        "out2": nir.Output(np.array([20, 2])),
    },
    edges=[
        ("in1", "flat"),
        ("in2", "flat"),
        ("flat", "out1"),
        ("flat", "out2")
    ]
)
outer_graph = nir.NIRGraph(
    nodes = {
        "in": nir.Input(np.array([4,5,2])),
        "inner": inner_graph,
        "out": nir.Output(np.array([20,2]))
    },
    edges=[
        ("in", "inner.in1"),
        ("in", "inner.in2"),
        ("inner.out1", "out"),
        ("inner.out2", "out")
    ]
)

[sheiksadique]

Thanks for the quick PR @matjobst . I have to highlight that this issue is not limited to Graphs but also Nodes because there could very well be nodes/modules that take in multiple inputs that have different meaning and then produce multiple values as outputs.

So the fix need to be extended to the Node definition perhaps and not just to the Graph

[sheiksadique]

An alternative implementation that came out of brainstorming with Carsten:

Keep the Node definition as it is now and introduce combinator/selector Nodes to deal with multi-dimensional IO. This representation would involve adding introducing a node between a projection between two nodes/modules that transforms an output into a form that is amenable or appropriate to the next node.

An implicit assumption here is that if there are two converging inputs to a node/graph, they will be added up. Assuming these inputs have to be a tuple of two tensors, the model will try to add them up the inner values/tensors.

This implementation would mean that the node and graph implementations are kept minimal/as they are, but a few more node types will be added for the combinator/selector functionality.

[DylanMuir]

Sure Jens, feel free to send a doodle.

[matjobst]

Here are my thoughts about the current proposals:

subscript string-based version (á la edges=[("in", "inner.in1")]):

• positive:
  • this does not require any code changes in NIR
  • in my opinion this is human readable and seems intuitive to me (you plug into named inputs and outputs)
  • this is not even limited to subgraphs, we can also easily define nodes to have multiple inputs and outputs. This would only be a matter of documentation (and then of course correct implementation in the target frameworks).
• negative:
  • this affects graph traversal code (but that would likely be true for any solution)
  • string parsing required when parsing the graph
  • good documentation on inputs and outputs of multi-in and multi-out nodes required
  • not "safe", you can easily create invalid graphs, but that was already true before because of the string-name based edges
  • open point: how do we attach to nodes with single inputs and outputs: do we still require a name for the inputs and outputs? This could be a source of unneded verbosity. If we do not require a name, then there might be ambiguity.

@sheiksadique 's Project proposal:

• positive:
  • nodes stay "simple", they always have one input and output
  • no string parsing for graph traversal required
• negative:
  • requires an additional node type
  • requires additional nodes in the graph
  • in my opinion less human readable and slightly unintuitive
  • you can still create invalid graphs, but push one possibly "unsafe" point from the edge definition into the Project node
  • good documentation on inputs and outputs of multi-in and multi-out nodes required

[Jegp]

Closed with #50