Skip to content

Conversation

@qbx2
Copy link
Contributor

@qbx2 qbx2 commented Apr 18, 2019

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Apr 18, 2019
Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

@qbx2 qbx2 Apr 18, 2019

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.

Copy link
Contributor Author

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?

@qbx2 qbx2 force-pushed the fix-scriptmoduleproxy-isinstance branch 5 times, most recently from ac193cd to ece7c3d Compare April 18, 2019 15:11
@qbx2
Copy link
Contributor Author

qbx2 commented Apr 18, 2019

https://circleci.com/gh/pytorch/pytorch/1361945?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Apr 18 14:53:46 self = <module 'torch.ops._original_cls' (built-in)>, op_name = '__bases__'
Apr 18 14:53:46 
Apr 18 14:53:46     def __getattr__(self, op_name):
Apr 18 14:53:46         # Get the op `my_namespace::my_op` if available. This will also check
Apr 18 14:53:46         # for overloads and raise an exception if there are more than one.
Apr 18 14:53:46         qualified_op_name = '{}::{}'.format(self.name, op_name)
Apr 18 14:53:46 >       op = torch._C._jit_get_operation(qualified_op_name)
Apr 18 14:53:46 E       RuntimeError: No such operator _original_cls::__bases__
Apr 18 14:53:46 
Apr 18 14:53:46 ../.local/lib/python2.7/site-packages/torch/_ops.py:60: RuntimeError

I think _jit_get_operation() should raise AttributeError instead of RuntimeError... I added if type(instance) is not ModuleMeta as a workaround.

@qbx2 qbx2 force-pushed the fix-scriptmoduleproxy-isinstance branch 2 times, most recently from dc212cd to 77c39ba Compare April 18, 2019 20:01
@qbx2 qbx2 force-pushed the fix-scriptmoduleproxy-isinstance branch from 77c39ba to 77902d8 Compare April 18, 2019 20:35
@qbx2 qbx2 marked this pull request as ready for review April 19, 2019 07:07
@pytorchbot pytorchbot added the module: nn Related to torch.nn label May 22, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@ailzhang
Copy link
Contributor

This was fixed in #20190 by @driazati in a slightly different way. I will close this PR for now. Thanks a lot for contributing!

@ailzhang ailzhang closed this May 23, 2019
@qbx2
Copy link
Contributor Author

qbx2 commented May 24, 2019

Nice fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: nn Related to torch.nn oncall: jit Add this issue/PR to JIT oncall triage queue open source

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[JIT] isinstance(m, nn.Linear) returns False in ScriptModules

6 participants