[jit] introduce module interface declaration #28408

wanchaol · 2019-10-22T04:46:04Z

Stack from ghstack:

[jit] submodule swapping via module interface #28409 [jit] submodule swapping via module interface
[jit] introduce module interface declaration #28408 [jit] introduce module interface declaration
[jit] guard against inheritance on torchscript classes #28407 [jit] guard against inheritance on torchscript classes

Summary:
This enable interface to defined on a nn.Module, and the InterfaceType
now have a field of is_module_ to distinguish if it's a module interface
or a normal interface (This is similar to what ClassType distinguish on
module and torchscript classes).

The module interface can be assigned with any ScriptModule that has the
compatible signatures on schemas. A normal object that is not a
ScriptModule will not be able to assigned to an module interface and
will error out when user explicitly doing so. Assigning a ScriptModule
to class interface will make it only available in attribute_list, not
module_list. More details on subtyping relationship documented in the
jit_type.h

If you declare an module interface inside an nn.Module that is being
compiled to a ScriptModule, behavior to our internal compilation will
be:

ConcreteModuleType will record it as an module attribute and add to
the attributes_ list.
JitType that is created from the ConcreteModuleType will record it as
an attribute and pre-genenerate the slot. The slot will be marked as
EntityType::MODULE still to make sure JitType record it as a Module
slot
cpp_module will also register it as a Module as the Slot type is the
source of truth

Since JitType will record it as attribute as store its type, it will
behave normally as the class interface attribute behave now. This means
the submodule assigned to this module interface is not getting inlined
into the graph as the normal Module::attr behave, it will generate
interface callMethod and allow us to later swap this with another
ScriptModule that implicitly implements this module interface.

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D18284311

Summary: This enable interface to defined on a nn.Module, and the InterfaceType now have a field of is_module_ to distinguish if it's a module interface or a normal interface (This is similar to what ClassType distinguish on module and torchscript classes). The module interface can be assigned with any ScriptModule that has the compatible signatures on schemas. A normal object that is not a ScriptModule will not be able to assigned to an module interface and will error out when user explicitly doing so. Assigning a ScriptModule to class interface will make it only available in attribute_list, not module_list. More details on subtyping relationship documented in the jit_type.h If you declare an module interface inside an nn.Module that is being compiled to a ScriptModule, behavior to our internal compilation will be: 1. ConcreteModuleType will record it as an module attribute and add to the attributes_ list. 2. JitType that is created from the ConcreteModuleType will record it as an attribute and pre-genenerate the slot. The slot will be marked as EntityType::MODULE still to make sure JitType record it as a Module slot 3. cpp_module will also register it as a Module as the Slot type is the source of truth Since JitType will record it as attribute as store its type, it will behave normally as the class interface attribute behave now. This means the submodule assigned to this module interface is not getting inlined into the graph as the normal `Module::attr` behave, it will generate interface callMethod and allow us to later swap this with another ScriptModule that implicitly implements this module interface. Test Plan: Reviewers: Subscribers: Tasks: Tags:

Summary: This enable interface to defined on a nn.Module, and the InterfaceType now have a field of is_module_ to distinguish if it's a module interface or a normal interface (This is similar to what ClassType distinguish on module and torchscript classes). The module interface can be assigned with any ScriptModule that has the compatible signatures on schemas. A normal object that is not a ScriptModule will not be able to assigned to an module interface and will error out when user explicitly doing so. Assigning a ScriptModule to class interface will make it only available in attribute_list, not module_list. More details on subtyping relationship documented in the jit_type.h If you declare an module interface inside an nn.Module that is being compiled to a ScriptModule, behavior to our internal compilation will be: 1. ConcreteModuleType will record it as an module attribute and add to the attributes_ list. 2. JitType that is created from the ConcreteModuleType will record it as an attribute and pre-genenerate the slot. The slot will be marked as EntityType::MODULE still to make sure JitType record it as a Module slot 3. cpp_module will also register it as a Module as the Slot type is the source of truth Since JitType will record it as attribute as store its type, it will behave normally as the class interface attribute behave now. This means the submodule assigned to this module interface is not getting inlined into the graph as the normal `Module::attr` behave, it will generate interface callMethod and allow us to later swap this with another ScriptModule that implicitly implements this module interface. Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

zdevito

Changes look good. I have minor comments. However, this is missing tests for serialization and deserialization preserving the is_module nature of an interface. Since there are no changes to serialization, I believe that information is getting lost during serialization.

zdevito · 2019-10-23T21:28:56Z

aten/src/ATen/core/type.cpp

