Create methods not being called when loading models from disk #4385
Comments
|
So I've done a couple of experiments regarding this issue and these are my findings. Experiment 1: Swaping the create and constructor order in ComponentCatalog.csWhen registering an assembly, the current implementation in So a simple way to solve this whole issue is to swap the order in which things are done. So in this git branch I modified TryGetIniters so it looks first for a Create method instead of a constructor. Then I ran all the existing tests in the repo and all of them passed. And I also looked into specific cases where the 5 classes I've mentioned in the first post in this issue are loaded from disk, and now they called their Create method as intended. Notice that, although it was unnecesary, I also modified the CreateInstanceCore method to check first for a Create method instead of a Constructor. The reason it's unnecesary to swap it here, is because in TryGetIniters, it first looks for a Create method, and if there's one, then it doesn't look for a constructor, and the constructor is set to be null. Nonetheless, I also modified the [Question 1.1] If there are other places where this pattern is used, please let me know so I can be aware of them. My conclusion is that this simple change seems to solve the problem. But I would still want to ask if there was a reason to first look for constructors, and then create methods, or if this order didn't mean anything. [Question 1.2] Is it okay, then, if I simply solve this problem by swaping the order as I've just described? Experiment 2: Finding how many classes were affected by this issueIn another experiment in this other git branch I modified the TryGetIniters method so that it would try to find both a constructor and a create method, and log the name of the class if it found both. I then ran all the existing tests and I found that 75 different classes had both a constructor and a create method that could be used by Although I haven't manually checked if the issue here described affected all of the 75 classes, I guess it's safe to think that it did, and that the create method of these 75 classes was being ignored. I also haven't looked at each class individually, but after selecting some at random, I could confirm that they had both constructor and create method. The ones I checked all followed the same pattern of having a create method that did some security checks and that then called the private constructor. If requested I can personally look at each of the classes, to see if any of them present particular behaviors in their create methods that don't follow the pattern I've mentioned. In fact, 4 of the 5 classes I mentioned in the first post of this issue, do have some extra code in their create methods that doesn't follow this pattern, but, as described in issue #4381, it might be the case that this code simply needs to be erased because it was not updated properly when releasing the first version of ML.net. Perhaps other classes also have the same problem of having code that needs to be erased or updated in their create method. [Question 2] Should I check individually all of these 75 classes' create methods? For what purpose? At the end of this post I list the 75 classes that I found on this experiment. Experiment 3: Removing the Instance GettersNotice that in the So then I ran another experiment completly removing the code in So my conclusion is that none of the tests actually loads a model from disk that uses an "instance getter" to load one of its classes. And perhaps it might be the case that no class actually implements an "InstanceGetter". [Question 3.1] Why do "instance getters" exist? Are there classes that actually use them when loading? 75 classes thought to be affected by this bug |
|
Hi @antoniovs1029, thanks for the thorough investigation! Regarding the Instance getters, I think it would be safe to remove, it's legacy code, and I'm not even sure that we ever had components that used that (it may have been there 'just in case'). |
See #922 and then subsequently #963. These two changes adversely affected this situation. It used to be the case that only |
|
@yaeldekel Thanks for your reply. I am still interested to know your opinion on Experiment 1. Would you think it's okay to simply swap the order in which create methods and constructors get invoked? About @eerhardt suggestion about the exception, I think what he meant is that I should add an exception to see what classes were being affected by this problem: i.e. that I should add it only for experimental purposes and then remove it. I believe he didn't mean that I should add the exception and leave it to then be merged into the master branch, precisely because it would be a big change. I did try to add such an exception, but it wasn't very informative, because the tests that loaded from disk would crash as soon as they tried to load the first class that had both create and constructor... and so I found the approach of Experiment 2 to give me a more comprehensive and practical list. |
Yes, I understand this is why this have become an issue. I only pointed at the 75 classes I found, just to have a general idea of how many classes were affected by it, since it seems to me that in all of them the create method would have been ignored given the changes made in the PRs you mentioned. Still my question remains if it would be ok to simply change the order in which ComponentCatalog does things, as I did in my Experiment 1. Thanks for pointing directly to those PRs, though. I see it would be useful to have a reference to those PRs in here. |
My assumption would be to match the previous behavior - if both are found, use the one that is
I did intend for the exception check to get merged into the master branch once we have fixed everything. That will ensure we don't get into a similar situation. |
|
Oh, sorry for the misunderstanding. I will follow your suggestions asap, and see what happens. About Experiment 3, on removing the instance getters, do you ( @eerhardt ) have any opinions? Since yael's answer was that:
I am not sure if I should remove them since they seem to be legacy code, or to leave them "just in case". It's my understanding that not all of TLC code has been ported to ML.net, so perhaps in the future they might become needed? |
|
On the instance getters, I'm not sure. No matter what you do, it should be separate from the Create method changes. |
Well, I did that and then I got several exceptions when running the tests. So I then decided to make the following experiment. Experiment 4: Finding the classes whose create and constructor methods share the same visibilityI made another experiment similar to Experiment 2, where I would log whenever a loadable class has both its create and constructor method with the same visibility (i.e. Public vs. Non-Public visibility). The code of the experiment can be found here. The result is that there are 2 classes whose create method and constructor are public, and 52 classes whose create method and constructor are non-public. So further work will need to be done to know which method to call. Unless someone suggests something better, the only way I can see of solving this is to manually go through those 54 classes, and see if they follow the pattern where the create method only does security checks and then calls the constructor... Having a quick look at them, it seems this is the case, and then swapping the order in which ComponentCatalog calls the Create or the creator method might be enough to fix the bug for these classes. If any class diverges from this pattern, then something else should be done in those cases. [Question 4.1] Should I follow this plan of manually looking into these classes to decide if it is better to simply swap the order in which ComponentCatalog does things as I did in Experiment 1? [Question 4.2] So far I've been assuming that Create methods are only used by the CreateInstanceCore method... are there other places (or tools, such as AutoML or ModelBuilder) that use them? If this is the case I would like to know more about how this affect this issue. 2 classes that have both Public create and constructor methods52 classes that have both Non-Public create and constructor methods. |
You have to look at the whole signature of the method, not just the visibility and the name.
I spot checked a few of these, and the ones I did do look like a problem. IMO to fix it, the |
I agree that I should pay attention to the whole signature, so I've done it in the experiment I will post below. But I just want to make sure that we are on the same page about "what ComponentCatalog is looking for". By reading your comments on the SkipTakeFilter and LabelIndicatorTransform classes, I am under the impression that you are assuming that only constructors that have ModelLoadContext (and potentially IHostEnvironment) as a parameter will be used by the ComponentCatalog. I believe this assumption would be wrong, since the TryGetIniters method looks for whatever constructor that has a signature that matches with the parmTypes variable that is passed to the TryGetIniters, if it fails then it tries finding one that matches with the parmTypesWithEnv; and if it fails then it does the same but looking for create methods. But notice that there is no reason why parmTypes needs to include a ModelLoadContext type (although as I will show below, it typically does, but not always); particularly its my understanding that parmTypes is always going to be the CtorTypes property from the LoadableClassAttribute being registered, because that's what is passed to the TryGetIniters method in here. So I am not sure if I am missing your point, and maybe you meant something different. Anyway, I will elaborate on how I think it should work for these classes down below, just to make sure that I am not missing something. Experiment 5: Finding the classes whose create and constructor methods share the same visibility and signature.Experiment 4 already looked for classes whose create and constructor methods shared the same visibility and signature (with an optional IHostEnvironment extra parameter), and it actually only looked at those that could be used according to the LoadableClass attribute that was used when registering the assemblies. So now I reran exactly the same experiment, but now logging the parmTypes that defines the signature that TryGetInitters is trying to find. This way I can have a more precise idea of what changes need to be made. The code for this experiment can be found here. Public classesThe above list tell us that the only create and constructor method that causes a conflict in the LabelIndicatorTransform are the ones with overload
where the argType is LabelIndicatorTransform.Options, the sigType SignatureDataTransform has only one parameter IDataView, and thus the CtorTypes (aka parmTypes) of this attribute is A similar analysis to this other LoadableClassAttribute for the LabelIndicatorClass, tell us that it will try to load a constructor or create method with parmTypes Applying the same analysis to the SkipTakeFilter class shows me that the only conflictive overloads for the constructor / creator methods would be the ones shown in the log. Looking at these 3 cases, it's my impression that they should call the create method instead of the constructor. But I wouldn't be sure. [Question 5.1] Should these 3 cases call its create method? How should I modify the classes to accomplish this? Non-Public classesAs shown in the log at the end of this post, all of the non-public classes that were found problematic on Experiment 4, have the problem specifically only in their create and constructor methods with overload of So following this suggestion by @eerhardt:
[Question 5.2] Should I then simply change the visibility of the Create methods with overload Here's the log: |
|
I have discussed this offline with @yaeldekel , and the conclusion is to follow Eric's suggestions, and that this shouldn't be a problem for other tools such as NimbusML, AutoML, etc... So if a class has both a create and constructor method that matches what the I would also need to change the methods and constructors access modifiers as appropriate, only changing the ones that are affected by this issue. By suggestion of @yaeldekel I changed this in a manner that the create method would be called instead of the constructor. |
… disk (#4485) * Changed ComponentCatalog so it would use the most public "initter" * Changed the visibility of several constructors and create methods so to choose the correct initter when loading models from disk
ghost
locked as resolved and limited conversation to collaborators
There are some classes that define a Create method which is supposed to be called when loading a model from disk, but the problem is that the method is not being called at all.
For example, I've noticed this unexpected behavior in the following classes:
when loading a model that uses any of these classes, their Create method is expected to be called, but, as stated, the method is not being called.
I also noticed this behavior in 3 other classes of the Calibrator.cs file (namely, the
ParameterMixingCalibratedModelParameters,ValueMapperCalibratedModelParameters, andFeatureWeightsCalibratedModelParametersclasses), but I've fixed this problem for those specific classes in my recent PR #4306 (which, as of the moment of writing this, is still waiting to get approved). It was while working on that PR that I noticed this problem on these classes, and I commented about it there... but it is appropriate to open this separate issue to better document this, since it is a problem that affects different classes across different files.In fact, as I will describe below, there's a certain code pattern that is related to this problem, and I've seen this pattern in other classes of the Calibrator.cs file as well. So, the problem I describe here might be affecting even more classes than the ones I've mentioned.
Cause of the problem
The CreateInstanceCore method in the
ComponentCatalog.csfile is responsible to try to call theCreatemethod of any class when loading a model from disk.The current implementation of the method actually checks first if the class has a constructor with parameters
(IHostEnvironment env, ModelLoadContext ctx)and invokes that constructor through reflection. If the class doesn't have such a constructor, then it checks if it has aCreate((IHostEnvironment env, ModelLoadContext ctx)method, and it gets invoked through reflection.This behavior is not desired for the classes I've mentioned (and potentially other classes), since they define both a constructor and a Create method with those parameters, but in these cases it's actually expected that the Create method gets called instead of the constructor. Thus, if a class follows the pattern of having a private or internal constructor (with the
(env, ctx)parameters) and also has a Create method, then this problem might also be affecting that class.Since the Create method typically only runs some security checks before calling the constructor, it turns out that the overall process of loading models doesn't seem affected by this issue. But the problem remains that these security checks are being missed along with whatever behavior the Create method adds to the process.
As explained by @yaeldekel in her comment on my recent PR #4306 (under "Answer 1"), this problem might had been introduced before the official release of ML.net, when the
ComponentCatalogmethod was modified in a way that permitted theCreateInstanceCoremethod to use private and internal constructors, which didn't use to happen... so before those changes were made, classes could have private or internal constructors and a Create method, and the latter would appropriately be called. But now the constructor gets called, and this is the case of all the classes mentioned in this issue.Since these changes were made while trying to internalize as many APIs as possible before the ML.net official release, many constructors where also made private or internal, and thus the changes in ComponentCatalog that permit using those constructors are also necessary.
Because of these, further investigation is needed to know for sure which classes are being affected by this problem, so to better find a way to fix this problem without affecting all of the other classes that doesn't present this situation.
The text was updated successfully, but these errors were encountered: