Added RFC for Flow Graph Helper Functions #1648
Conversation
| * Collecting feedback on user experience confirming the choices made on the open questions and limitations: | ||
| * Limitation that constructors can be used to set predecessors or successors but not both. | ||
| * The multiport node rule that makes edges from each node in the set to the corresponding port. | ||
| * The corresponding oneTBB specification update should be done backed by the user feedback provided. |
There was a problem hiding this comment.
With regard to the specification update, I would explicitly ask the following questions:
- Should we keep the "node set" type unspecified or define it properly?
- Should the ordering (preceding/following) become the property of a node set?
| ## Introduction | ||
|
|
||
| The flow graph API that lacks an simple way to create multiple edges with a single call. The result of this | ||
| limitation is verbose, less-readable code. For example, the follow pciture shows a flow graph where the |
There was a problem hiding this comment.
A typo. Also, form an adjective out of the noun?
| limitation is verbose, less-readable code. For example, the follow pciture shows a flow graph where the | |
| limitation is verbose, less-readable code. For example, the following picture shows a flow graph where the |
| limitation is verbose, less-readable code. For example, the follow pciture shows a flow graph where the | ||
| node `input` has three successors. | ||
|
|
||
| <img src="graph_with_many_edges.png" width=500> |
There was a problem hiding this comment.
I wonder why the width specified to be 500? Should it fit the smartphone's screen width? And if yes, will it? Should we avoid specifying it to have the default value?
| <img src="graph_with_many_edges.png" width=500> | |
| <img src="graph_with_many_edges.png" width=500> |
|
|
||
| <img src="graph_with_many_edges.png" width=500> | ||
|
|
||
| Without the proposed extensions, three separate calls to `make_edge` are require to connect the |
There was a problem hiding this comment.
A typo?
| Without the proposed extensions, three separate calls to `make_edge` are require to connect the | |
| Without the proposed extensions, three separate calls to `make_edge` are required to connect the |
| - make_node_set function template | ||
| - make_edges function template | ||
| - two additional constructors for each flow graph node | ||
| - follows and precedes function templates |
There was a problem hiding this comment.
| - make_node_set function template | |
| - make_edges function template | |
| - two additional constructors for each flow graph node | |
| - follows and precedes function templates | |
| - `make_node_set` function template | |
| - `make_edges` function template | |
| - two additional constructors for each flow graph node | |
| - `follows` and `precedes` function templates |
|
|
||
| There are two ways to connect nodes in a set and a single node using make_edges: | ||
|
|
||
| <img src="make_edges.png" width=600> |
There was a problem hiding this comment.
Here the width of the picture is specified to be 600. What it is the rule adding a picture in the RFC? I don't think we have one so far. Maybe, we should add something with supporting statements describing why it is so and not a different way.
| - two additional constructors for each flow graph node | ||
| - follows and precedes function templates | ||
|
|
||
| ### Node Sets |
There was a problem hiding this comment.
Do we want to say anything about the graph object to which the nodes belong? Something like, the node set should compose from the nodes belonging to the same graph. Is it enforced somehow in the code? Is it really a requirement?
|
|
||
| The object returned by follows or precedes replaces the graph argument, which in other constructors is passed | ||
| as the first argument. The graph argument for the node being constructed is obtained either from the specified | ||
| node set or the sequence of nodes passed to follows or precedes. |
There was a problem hiding this comment.
I have found this in the implementation of the node_set:
graph& graph_reference() const {
return get_graph_helper::get(std::get<0>(nodes));
}This thing is used in the constructors of the nodes. Not sure this implementation detail needs to be exposed in the RFC, but you requested to be accurate. So, a more accurate and precise description would mention something about that. 🙂
There was a problem hiding this comment.
As far as I remember, we agreed that only the nodes that belongs to the same graph can be wrapped into the node set. I think it should be states as undefined behavior somewhere (and in the future specification for sure).
| } | ||
| ``` | ||
|
|
||
| When run, the above code results in: |
There was a problem hiding this comment.
Perhaps, mention possible effect of parallelism:
| When run, the above code results in: | |
| When run, at the accuracy of the `n2` and `n3` execution order the above code results in: |
| 2:200 | ||
| ``` | ||
|
|
||
| The multi-port nodes include `multifunction_node`, `join_node`, `split_node` and `indexer_node`. |
There was a problem hiding this comment.
What about composite_node? It allows having multiple input and output ports as well, isn't it?
There was a problem hiding this comment.
Actially, the composite_node is an interesting aspect of this proposal. If I remember correct, currently it does not support follows and precedes API, but it may be useful to have such an ability for user-provided nodes.
I think we need to investigate on how it can be expressed on the node implementor's side - I think it may result in making the node set public.
Co-authored-by: Aleksei Fedotov <[email protected]>
| 2:200 | ||
| ``` | ||
|
|
||
| The multi-port nodes include `multifunction_node`, `join_node`, `split_node` and `indexer_node`. |
There was a problem hiding this comment.
Actially, the composite_node is an interesting aspect of this proposal. If I remember correct, currently it does not support follows and precedes API, but it may be useful to have such an ability for user-provided nodes.
I think we need to investigate on how it can be expressed on the node implementor's side - I think it may result in making the node set public.
| The multi-port nodes include `multifunction_node`, `join_node`, `split_node` and `indexer_node`. | ||
| The rule for creating edges from sets is the same for each of these types. | ||
|
|
||
| ### `follows` and `precedes` Helper Functions |
There was a problem hiding this comment.
One important thing is missed from the RFC in regard to helper functions - deduction guides.
Current preview feature provides additional extensions for Flow Graph deduction guides while follows and precedes APIs are used. Actually, only them allows to use deduction guides for nodes without the body.
E.g. join_node j(follows(f1, f2, f2)); deduces the node as join_node<std::tuple<o1, o2, o3>> where oN is the output type for the corresponding fN node.
But as far as I remember, one of the issues was with the multifunction_node and async_node - as far as I see they does not support CTAD now even if the simplified API is used. I don't remember much details, but one of the reasons was the fact that to create a multifunction_node you need to define a body that takes node_type::output_ports_type as an argument that requires knowledge about the node_type at it was not yet deducted.
May be we can define some deduction guides for the case when the generic lambda and precedes are provided.
Anyway, we need to investigate it somehow.
|
|
||
| The object returned by follows or precedes replaces the graph argument, which in other constructors is passed | ||
| as the first argument. The graph argument for the node being constructed is obtained either from the specified | ||
| node set or the sequence of nodes passed to follows or precedes. |
There was a problem hiding this comment.
As far as I remember, we agreed that only the nodes that belongs to the same graph can be wrapped into the node set. I think it should be states as undefined behavior somewhere (and in the future specification for sure).
| // node_set is an exposition-only name for the type returned from make_node_set function | ||
|
|
||
| template <typename Node, typename... Nodes> | ||
| /*unspecified*/ make_node_set( Node& node, Nodes&... nodes ); |
There was a problem hiding this comment.
As far as I see, current make_edge function in the general case only allows setting edges between sender and receiver of the same type. Because of this, it seems useless to allow make_node_set to accept nodes with different input_type or output_type. I think we can think about adding reasonable constraints APIs mentioned in this paper.
Description
This adds the description of an already existing experimental feature to the rfcs/experimental directory. The content should be reviewed for accuracy in describing the feature and for the proposed exit criteria.
Fixes # - issue number(s) if exists
Type of change
Choose one or multiple, leave empty if none of the other choices apply
Add a respective label(s) to PR if you have permissions
Tests
Documentation
Breaks backward compatibility
Notify the following users
List users with
@to send notificationsOther information