OnnxTransform: Fix 3 bugbash bugs #1080
Conversation
|
|
jignparm
requested review from
jignparm,
Ivanidzo4ka,
shauheen,
abgoswam and
yaeldekel
| Input = args.InputColumn; | ||
| Output = args.OutputColumn; | ||
|
|
||
| var outputNodeInfo = Model.GetOutputsInfo().Where(x => x.Name == args.OutputColumn).First(); | ||
| var outputNodeInfo = Model.ModelInfo.OutputsInfo[0]; |
There was a problem hiding this comment.
ar outputNodeInfo = Model.ModelInfo.OutputsInfo[0]; [](start = 13, length = 51)
Why you made this change? Why you make this restriction on output column? #Closed
There was a problem hiding this comment.
I removed the restriction. Now the onnx_model input/output node names don't matter, b/c there's only 1 input and 1 output. We just tie them to whatever IDV column is specified as input/output, w/o forcing the names to match.
In reply to: 221091553 [](ancestors = 221091553)
There was a problem hiding this comment.
How many models out there have only one input and one output? By imposing this restriction you denied any model which not suite this criteria.
What if user want's output from hidden layer in DNN, or some layer which is not last one in execution graph?
If problem with output node's names, why can you accept two strings for input and output, one as IDV column name, and other as ONNX name?
In reply to: 221109171 [](ancestors = 221109171,221091553)
There was a problem hiding this comment.
In Tensorflow, there are many examples of models with multiple inputs and outputs. But for Onnx , all the models in the zoo (see https://github.com/onnx/models) have exactly 1 in/output. . We can probably add support for multiple input/outputs as part of ML.net v0.7, to make it future proof, but currently this should cover 100% of all existing pretrained Onnx 1.2 models.
The change above gives more flexibility on how the user wants to name the IDV output column (i.e. it doesn't have to be same name as model)
In reply to: 221112683 [](ancestors = 221112683,221109171,221091553)
There was a problem hiding this comment.
@wschin please review this.
There was a problem hiding this comment.
Sorry, do we disagree with @Ivanidzo4ka's proposal? It seems quite sensible to me. You have an output column name in IDV land, but what it corresponds to in tensorflow land is incidental.
As it happens, Ivan's design also makes it possible to make a well written pigstention, which currently is quite difficult under the current rather inflexible design. #Closed
There was a problem hiding this comment.
We resolved this after the scrum today with Ivan, Pete, Jignesh, Yael.
Conclusion: No change is needed.
Summary: B/c sonoma API only allows access to inputs and outputs (not intermediate layers), and all the onnx models in model zoo have just 1 input and 1 output, we can keep OnnxTransform API as is.
In reply to: 221142181 [](ancestors = 221142181)
|
@shmoradims please use the correct notation when associating with multiple issues. #Resolved |
|
sure In reply to: 425318353 [](ancestors = 425318353) |
ghost
locked as resolved and limited conversation to collaborators
Fixes #1050 #1051 #1053