lang: CallExpr must generate a subgraph #697
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Representing an MCL function value as a golang function from Value to
Value was a mistake, it should be a function from Vertex to Vertex.
Here is why this is a mistake:
The output of a function like
$f = fn(x) {
Shell(Sprintf("seq %d", x))
}
varies over time, while a single Value does not. Thus, code like
Map($f, list(1, 2))
would first produce the value list("1", "1"), but then it would
_not_ update to list("1", "2") when "seq 2" produces its second
line. That's because with the mistaken design, when Map receives a
new FuncValue or a new input list of N elements, Map calls the
function from Value to Value N times and produces a single output
list of N elements.
Here is why the corrected design is better:
Here's what happens with this new design when Map receives a new
FuncValue or a new input list of N elements.
First, Map constructs N item-input nodes, each of which extracts a
different entry from the list. Then, Map calls the function from
Vertex to Vertex N times, once for each item-input node, and thus
obtain N item-output nodes. Finally, Map constructs an
item-collecting node which constructs a list out of all of its
inputs, and Map connects the N item-output nodes to the
item-collecting node. This item-collecting node is the output of
Map.
The Vertex to Vertex function constructs and connects its own nodes;
in this case, it constructs an Sprintf node and connects the
item-input node to it, and then constructs a Shell node and connects
the Sprintf node to it, and then returns the Shell node as the
item-output node.
The two Shell node in this sub-graph emit a first value "1", which
propagates to the item-collecting node and causes it to output a
first value list("1", "1"). Then, the second Shell node emits a
second value "2", which propagates to the item-collecting node and
causes it to output a second value list("1", "2"), as desired.
Here is how this commit brings us closer to the above plan:
Changing FuncValue throughout the codebase is a big change. One of
the difficulties is that it is not just nodes which are emitting
FuncValues, there are also many other places in the code where
FuncValue is used to hold a golang function from Value to Value.
Some of those places now need to hold a golang function from Vertex
to Vertex, but other places still need to hold a golang function
from Vertex to Vertex.
Thus, as a first step, we need to split FuncValue into two types.
This commit splits the old FuncValue into two types:
1. The new FuncValue will hold a function from Vertex to Vertex.
FuncValue is a Value.
2. A new type named "SimpleFn" will hold a function from Value to
Value. SimpleFn is not a Value.
This commit replaces occurrences of the old FuncValue with one of
those two new types, as appropriate. This commit does not yet adapt
the surrounding code to make use of the new representation; that
will be done in future commits. I have annotated the missing parts
with the following panic message in order to make it easy to find
which parts still need to be implemented. The "..." part explains
what needs to be implemented.
panic("TODO [SimpleFn]: ...");
Here's where I need help:
One part of the code which is not clear to me are the parts which
use reflection. I don't understand the purpose of that code well
enough to explain what needs to be implemented. I have annotated
those "known unknown" parts of the remaining work with the following
panic message in order to make it easy to find which parts still
need more thinking and planning:
panic("TODO [SimpleFn] [Reflect]: ...");
|
This PR builds on top of #696, and so the diff will be much easier to review after that one is merged. This is the majority of the work @purpleidea and I did this weekend during our pairing session. It is relatively self-contained because it all pertains to porting |
This will eventually let functions change the running graph via a transaction API. At the moment the core Lock and Unlock primitives aren't implemented.
This is useful for the common case in which we call one FuncValue to construct a bunch of nodes, and later we switch to a different FuncValue and so we want to remove all the nodes added by the first FuncValue and replace them by the nodes added by the second FuncValue.
Merging those two packages allows us to avoid import cycles when a Func needs to add an Expr to the graph.
FuncValues are now manipulating the graph instead of manipulating values, so the logic for calling a FuncValue must now follow suit.
gelisam
force-pushed
the
gelisam/call-as-graph-transformer-with-james
branch
from
dc8d2e4 to
269e567
Compare
|
force-pushed to remove a temporary commit which I did not mean to include in this PR. |
|
The tests fail, but as in #696, this is expected, because a lot of |
|
This PR has now been incorporated into #704. Closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
FuncValues are now manipulating the graph instead of manipulating values, so the logic for calling a FuncValue must now follow suit.