Support scripting classmethod called with object instances #49967
Conversation
facebook-github-bot
added
cla signed
oncall: jit
💊 CI failures summary and remediationsAs of commit 31db055 (more details on the Dr. CI page):
ci.pytorch.org: 1 failedThis comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. This comment has been revised 3 times. |
Codecov Report
@@ Coverage Diff @@
## master #49967 +/- ##
==========================================
+ Coverage 75.14% 80.69% +5.54%
==========================================
Files 1891 1895 +4
Lines 205325 205461 +136
==========================================
+ Hits 154291 165794 +11503
+ Misses 51034 39667 -11367 |
ppwwyyxx
requested review from
SplitInfinity and
eellison
and removed request for
SplitInfinity
| @@ -217,7 +222,7 @@ def remove_prefix(text, prefix): | |||
| return aligned_prefix + aligned_suffix | |||
|
|
|||
|
|
|||
| def get_jit_def(fn, def_name, self_name=None): | |||
| def get_jit_def(fn, def_name, self_name=None, is_classmethod=False): | |||
There was a problem hiding this comment.
Is it possible to determine if fn is a classmethod without passing an extra parameter? I think it is:
>>> class A:
... @classmethod
... def a(cls):
... pass
... def b(self):
... pass
... @staticmethod
... def c(s, t):
... pass
...
>>> A.a.__self__
<class '__main__.A'>
>>> A.b.__self__
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'function' object has no attribute '__self__'
>>> A.c.__self__
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'function' object has no attribute '__self__'
So it seems that __self__ is only present on classmethods? So I think we can do that check in get_jit_def directly without the extra parameter.
There was a problem hiding this comment.
Regular methods that are bounded to instances, e.g. A().b in your example will have __self__. Could get_jit_def be called with such methods?
There was a problem hiding this comment.
From my knowledge, no. Can you try it to see if it's possible (to get rid of the extra function argument)?
There was a problem hiding this comment.
This fails many tests. I added print(fn) when __self__ is present, and saw cases it is called with bound regular methods:
$python3 test/test_jit.py
Fail to import hypothesis in common_utils, tests are not derandomized
.................................<bound method BasicModule.forward of BasicModule()>
E<bound method BasicModule.forward of BasicModule()>
E<bound method BasicModule.forward of BasicModule()>
E./private/home/yuxinwu/DL/pytorch/test/jit/test_builtins.py:127: DeprecationWarning: Please use assertEqual instead.
self.assertEquals(py_out, jit_out)
.<bound method TestBuiltins.test_has_attr.<locals>.Mod.forward of Mod(
(mods): ModuleList(
(0): HasA()
(1): HasB()
)
)>
<bound method ModuleList.forward of ModuleList(
(0): HasA()
(1): HasB()
)>
E<bound method TestBuiltins.test_has_attr_invalid_args.<locals>.Mod.forward of Mod(
(mod): Linear(in_features=1, out_features=1, bias=True)
)>
It seems that's because scripted modules are always created from instance, not class - so regular methods are bound.
There was a problem hiding this comment.
@ppwwyyxx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: require pytorch/pytorch#49967 Reviewed By: theschnitz Differential Revision: D25852122 fbshipit-source-id: eb2da41f28861fd8ac4f348283bb072abfe8fedd
Currentlt classmethods are compiled the same way as methods - the first argument is self.
Adding a fake statement to assign the first argument to the class.
This is kind of hacky, but that's all it takes.