+    if (!is_module() && iface->is_module()) {
+      if (why_not) {
+        *why_not << "Interface '" << python_str() << "' is not a subtype of "
+                  << "the module interface '" << rhs->python_str() << ".\n";


missing end quote '

zdevito · 2019-10-23T21:32:41Z

aten/src/ATen/core/jit_type.h


 // Interfaces are a list of abstract methods that a class might meet.
 // If a class provides those methods, it implicitly meets the interface.
+// Subtype relations for Interface/ClassType


I find this comment more confusing to understand than the actual code implementation. It would be better to explain that a class or interface lhs is a subtype of rhs if:

lhs methods are a superset of the methods of rhs

if rhs is a module interface, then lhs must be a module interface or module itself.

zdevito · 2019-10-23T21:33:43Z

torch/jit/__init__.py

    rcb = _jit_internal.createResolutionCallbackFromFrame(1)
-    torch._C._jit_script_interface_compile(qualified_name, ast, rcb)
-    obj.__torch_script_interface__ = True
+    if issubclass(obj, torch.nn.Module):


Unnecessary copypasta:
is_module_interface = issubclass(obj, torch.nn.Module)

zdevito · 2019-10-23T21:34:26Z

torch/jit/frontend.py

+    # TODO: proper overriding analysis when implementing class inheritance
    methods = inspect.getmembers(
-        cls, predicate=lambda m: inspect.ismethod(m) or inspect.isfunction(m))
+        cls, predicate=lambda m: (inspect.ismethod(m) or inspect.isfunction(m)) and m.__name__ in cls.__dict__)


Why did this change?

This is to make sure that we are only compiling the user defined functions in the module interface declaration, so that none of the methods in nn.Module is trying to be compiled.

This change doesn't seem fully general to me. For example, for regular class B which inherits from class A, shouldn't A's methods be part of the interface?

Yeah I agree, this should be done more generally when we have inheritance on class or interface. Given that we don't have them (partially have but not complete, like missing super and classtype inheritance), I explicitly disable the class inheritance in the PR below. Maybe it's valid to disable the inheritance for interface as well? (interface can only inherit from object or nn.Module)

zdevito · 2019-10-23T21:37:28Z

test/test_jit_py3.py

+        with self.assertRaisesRegex(RuntimeError, "is not compatible with interface"):
+            as_module_interface(scripted_wrong_mod)
+
+


We need a test that the is_module nature of an interface lasts through serialization.

This has been done in test_interface.cpp

Summary: This enable interface to defined on a nn.Module, and the InterfaceType now have a field of is_module_ to distinguish if it's a module interface or a normal interface (This is similar to what ClassType distinguish on module and torchscript classes). The module interface can be assigned with any ScriptModule that has the compatible signatures on schemas. A normal object that is not a ScriptModule will not be able to assigned to an module interface and will error out when user explicitly doing so. Assigning a ScriptModule to class interface will make it only available in attribute_list, not module_list. More details on subtyping relationship documented in the jit_type.h If you declare an module interface inside an nn.Module that is being compiled to a ScriptModule, behavior to our internal compilation will be: 1. ConcreteModuleType will record it as an module attribute and add to the attributes_ list. 2. JitType that is created from the ConcreteModuleType will record it as an attribute and pre-genenerate the slot. The slot will be marked as EntityType::MODULE still to make sure JitType record it as a Module slot 3. cpp_module will also register it as a Module as the Slot type is the source of truth Since JitType will record it as attribute as store its type, it will behave normally as the class interface attribute behave now. This means the submodule assigned to this module interface is not getting inlined into the graph as the normal `Module::attr` behave, it will generate interface callMethod and allow us to later swap this with another ScriptModule that implicitly implements this module interface. Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

suo · 2019-10-30T21:46:44Z

torch/jit/__init__.py

+    # the user provided methods as part of the interface
+    is_module_interface = issubclass(obj, torch.nn.Module)
+    ast = get_jit_class_def(obj, obj.__name__)
+    torch._C._jit_script_interface_compile(qualified_name, ast, rcb, is_module_interface)


Do we have to do a special case for modules here? The ClassDef treeview can have a base class, which is a more natural place to put that info

we actually just pass a flag into _jit_script_interface_compile and it will just create an InterfaceType with is_module inside.

I mean: instead of a flag, can we put the module information in the ClassDef itself? That avoids the need for an extra flag

Agreed that ClassDef be the source of truth is better. I did some investigation on this approach, I am surprised that our python frontend now actually does not parse the superclass at all, it will leave the Maybe<Expr> as just a Maybe with no Expr inside. But string frontend does have it since it needs to support deserialization. Getting python frontend to parse base class should not be too hard but it's a fair amount of changes that is not related to this PR (we need to inspect the mro() and get all base classes someway). I will make a separate PR to support this.

suo · 2019-10-30T21:52:28Z

torch/csrc/jit/script/class_type.cpp

  if (auto iface = rhs->cast<InterfaceType>()) {
+    // ClassType is not a subtype of InterfaceType if the InterfaceType is a
+    // Module Interface Type but the Class Type is not a Module Class Type
+    if(!is_module() && iface->is_module()) {


I think we should restrict this check even more, to is_module() != iface->is_module().

Is there a valid reason to assign a module to a class-attribute interface? Things like parameters(), etc. will not work the way that people expect so I feel like it's probably a bad idea to allow it.

From python syntax wise, I think it's a valid pattern. A nn.Module as a normal 'class', if it implement all the methods of class-attribute interface, it should implement that interface. This is a valid concern, maybe we should consider throw better error message when we setattr on our script::Module?

In Python, doing mymod.foo = SomeModule automatically registers a submodule, so you can't normally get into a situation where an nn.Module behaves as a normal class attribute. So I think we should restrict this to avoid confusion.

I don't think the check should be in setattr—setattr shouldn't do too much special stuff. We should locate the check in this part of the code, since it's responsible for deciding "does object X fulfill interface Y".

I actually disagree here. A module is a more specific type of a class. If it has the appropriate methods to match a non-Module interface, then it fulfills the contract of that interface and should be a subtype of that interface. Let's say someone has an interface with a str method on it. If the module defines str then it should be able to be passed as a member of that interface even if it is not itself an interface. The reverse is not true, only module instances can be members of a module interface.

suo · 2019-10-30T22:09:16Z

aten/src/ATen/core/type.cpp

 }

+bool Type::is_module() const {
+  if (auto cls_type = this->cast<ClassType>()) {


ClassType and InterfaceType should override is_module(), let's not do a cast here.

oops yeah they do overwrite it, this cast is not needed, I will remove.

suo · 2019-10-30T22:10:16Z

torch/jit/frontend.py

+    # TODO: proper overriding analysis when implementing class inheritance
    methods = inspect.getmembers(
-        cls, predicate=lambda m: inspect.ismethod(m) or inspect.isfunction(m))
+        cls, predicate=lambda m: (inspect.ismethod(m) or inspect.isfunction(m)) and m.__name__ in cls.__dict__)


This change doesn't seem fully general to me. For example, for regular class B which inherits from class A, shouldn't A's methods be part of the interface?

Summary: This enable interface to defined on a nn.Module, and the InterfaceType now have a field of is_module_ to distinguish if it's a module interface or a normal interface (This is similar to what ClassType distinguish on module and torchscript classes). The module interface can be assigned with any ScriptModule that has the compatible signatures on schemas. A normal object that is not a ScriptModule will not be able to assigned to an module interface and will error out when user explicitly doing so. Assigning a ScriptModule to class interface will make it only available in attribute_list, not module_list. More details on subtyping relationship documented in the jit_type.h If you declare an module interface inside an nn.Module that is being compiled to a ScriptModule, behavior to our internal compilation will be: 1. ConcreteModuleType will record it as an module attribute and add to the attributes_ list. 2. JitType that is created from the ConcreteModuleType will record it as an attribute and pre-genenerate the slot. The slot will be marked as EntityType::MODULE still to make sure JitType record it as a Module slot 3. cpp_module will also register it as a Module as the Slot type is the source of truth Since JitType will record it as attribute as store its type, it will behave normally as the class interface attribute behave now. This means the submodule assigned to this module interface is not getting inlined into the graph as the normal `Module::attr` behave, it will generate interface callMethod and allow us to later swap this with another ScriptModule that implicitly implements this module interface. Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

zdevito

This looks good once @suo's comments are addressed, thanks!

Summary: This enable interface to defined on a nn.Module, and the InterfaceType now have a field of is_module_ to distinguish if it's a module interface or a normal interface (This is similar to what ClassType distinguish on module and torchscript classes). The module interface can be assigned with any ScriptModule that has the compatible signatures on schemas. A normal object that is not a ScriptModule will not be able to assigned to an module interface and will error out when user explicitly doing so. Assigning a ScriptModule to class interface will make it only available in attribute_list, not module_list. More details on subtyping relationship documented in the jit_type.h If you declare an module interface inside an nn.Module that is being compiled to a ScriptModule, behavior to our internal compilation will be: 1. ConcreteModuleType will record it as an module attribute and add to the attributes_ list. 2. JitType that is created from the ConcreteModuleType will record it as an attribute and pre-genenerate the slot. The slot will be marked as EntityType::MODULE still to make sure JitType record it as a Module slot 3. cpp_module will also register it as a Module as the Slot type is the source of truth Since JitType will record it as attribute as store its type, it will behave normally as the class interface attribute behave now. This means the submodule assigned to this module interface is not getting inlined into the graph as the normal `Module::attr` behave, it will generate interface callMethod and allow us to later swap this with another ScriptModule that implicitly implements this module interface. Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: This enable interface to defined on a nn.Module, and the InterfaceType now have a field of is_module_ to distinguish if it's a module interface or a normal interface (This is similar to what ClassType distinguish on module and torchscript classes). The module interface can be assigned with any ScriptModule that has the compatible signatures on schemas. A normal object that is not a ScriptModule will not be able to assigned to an module interface and will error out when user explicitly doing so. Assigning a ScriptModule to class interface will make it only available in attribute_list, not module_list. More details on subtyping relationship documented in the jit_type.h If you declare an module interface inside an nn.Module that is being compiled to a ScriptModule, behavior to our internal compilation will be: 1. ConcreteModuleType will record it as an module attribute and add to the attributes_ list. 2. JitType that is created from the ConcreteModuleType will record it as an attribute and pre-genenerate the slot. The slot will be marked as EntityType::MODULE still to make sure JitType record it as a Module slot 3. cpp_module will also register it as a Module as the Slot type is the source of truth Since JitType will record it as attribute as store its type, it will behave normally as the class interface attribute behave now. This means the submodule assigned to this module interface is not getting inlined into the graph as the normal `Module::attr` behave, it will generate interface callMethod and allow us to later swap this with another ScriptModule that implicitly implements this module interface. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D18284311](https://our.internmc.facebook.com/intern/diff/D18284311) [ghstack-poisoned]

Summary: Pull Request resolved: pytorch/pytorch#28408 This enable interface to defined on a nn.Module, and the InterfaceType now have a field of is_module_ to distinguish if it's a module interface or a normal interface (This is similar to what ClassType distinguish on module and torchscript classes). The module interface can be assigned with any ScriptModule that has the compatible signatures on schemas. A normal object that is not a ScriptModule will not be able to assigned to an module interface and will error out when user explicitly doing so. Assigning a ScriptModule to class interface will make it only available in attribute_list, not module_list. More details on subtyping relationship documented in the jit_type.h If you declare an module interface inside an nn.Module that is being compiled to a ScriptModule, behavior to our internal compilation will be: 1. ConcreteModuleType will record it as an module attribute and add to the attributes_ list. 2. JitType that is created from the ConcreteModuleType will record it as an attribute and pre-genenerate the slot. The slot will be marked as EntityType::MODULE still to make sure JitType record it as a Module slot 3. cpp_module will also register it as a Module as the Slot type is the source of truth Since JitType will record it as attribute as store its type, it will behave normally as the class interface attribute behave now. This means the submodule assigned to this module interface is not getting inlined into the graph as the normal `Module::attr` behave, it will generate interface callMethod and allow us to later swap this with another ScriptModule that implicitly implements this module interface. Test Plan: Imported from OSS Differential Revision: D18284311 fbshipit-source-id: e0b8f6e8c34b2087fab337a969e5ea3fb37ec209

facebook-github-bot · 2019-11-03T02:34:31Z

This pull request has been merged in e95dc98.

wanchaol requested a review from apaszke as a code owner October 22, 2019 04:46

This was referenced Oct 22, 2019

[jit] guard against inheritance on torchscript classes #28407

Closed

[jit] submodule swapping via module interface #28409

Closed

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Oct 22, 2019

wanchaol requested review from suo and zdevito October 22, 2019 20:13

wanchaol added 2 commits October 22, 2019 17:55

zdevito reviewed Oct 23, 2019

View reviewed changes

wanchaol added 3 commits October 25, 2019 14:06

wanchaol requested a review from zdevito October 29, 2019 00:51

suo reviewed Oct 30, 2019

View reviewed changes

zdevito approved these changes Oct 31, 2019

View reviewed changes

wanchaol added 3 commits October 31, 2019 17:19

facebook-github-bot closed this in e95dc98 Nov 2, 2019

facebook-github-bot added the merged label Nov 3, 2019

Rikorose mentioned this pull request Nov 4, 2019

[jit] RuntimeError: Expected ty->isSubtypeOf(atype) to be true, but got false. #29122

Closed

facebook-github-bot deleted the gh/wanchaol/59/head branch November 6, 2019 15:16

masahi mentioned this pull request Jan 14, 2020

[Relay] Add a PyTorch to Relay Parser apache/tvm#4497

Merged

mruberry added the Merged label Oct 28, 2020

		with self.assertRaisesRegex(RuntimeError, "is not compatible with interface"):
		as_module_interface(scripted_wrong_mod)

[jit] introduce module interface declaration #28408

[jit] introduce module interface declaration #28408

Uh oh!

Conversation

wanchaol commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wanchaol Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zdevito Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wanchaol commented Oct 22, 2019 •

edited

Loading

wanchaol Oct 30, 2019 •

edited

Loading

zdevito Oct 31, 2019 •

edited

Loading