sklearn-onnx
sklearn-onnx copied to clipboard
Add showcase of inputs being unexpectedly renamed
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.
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.
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.
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.