[graph] Method to test if placeholder is an output

[bertmaher]

Description: I've often wanted to know whether a placeholder is an output of a function, but we lack a succinct way of doing that test. So I've added one :-)

Testing: new graphTest case

Documentation: n/a

[bertmaher]

Two questions I had while writing this:

1. This could be confusing if the same Placeholder is shared by multiple Functions in a Module, and is an output of one but not another. Should isOutput take a Function to test against?
2. Are there some logical places we could use this elsewhere in the codebase? I've only added it in a test case so far.

[rdzhabarov]

related to #1953

[rdzhabarov]

Are there some logical places we could use this elsewhere in the codebase? I've only added it in a test case so far.

See the issue above. We copy both inputs/outputs when we should not do it.

[jfix71]

This could be confusing if the same Placeholder is shared by multiple Functions in a Module, and is an output of one but not another. Should isOutput take a Function to test against?

Yeah, I think it makes sense to take a Function to test against. Given the use case @rdzhabarov mentioned, where it's used to determine what to copy over to a device for executing a Function, you would want to check if the Placeholder is an output of that Function you're executing.

[rdzhabarov]

we'd similarly need isInput().
Probably !isOutput should be good, unless we could make isInput more efficient in implementation

[nadavrot]

Some nodes can be inputs and outputs. For example, quantization nodes. How do we handle them?

[rdzhabarov]

Good point! Profiling nodes are kinda special (similar thing would happen with training placeholders, but we have a way to check isTraining).

• one simple solution would be to have an enum for type of placeholder { in, out, inout } and return it here.
• another idea is to force placeholders for profiling nodes to be trainable (since we change values and then re-read them, similar process to training).
• something else?

First approach seems to be cleaner.

[jfix71]

Another option could be to use isOverwrittenNthInput() -- we use it for checking for these other cases (profiling and training), right? So we could also add a check here for if a user is using the Placeholder as an overwritten input. Then we would also need a separate isInput().

[rdzhabarov]

@nadavrot raised a good point about in/out placeholders.

Let's sort it out before merging

[qcolombet]

Given the in and out properties are part of the protobuf description (at least in C2), could we tag the placeholder upfront?
As for the quantization node problem, it feels to me like an exception that we don't really care in the sense that real network won't have those.

[bertmaher]

Yeah, tagging the output PH up front maps pretty well to what I actually want out of this method. Of course there's still the matter of the 600 or so usages of createPlaceholder from the C++ APIs in unit tests :-(.

[nadavrot]

I think that one way to move forward here is to change the name of the method into something like "isSavedInto" or something like that. In the future we should probably adopt the in/out attributes that the low-level IR has to the graph. What do you think?

[bertmaher]

I'd be cool with a name-change in the short term (with a long-term desire to add attributes). I guess I also don't have a super-pressing need to land this change in the master branch. I was going to look at the use-case in the runtime so that this change is actually used by non-test code :)

[nadavrot]

Approving this PR because it sounds like we have a way to move forward.

[bertmaher]

So I realized this morning that the way I was using this in a backend was just incorrect (for the curious, I was looking for SaveNode outputs during CompiledFunction::execute, which assumes the Function still exists, which is not guaranteed). Since I don't have any other use of this method planned, I'd rather not provide others with a convenient footgun. If this is useful at some future point, it won't be hard to re-create this PR.