Modified how DataViewTypes are registered - #4187
Conversation
The DataViewManager registers DataViewTypes to determine how to represent the the data within ML.Net. However, there was a bug in how types were being queried. Types that were being registered via an Attribute were being added to the manager with only that Attribute. However, when queried from the manager all custom Attributes were being passed. To solve this we need to only pass custom Attributes that are relevant to the Manager. All Type attributes now inherit from a single TypeAttribute class such that we can filter custom attributes for relevant types.
|
|
|
We should use the scenario from #4121 to test whether this change fixes the problem. Use an |
Since the DataViewManager works only on DataViewTypeAttributes, we do not need to subclass attributes, but only the filter on the DataViewTypeAttribute instead.
There are things that can go wrong when doing that.
In a case like this, capturing the original scenario in a test case to ensure it is fixed, and prevent future regressions, is ideal. #Resolved |
While the unit test added previously addressed the underlying issue, this test case proves that the issue in Bug #4121 is now completely resolved, and there should be no further issues.
Did not want to polute the repo with large models so was able to find an existing model which had an appropriate output such that we could still verify the same behavior.
Since the the register function is part of the public API we cannot remove/change it. However, I have marked that form of the call to be obsolete, and instead have created a new register API which takes in only a singular DataViewTypeAttribute. This will allow us to keep existing functionality while still providing guidance on how the API should actually be used. Once we increment the major version of the API we can then remove the obsoleted version of register.
While working locally, the test were not working on the CI job. this appeared to be due to issues with path for the model, as well as not using OnnxFact.
|
@wschin - can you comment on the changes? What were the scenarios necessary for having an Enumerable of attributes? Looking at #3263 (the original PR) I assume it has to do with you can have multiple ImageTypeAttributes with different height and width values. @bpstark, Can you explain why you made that change to the API? #Resolved |
|
@eerhardt According to the usage the only types which are ever passed to the DataViewManager register function are subclassed from DataViewTypeAttribute. according to the schema doc which @yaeldekel referenced we are only allowed to ever have at most 1 DataViewTypeAttribute associated with a given member var. As such we can never have more than 1 attribute passed to the register function. #Resolved |
|
Sounds fine to me. Please make sure @wschin signs off on this change, since he has the full background. #Resolved |
|
@wschin could you please take a look at my change. #Resolved |
ghost
locked as resolved and limited conversation to collaborators
6 participants
The DataViewManager registers DataViewTypes to determine how to
represent the the data within ML.Net. However, there was a bug in how
types were being queried. Types that were being registered via an
Attribute were being added to the manager with only that Attribute.
However, when queried from the manager all custom Attributes were being
passed.
To solve this we need to only pass custom Attributes that are relevant
to the Manager. All Type attributes now inherit from a single
TypeAttribute class such that we can filter custom attributes for
relevant types.
Fixes #4121