-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exception occurred when getting the state dictionary(state_dict). #1272
Comments
I suspect it's because of this API that it fails to recognize all the modules. |
Is there a smallish repro case that I can debug? |
In this case, the state_dict of cv1 cannot be obtained.
In this case, the state_dict of cv1 can be obtained. |
So I guess the problem is that |
Sequential and ModuleList have different implementation methods for RegisterComponents. |
I suppose the issue could be simply solved by adding a call to the submodule's However actually in my opinion, all the modules should always call But it seems not... Even using static TorchSharp.torch.nn;
var l = ModuleList(Linear(1, 1));
Console.WriteLine(l.state_dict().Count); // 0 And |
This is the implementation of ModuleList, and I think adding similar code in Sequential should be able to fix it. |
However you will still find that it's unable to use So one solution might be to call the submodule's And let me repeat my suggestion. Perhaps all the modules should always |
We can call RegisterComponents once in the top-level model, so that other models will be registered automatically, which is the most convenient. |
ahh... Even |
I think this could work without side effects... but... humm... I can't say... protected override void RegisterComponents()
{
foreach(var module in this._modules) {
this.register_module("sub", (nn.Module)module);
_internal_submodules.Clear();
}
} |
To facilitate the use of pre-trained weights in TorchSharp, it is advisable to maintain consistency with PyTorch as much as possible. |
That is what I mean. A module should register the parameters by themselves. In PyTorch it is done by In other words, all the modules should be able to use alone, instead of being required to be a part of other modules. In PyTorch Umm... Perhaps the best solution would be a source generator? |
I'm not sure if this is feasible or not, but the idea is to call RegisterComponents within the parameterless constructor of nn.Module. This way, when you create a custom model that inherits from nn.Module, it will automatically register itself. |
Unfortunately, that's impossible. The constructor of nn.Module is runned before the constructor of your model. Thus no values have been assigned to the properties and fields, so we can't register them... |
Switching gears, we could declare properties and then mark them with a custom attribute that has a “name” which will be used as the registration name. Subsequently, we could employ Fody to inject code into the getter and setter methods to handle the registration process. However, this approach is somewhat cumbersome. |
Yes I suppose Fody/SourceGenerator could be a beautiful solution. And we can easily expose properties instead of fields in that way. That's also great. @NiklasGustafsson Could you please take a look at this? |
I have made a simplified demo here: https://github.com/yueyinqiu/TorchSharp.AutoRegister PS: It's still impossible to get rid of the traditional constructors (and use a primary constructor instead), because we have to access the generated property. So sad :( |
That is exactly right. That's why |
That capability already exists. For example, in the rewrite we're working on for some of the standard modules, which will enable more attributes to be exposed, the parameters of
|
Yes, this is exactly the intended protocol, and the documentation says so: The exception from this rule will be modules (such as the |
As much as I dislike relying on reflection, which the current scheme does (I dislike it because it prevents AOT), having to use source code generation adds complexity and something that has to be automated. That would be a last resort, I think. The current scheme works fairly well as long as you follow the instructions very closely and don't do advanced stuff like the |
So, to address the issue of Sequential not registering its sub-modules, for the time being, should we also rewrite Sequential’s RegisterComponents, just like we did with ModuleList? |
So my understanding is that custom modules should not relies on others calling its |
We can certainly reconsider the protocol for module registration for the future. However, if the guidelines for custom modules described in the Wiki article are followed, the current protocol works. |
As unsatisfying as this answer is -- I don't recall. |
haha... Perhaps we should check all the modules provided by TorchSharp, and remove this call if nothing depends on that? I believe this should be done as early as possible, to avoid more projects' relying on it by mistake. |
oh well there is at least one thing ( using static TorchSharp.torch.nn;
var l = ModuleList(Linear(1, 1));
Console.WriteLine(l.state_dict().Count); // 0 I suppose it should be modified to have a similar behavior as |
Now it’s calling RegisterComponents() in the custom module, it just feels a bit verbose, haha. |
In TorchSharp, I defined a model that contains nn.Sequential cv4. However, when I obtained the state_dict of the entire model, the dictionary for cv4 was missing, which is very strange. Other models also have nn.Sequential, and they can all be correctly obtained, but not the last layer.
The text was updated successfully, but these errors were encountered: