-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Fix isinstance() for WeakScriptModuleProxy #19403
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
Conversation
torch/nn/modules/module.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to use getattr(), but got NotImplementedError for some modules. Also, tried to use isinstance(instance, WeakScriptModuleProxy). Couldn't import it due to a recursive import. Any idea for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
torch.jit depends on torch.nn since torch.jit.ScriptModule is a subclass of torch.nn.Module. I think if we want to support isinstance it has to be done only on WeakScriptModuleProxy (without making any changes to the inheritance chain of nn.Module). Maybe a less intrusive solution is to subclass ScriptMeta, add the __instancecheck__ method, and somehow install that on only the WeakScriptModuleProxy class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but isinstance() does not work as that way. When we do isinstance(module, nn.Linear), python does: type(nn.Linear).__instancecheck__(nn.Linear, module). That's why I added ModuleMeta to cover every module's __instancecheck__. We don't have an option implementing __instancecheck__ on ScriptMeta, refer to https://stackoverflow.com/questions/52168971/instancecheck-overwrite-shows-no-effect-what-am-i-doing-wrong for more information.
If we're not going this way, there's a method providing torch.isinstance() as I suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or... can we inherit from both class: SomeProxy(WeakScriptModuleProxy, original_cls), when creating WeakScriptModuleProxy instance?
ac193cd to
ece7c3d
Compare
I think |
dc212cd to
77c39ba
Compare
77c39ba to
77902d8
Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Nice fix! |
Close #19348
cc: @driazati @ailzhang