Return better output shape for Loop with zero iterations #1233
Conversation
…en there are no iterations.
For a loop output (vs. loop carried dependency) the first dimension is the iteration count so will have a value of 0 and the output size will be zero. Use the rank of the matching subgraph output if available.
If the subgraph output rank is not available output a warning and use a rank 1 shape of {0}. We could possibly make a throw-away call to ExecuteGraph to get the rank from the fetches in that call if this proves to be insufficient.
| @@ -399,10 +399,33 @@ Status LoopImpl::Execute(FeedsFetchesManager* ffm, const FeedsFetchesManager* ca | |||
| copy_tensor_from_mlvalue_to_output(feeds[i + 2], i); // skip iter# and cond | |||
| } | |||
|
|
|||
| // create empty outputs for loop outputs | |||
| TensorShape empty; | |||
| // create empty outputs for loop outputs using the subgraph output shapes for the rank | |||
There was a problem hiding this comment.
It sounds like this is an incompleteness (and ambiguity) in the existing Loop spec. It seems to me that the spec needs to be fixed in ONNX in a compatible fashion. The trouble is that this is making the semantics of the loop dependent on shape-inference … so far, shape-inference has always been for optimization, with no effect on the semantics or specification. That could be a potential concern. Would it be better for the Loop op to have an attribute specifying the desired output-shape (for the no-iteration case) as an attribute? Or, would it be better for the model itself to reshape the output as desired? It would be useful to understand the use-case/example where the current behavior (an output of shape [0]) is insufficient or is a problem.
There was a problem hiding this comment.
The current behavior is to output an empty shape, which I think is incorrect as that really represents a scalar.
Specific use case is the ssd_mobilenet model where there are NonZero nodes that lead up to a Loop. If there are no matches from the NonZero the Loop has zero iterations. The current behavior of outputting an empty shape breaks downstream (which is the issue I'm trying to fix). If I change it to return a rank 1 shape with a value of 0 it's happy, but that is most likely model specific as it's expecting scalar output from each Loop iteration so a rank 1 value is by chance correct.
It would be nicer if the model had a check that the NonZero node produced output but I don't know how viable it would be to require that.
I also don't know whether hitting a Loop with zero iterations will be a common problem so it's hard to say how comprehensive the solution needs to be. Whatever the rank is, the value for Size() returned by the shape is going to be zero so anything downstream is going to get empty data.
There was a problem hiding this comment.
Yes, an empty shape denoting a scalar would be wrong. I thought it was returning rank 1 shape with size 0. I agree that the most logical-return-shape would be NumIteration X Per-Iteration-Shape with NumIteration=0. Ideally, the Per-Iteration-Shape should be obtained from the model (e.g., as specified in the output-shape info of the loop-body subgraph). But since that may be unspecified or may have unknown dimensions, we end up with the scenario where the output-shape depends on shape-inference … this means that if somebody "improves" the shape-inference logic for some op, the value returned by inference will change … which seems questionable. I am just thinking aloud as to what the ONNX specification should say … perhaps something like "Users should specify a constant output-shape in the body-graph to get a corresponding output-shape in the zero-iteration case. Alternatively, the model should reshape the output into desired shape explicitly after the loop, e.g., in cases where the shape cannot be statically specified."
There was a problem hiding this comment.
The potential ONNX spec update wording sounds fine to me. Did you want to propose that change?
raymondxyang
deleted the
skottmckay/Loop_ImproveZeroIterationsOutputShape
branch
Attempt to provide the correct rank for an output from a Loop node when there are no iterations instead of returning an output with an empty shape.
For a loop output (vs. loop carried dependency) the first dimension is the iteration count so will have a value of 0 and the output size will be zero. Use the rank of the matching subgraph output if available.
If the subgraph output rank is not available output a warning and use a rank 1 shape of {0}. We could possibly make a throw-away call to ExecuteGraph to get the rank from the fetches in that call if this proves to be insufficient.