Avoid propagating some input columns when applying an Onnx model #5012
Conversation
* Let OnnxDataTransform use the new Bindings to drop columns
| @@ -37,7 +37,7 @@ namespace Microsoft.ML.Transforms.Onnx | |||
| /// <summary> | |||
| /// <see cref="ITransformer"/> resulting from fitting an <see cref="OnnxScoringEstimator"/>. | |||
| /// </summary> | |||
| public sealed class OnnxTransformer : RowToRowTransformerBase | |||
| public sealed class OnnxTransformer : RowToRowTransformerBase // MYTODO: Should I consider not to inherit from this, since now OnnxTransformer would be able to drop columns and not use the RowToRowMapperTransform? | |||
There was a problem hiding this comment.
The RowToRowTransformerBase clearly states that it's only supposed to add columns, not drop or modify any input columns. The whole design of that class, and its use of RowToRowMapperTransform, enforces this. Having to hack it by changing some of its methods into virtual methods feels somewhat hacky and incorrect.
So I would appreciate other thoughts regarding if it's okay to leave this as it is?
I guess I could simply not inherit from that class, and copy its code in here. But, is it worth it? Also, I might be concerned about backward compatibility with ML.NET models that were saved to disk and included OnnxTransformers; I would still need to test if removing this inheritance would affect loading back those models.
| [BestFriend] // MYTODO: Is this necessary? | ||
| internal sealed class Bindings // MYTODO: Should I move this inside OnnxDataTransform? | ||
| { | ||
| // MYTODO: Should I simply inherit from ColumnBindings, since everything is the same except for the constructor (specifically, only, the way it created the _colMap)? |
There was a problem hiding this comment.
So the only difference between this and ColumnBindings is the code in the constructor (specifically only the code that builds _colMap and the output schema).
So I thought that inheriting from ColumnBindings might be an option, but it's sealed and also it seems it's meant to only add columns not remove them. So I've been hesitant to change that. Is it ok to simply copy all that code in here? Or would it be preferable to unseal it and inherit that in here?
| // MYTODO: Is it even worth it to have this OnnxDataTransform class when it (including the RowImpl and Cursor) | ||
| // are identical to the RowToRowMapperTransform? The differences are: | ||
| // - This one expects specifically a OnnxTransformer.Mapper as _mapper from where to get the GetColumnsNames, whereas RTRMT expects a generic IRowMapper | ||
| // - This one has a _bindings object which is off type OnnxTransformer.Bindings, whereas RTRMT expects a generic ColumnsBindings | ||
| // - This one in here has a differend override for the Save method | ||
| // - This one in here doesn't have (but I don't know if it could have) methods related to SaveOnnx, SavePfa, ApplyToData, and VersionInfo of RTRMT. | ||
| // - RTRMT has an extra member called "_mapperFactory" that is used in ApplyToData |
There was a problem hiding this comment.
The only differences between RowToRowMapperTransform and OnnxDataTransform are listed here. The main and central one is simply that OnnxDataTransform uses the Bindings class I added in this PR. Pretty much all the code is exactly the same.
So I don't know if it's worth it to copy all of this code. I can think of alternatives to enable ColumnBindings to drop columns, so that there's no need to create the Binding class in here and the OnnxDataTransform in here, and reuse the existing classes instead. Would have to test if it works.
So, what do people think? Is it ok to copy all of these code, or should I try to reuse the existing one in the other classes since it's nearly identical? I've been hesitant to reuse them since, again, they seem to be designed for only adding columns, so it might be against the design and code readability to hack this behavior into those classes.
| // MYTODO: Is this side effect acceptable? Should I remove this from this test? | ||
| // OnnxTransformer will drop all the columns in the input that have | ||
| // a corresponding input in the onnx model. | ||
| // A side effect of this, is that after applying a pretrained onnx model | ||
| // like the one in here, we won't be able to use or access the input | ||
| // columns of the model. | ||
| var transformedData = pipe.Fit(dataView).Transform(dataView); | ||
| var col = transformedData.Schema["data_0"]; |
There was a problem hiding this comment.
One side effect of dropping all the input columns that were used by the onnx model, is that, obviously, we won't be able to access those input columns after applying the onnx model.
In scenarios like the ones in OnnxConversionTests, where full pipelines are exported to onnx and applied to the same input, this is not much a problem. But in scenarios like the one in here, where a preexisting onnx model is applied as a featurizer, it might become a problem if the user wants to use again the "data_0" column.
Furthermore, this will be unexpected behavior, because the applied onnx model wouldn't have come from exporting a ColumnSelectingTransformer, but rather it might come from any other source. So dropping columns wouldn't be entirely right.
3 possible solutions I can think for this secnario:
- The user should somehow know (perhaps through docs) that the OnnxTransformer will drop the input column, so if they want to use it they should copy the column before applying the model.
- We could add Options to the ApplyOnnxModel method to let the user decide if they want to drop all the input columns or not, or even let them choose which one of them to drop. Then the GetDropColumnsNames in the OnnxTransformer would return the appropiate columns to drop.
- We could save which columns to drop inside the Onnx model, in some sort of metadata (such as the SlotNames were saved on PR Added slot names support for OnnxTransformer #4857 ) and then let the GetDropColumnsNames get those names from there. There was some opposition from Emad to use onnx nodes to save metadata in this way, though... so perhaps it wouldn't be the best option.
There was a problem hiding this comment.
Another alternative that is occurring to me is that onnxtransformer can hide instead of drop the input columns. Is there a way for users to un-hide columns?
There was a problem hiding this comment.
So this version is better than the previous version of dropping every column except for the ONNX outputs, but I still think that it is introducing wrong behavior, because of the side effects that @antoniovs1029 mentioned above. Dropping the input columns will be wrong in most cases (most ONNX models won't contain DropColumns). In addition, this change might do the wrong thing even when the ONNX model does contain a DropColumns node, since there is no guarantee that the columns being dropped are the input columns, is there?
I still think that this change is solving a non-issue, and should be avoided. The expected behavior of the OnnxTransformer is to compute the column that the user requests, and it is not expected to change the rest of the data view schema.
If the only reason this issue has come up is that in the tests of the ONNX export of ColumnSelectingTransformer it seems as though the ONNX model is not doing what it is intended to do, I suggest testing this in a different way. One possibility, is to create an OnnxTransformer asking for one of the columns that have been dropped, and verifying that we get an error saying something like "there is no such column in the ONNX model" (this is probably not the syntax of the error message, but you get the idea). Again, I really think the change needs to be in the test, and not in the code of OnnxTransformer.
In reply to: 406110390 [](ancestors = 406110390)
There was a problem hiding this comment.
In addition, this change might do the wrong thing even when the ONNX model does contain a DropColumns node, since there is no guarantee that the columns being dropped are the input columns, is there?
I am not sure I'm following this point,. "DropColumns nodes" don't exist in ONNX. since ONNX doesn't even have a notion of "column". The way the ColumnSelectingTransformer ONNX export works is that if a column was to be dropped, then the final ONNX model won't contain the output onnx node that would cause the column to propagate (this will happen whether the column is in the input of the pipeline being exported, or if the column is created inside the pipeline). The PR here will drop input columns that are dropped or hidden by the pipeline being exported, and it will also drop columns that were created inside the pipeline and also dropped or hidden inside the pipeline (this is demonstrated by cases 1a, 1b, and 3 of the ColumnSelectingOnnxTestColumnPropagation test I added, although I missed including the case of dropping columns that were created and hidden in the pipeline being exported).
So there's no guarantee that a SelectingColumnTransformer will drop columns from the input, since it could be dropping columns created inside the pipeline being exported (it's either one or the other), and in the latter case the input column would have been hidden anyway if there's another column with the same name which shall be dropped. So, I think that the PR here works in the sense that it always drops columns that an exported SelectingColumnsTransformer will drop (although it also drops other hidden columns). On the other hand, the side effect I pointed to here is still a problem.
I still think that this change is solving a non-issue, and should be avoided. The expected behavior of the OnnxTransformer is to compute the column that the user requests, and it is not expected to change the rest of the data view schema.
I've discussed this with @harishsk , and I also agree that this issue shouldn't be fixed. But he thinks otherwise, because if the SelectingColumnTransformer drops some columns, the onnx exported model should also drop all of them... so I agree this is a bug, but it's probably not one that should be fixed. Particularly, I think he wants this issue to be fixed, since it might cause performance issues in NimbusML, where when a user wants the outputs of a transforms pipeline, he receives the output of all unhidden columns... if the user has a pipeline that includes ColumnSelector to drop undesired columns, and then exports it to onnx, then it's the case that the exported version won't drop the input columns being propagated (which in some causes could be thousands of columns), and then when applying the onnx model, it takes a lot of time to retrieve the values of all the columns in the input.
Still, there's a workaround: let the user add a ColumnSelectingTransformer after applying the onnx model, and the problem is solved... I found that this works in both ML.NET and NimbusML. @harishsk is hesitant about this, since his opinion is that this should be done automatically since the user might not know which columns to drop. But unless we save some sort of metadata in the onnx model, to let ML.NET know when to drop columns from it, I don't see how this can be done without the side effect I pointed to here.
In reply to: 406669100 [](ancestors = 406669100,406110390)
There was a problem hiding this comment.
After discussing this offline with @yaeldekel and @harishsk , we've come to the conclussion that this side effect is not acceptable, and we should let users apply pretrained onnx models without dropping the input column.
So to fully fix this issue, there should be a way (perhaps metadata) to annotate in the onnx model which columns to drop, and then let GetDropColumnsNames to get those names from there.
In reply to: 406839974 [](ancestors = 406839974,406669100,406110390)
| for (int i = 0; i < input.Count; i++) | ||
| { | ||
| if (InputSchema[i].IsHidden || dropColumnsNames.Contains(InputSchema[i].Name)) // MYTODO: Should I drop all hidden columns? Only the ones that are inside the dropColumnsNames list? | ||
| continue; |
There was a problem hiding this comment.
The ColumnSelectingTransformer has a couple of options such as if it should drop Hidden columns or not, and if it should do it only with the names it's dropping or not. These options are lost when exporting the model to onnx, so there's no way to know what should we do with Hidden columns when applying the onnx model.
So I simply decided to drop all the Hidden columns (regardless if its name was in the input or output of the onnx model). I think it's better for the user, since it might be even more confusing to see some non-hidden columns being dropped, while the hidden ones remain.
| // by the model) should be propagated as if being untouched. | ||
| // Case 6. An input column that doesn't have a corresponding input node in the onnx model, but that has the same name as an output | ||
| // column created by the onnx transformer, should be hidden. | ||
| // |
There was a problem hiding this comment.
I need to add case 7: a column that was created and hidden inside the pipeline being exported. It will be dropped.
Also perhaps consider adding to the test the case where and input column that will be dropped gets copied into another column of another name. This will technically be case 4, but it might still be good to include it.
|
In offline discussions with @yaeldekel and @harishsk it's been decided not to fix this issue, since @yaeldekel considers this is actually not an issue, and in general it's not clear if we should let onnx transformer to drop columns, since it goes against the design of most transformers where the users expect transformers to output new columns but not remove old ones. The contents of this PR will be pushed to a new branch inside the main ML.NET repository, in case this become an issue in the future. The new branch is: |
ghost
locked as resolved and limited conversation to collaborators
Fixes #4970
After discussing this issue with @harishsk we agreed that the way to go is to drop all the input columns that are used as inputs of the onnx model that is applied by an
OnnxTransformer. I've added theColumnSelectingOnnxTestColumnPropagationwhere I explain and show more clearly what effects this has on different scenarios.Since the
OnnxTransformeris aRowToRowTransformerBase, it can't drop columns only add columns. In fact, it seems that there's no transformer that do both dropping and adding columns.To solve this, changes needed to be made in
RowToRowTransformerBaseto letOnnxTransformeroverride some methods, so that it can have more control on the output schema. This way theOnnxTransformernow creates aOnnxDataTransformwhich in turn uses the sameOnnxTransformer.Mapperthat always existed, but now it also uses aOnnxTransformer.Bindingsthat enable dropping columns while also adding the new columns added by the mapper. This is different from the previous behavior, where theRowToRowTransformerBasewould return aRowToRowMapperTransformwhich used the typicalColumnBindingswhich only add columns to the input but don't drop columns from it.NOTE: I have left several "//MYTODO" comments with thoughts and questions for myself, which shall be removed before merging this PR. I've also added some comments here on GitHub pointing to the thoughts and questions that I find more important to address first.