sklearn-onnx icon indicating copy to clipboard operation
sklearn-onnx copied to clipboard

Add showcase of inputs being unexpectedly renamed

Open cbourjau opened this issue 3 years ago • 3 comments

This Draft PR adds a failing test showing the (IMHO) unexpected renaming of the input variable of the ONNX graph if it is identical to the output variable name.

cbourjau avatar Oct 19 '21 14:10 cbourjau

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Oct 20 '21 00:10 CLAassistant

Thank you very much for taking a look at this! May I ask why the decision was made to give priority to the output names rather than the input names if the two conflict? It seems to me that having the input names as "asked for" is more important since those are mandatory at inference time (with onnxruntime.InferenceSession.run at least) time while the outputs are always returned as an array.

cbourjau avatar Oct 20 '21 17:10 cbourjau

This is a tricky case. Names must be different and they are almost always different. We did not have any unit test covering that specific case and that's why it was not detected. The implementation was changed because when a pipeline contains multiple regressors or classifiers, they go through the same parser. The first model output is labelled "variable", the second one "variable1", the next one "variable2" ... and the final output name was difficult to guess for the user. Now, if the pipeline is a regressor, the final output name is renamed into "variable" and switch name with the intermediate result "variable". The side effect of this logic is the behaviour you highlighted in this PR.

xadupre avatar Oct 20 '21 18:10 xadupre