[JIT] Add API for ignoring arbitrary module attributes #45262

SplitInfinity · 2020-09-24T06:06:08Z

Stack from ghstack:

[JIT] Add recursive scripting for class type module attributes #45234 [JIT] Add recursive scripting for class type module attributes
[JIT] Add API for ignoring arbitrary module attributes #45262 [JIT] Add API for ignoring arbitrary module attributes

Summary
This commit adds an API for ignoring arbitrary module attributes during
scripting. A class attribute named ignored_attributes containing names
of attributes to ignore can be added to the class of the instance being
scripted. Attributes ignored in this fashion cannot be used in
forward, methods used by forward or by @exported methods. They
are, however, copied to the RecursiveScriptModule wrapper and can be
used by @ignored methods and regular Python code.

Test Plan
This commit adds unit tests to TestScriptPy3 to test this new API.

Differential Revision: D23971882

**Summary** This commit adds an API for ignoring arbitrary module attributes during scripting. A class attribute named `ignored_attributes` containing names of attributes to ignore can be added to the class of the instance being scripted. Attributes ignored in this fashion cannot be used in `forward`, methods used by `forward` or by `@exported` methods. They are, however, copied to the `RecursiveScriptModule` wrapper and can be used by `@ignored` methods and regular Python code. **Test Plan** This commit adds unit tests to `TestScriptPy3` to test this new API. [ghstack-poisoned]

**Summary** This commit adds an API for ignoring arbitrary module attributes during scripting. A class attribute named `ignored_attributes` containing names of attributes to ignore can be added to the class of the instance being scripted. Attributes ignored in this fashion cannot be used in `forward`, methods used by `forward` or by `@exported` methods. They are, however, copied to the `RecursiveScriptModule` wrapper and can be used by `@ignored` methods and regular Python code. **Test Plan** This commit adds unit tests to `TestScriptPy3` to test this new API. ghstack-source-id: b9fcadc0f56ffe932250905f1c31743ef9de9a0c Pull Request resolved: #45262

dr-ci · 2020-09-24T07:31:49Z

💊 CI failures summary and remediations

As of commit ded8d28 (more details on the Dr. CI page):

Commit ded8d28 was recently pushed. Waiting for builds...

This 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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 68 times.

**Summary** This commit adds an API for ignoring arbitrary module attributes during scripting. A class attribute named `ignored_attributes` containing names of attributes to ignore can be added to the class of the instance being scripted. Attributes ignored in this fashion cannot be used in `forward`, methods used by `forward` or by `@exported` methods. They are, however, copied to the `RecursiveScriptModule` wrapper and can be used by `@ignored` methods and regular Python code. **Test Plan** This commit adds unit tests to `TestScriptPy3` to test this new API. [ghstack-poisoned]

**Summary** This commit adds an API for ignoring arbitrary module attributes during scripting. A class attribute named `ignored_attributes` containing names of attributes to ignore can be added to the class of the instance being scripted. Attributes ignored in this fashion cannot be used in `forward`, methods used by `forward` or by `@exported` methods. They are, however, copied to the `RecursiveScriptModule` wrapper and can be used by `@ignored` methods and regular Python code. **Test Plan** This commit adds unit tests to `TestScriptPy3` to test this new API. ghstack-source-id: 7f7e38a6ab760e5f7cf1195dbca468eb80d5d6a4 Pull Request resolved: #45262

eellison

Cool! This is definitely needed as we're adding compilation of a few types of module attributes that we didn't previously (properties, enums, class types).

I guess there's some question about __unused__ vs __ignored__ - I think ignore'd makes since here bc we're attaching it to the model but maybe not.

We could potentially allow attribute access in scripted methods but probably not worth the effort for this PR.

Bikeshedding: wdyt of __jit_ignored__attributes__ ? __constants__ doesnt explicitly mention jit, but I think maybe it'd be clearer if it did.

ping @suo for another person to look at API.

**Summary** This commit adds an API for ignoring arbitrary module attributes during scripting. A class attribute named `ignored_attributes` containing names of attributes to ignore can be added to the class of the instance being scripted. Attributes ignored in this fashion cannot be used in `forward`, methods used by `forward` or by `@exported` methods. They are, however, copied to the `RecursiveScriptModule` wrapper and can be used by `@ignored` methods and regular Python code. **Test Plan** This commit adds unit tests to `TestScriptPy3` to test this new API. [ghstack-poisoned]

**Summary** This commit adds an API for ignoring arbitrary module attributes during scripting. A class attribute named `ignored_attributes` containing names of attributes to ignore can be added to the class of the instance being scripted. Attributes ignored in this fashion cannot be used in `forward`, methods used by `forward` or by `@exported` methods. They are, however, copied to the `RecursiveScriptModule` wrapper and can be used by `@ignored` methods and regular Python code. **Test Plan** This commit adds unit tests to `TestScriptPy3` to test this new API. ghstack-source-id: 62642931355508cc7e76ff6b15e1225e8bfa0494 Pull Request resolved: #45262

SplitInfinity · 2020-09-25T19:20:14Z

I like __jit_ignored_attributes__, but we'd have to change __ignored_properties__ to __jit_ignored_properties__ too. Not saying that's a problem but just something to keep in mind (so that I/we remember to do it).

**Summary** This commit adds an API for ignoring arbitrary module attributes during scripting. A class attribute named `ignored_attributes` containing names of attributes to ignore can be added to the class of the instance being scripted. Attributes ignored in this fashion cannot be used in `forward`, methods used by `forward` or by `@exported` methods. They are, however, copied to the `RecursiveScriptModule` wrapper and can be used by `@ignored` methods and regular Python code. **Test Plan** This commit adds unit tests to `TestScriptPy3` to test this new API. [ghstack-poisoned]

eellison · 2020-09-25T20:43:36Z

We haven’t done a release yet so I think it’s good to get the best api even if we have to switch it once. We can also let the old api still work if we want

**Summary** This commit adds an API for ignoring arbitrary module attributes during scripting. A class attribute named `ignored_attributes` containing names of attributes to ignore can be added to the class of the instance being scripted. Attributes ignored in this fashion cannot be used in `forward`, methods used by `forward` or by `@exported` methods. They are, however, copied to the `RecursiveScriptModule` wrapper and can be used by `@ignored` methods and regular Python code. **Test Plan** This commit adds unit tests to `TestScriptPy3` to test this new API. [ghstack-poisoned]

eellison

Conventionally, unused means that the function can be used in a script'd function and will error if it's being invoked but the model can still be exported. ignore means we retain the python function, it can be invoked in the module, but will error if export'd. Here we're retaining python attributes, but not allowing them to be invoked and still the model can be exported.

It might make more sense for this to be unused instead of ignored, although on the other hand we could just as easily extend __jit_ignored_attributes__ to allow accessing them in scripted functions.

I don't think this should block landing just something to think about.

SplitInfinity · 2020-09-28T20:44:43Z

Conventionally, unused means that the function can be used in a script'd function and will error if it's being invoked but the model can still be exported. ignore means we retain the python function, it can be invoked in the module, but will error if export'd. Here we're retaining python attributes, but not allowing them to be invoked and still the model can be exported.

It might make more sense for this to be unused instead of ignored, although on the other hand we could just as easily extend __jit_ignored_attributes__ to allow accessing them in scripted functions.

I don't think this should block landing just something to think about.

Hm, but the ignored attributes can be used in ignored functions, so I think that means they can also be used in regular Python code too (I can add a test for that).

codecov · 2020-09-29T00:19:49Z

Codecov Report

❗ No coverage uploaded for pull request base (gh/splitinfinity/53/base@6403714). Click here to learn what that means.
The diff coverage is n/a.

@@                     Coverage Diff                     @@
##             gh/splitinfinity/53/base   #45262   +/-   ##
===========================================================
  Coverage                            ?   68.20%           
===========================================================
  Files                               ?      410           
  Lines                               ?    53257           
  Branches                            ?        0           
===========================================================
  Hits                                ?    36323           
  Misses                              ?    16934           
  Partials                            ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6403714...ded8d28. Read the comment docs.

suo · 2020-09-29T17:40:04Z

torch/jit/_recursive.py

@@ -123,6 +125,10 @@ def infer_type(name, item):
    added_names = set()

    for name, item in nn_module._parameters.items():
+        if name in user_annotated_ignored_attributes:


I don't think we need to check user_annotated_ignored_attributes for each present attribute, we can just add it all to the concrete type right away, e.g.

concrete_type_builder.add_ignored_attributes(user_annotated_ignored_attributes)

The only difference would be that ignored attributes would be added even if they aren't present in the module object, but that won't make a difference in practice I think.

But we still need to skip adding attributes if they are ignored, right? So instead of

if name in user_annotated_ignored_attributes: concrete_type_builder.add_ignored_attribute(name) continue

we'd have

if name in user_annotated_ignored_attributes: continue

or

if {existing_condition} and name not in user_annotated_ignored_attributes: concrete_type_builder.add_xxx(...)

suo · 2020-09-29T17:40:29Z

torch/jit/_recursive.py

@@ -104,6 +104,8 @@ def infer_concrete_type_builder(nn_module, share_types=True):
        concrete_type_builder.set_module_list()

    class_annotations = getattr(nn_module, '__annotations__', {})
+    # Get user-annotated ignored attributes.
+    user_annotated_ignored_attributes = getattr(nn_module, "__jit_ignored_attributes__", set())


__torchscript_ignored__? JIT is a little overloaded.

:'( I just landed a change to name the equivalent class attribute for unused properties __jit_unused_properties__. I'm good with either, but I just want to make sure they're consistent. So I can either keep this or change both to __torchscript_ignored_xxx__.

suo · 2020-09-29T17:42:09Z

torch/csrc/jit/frontend/concrete_module_type.cpp

@@ -186,6 +186,10 @@ c10::optional<std::string> ConcreteModuleType::findFailedAttribute(
  return c10::nullopt;
 }

+bool ConcreteModuleType::isIgnoredAttribute(const std::string& name) const {


You need to edit ConcreteModuleTypeBuilder::equals so that this field gets used to disambiguate instances of a concrete type.

I added this but it's hard to test because ignoring an attribute changes the list of attributes on the ConcreteModuleTypeBuilder instance. The equality check between ConcreteModuleTypeBuilder instances (also ConcreteModuleType and Builder instances) fails when when an attribute is ignored even if there is no ignoredAttributes_ == other.ignoredAttributes_ condition added to ConcreteModuleTypeBuilder::equals because attributes_ == other.attributes_ is false .

I'm not saying we shouldn't have it, but I can't prove that it works.

As far as I understand, ConcreteMuduleType::equals is to disambiguate between different instantiations of an nn.Module which can result in a different type. __jit_ignored_attributes__ is a class-level attribute, I don't see how it could differ between different instantiations.

That's my understanding too, but as per one of my tests, the class attribute can be edited between invocations to torch.jit.script. I don't think it's a realistic use case, but it is possible. But as I mentioned in my earlier comment, unintended type sharing does not happen because modifying that class attribute changes which attributes are included/excluded in ConcreteModuleTypeBuilder.

It can be edited, or we can set it in the body of the __init__, e.g.

def __init__(self, ignored): self.__ignored__ = ignored

Both happen with some degree of regularity for other elements of ConcreteModuleType. But good point that it changes the available attributes, so checking ignored attrs is somewhat redundant. I think we should do it still for consistency, since all other attributes of ConcreteModuleType should participate in equality.

suo · 2020-09-29T17:53:03Z

test/test_jit_py3.py

@@ -678,6 +678,56 @@ def attr(self):
        with self.assertRaisesRegex(torch.nn.modules.module.ModuleAttributeError, "has no attribute"):
            scripted_mod.ignored_attr

+    def test_ignoring_module_attributes(self):


can we add tests to test_type_sharing.py to make sure we are not improperly sharing types with different ignored attributes sets?

I think we should stop adding to test_jit_py3.py. This file originally existed because it let us use py3 syntax, but that no longer has a purpose now that we are py3 only.

I added tests to test_type_sharing.py to test that types are shared if ignoring attrs make them equal and not shared if the ignored attributes list is modified and the types of modules before and after the modification should be different.

Where should I move them?

For 2: not sure, I wish we had the tests that exercise "compilation customization" APIs, but they have not yet been factored out of test_jit.py.

**Summary** This commit adds an API for ignoring arbitrary module attributes during scripting. A class attribute named `ignored_attributes` containing names of attributes to ignore can be added to the class of the instance being scripted. Attributes ignored in this fashion cannot be used in `forward`, methods used by `forward` or by `@exported` methods. They are, however, copied to the `RecursiveScriptModule` wrapper and can be used by `@ignored` methods and regular Python code. **Test Plan** This commit adds unit tests to `TestScriptPy3` to test this new API. Differential Revision: [D23971882](https://our.internmc.facebook.com/intern/diff/D23971882) [ghstack-poisoned]

eellison

Changes LGTM, bikeshedding was addressed, and this is improvement to the codebase so accepting. It'd also be good to land the API change for the upcoming release to reduce API churn so any remaining outstanding comments can be addressed in a follow up

eellison · 2020-10-05T21:47:09Z

torch/csrc/jit/frontend/concrete_module_type.cpp

@@ -186,6 +186,10 @@ c10::optional<std::string> ConcreteModuleType::findFailedAttribute(
  return c10::nullopt;
 }

+bool ConcreteModuleType::isIgnoredAttribute(const std::string& name) const {


As far as I understand, ConcreteMuduleType::equals is to disambiguate between different instantiations of an nn.Module which can result in a different type. __jit_ignored_attributes__ is a class-level attribute, I don't see how it could differ between different instantiations.

Addressed

**Summary** This commit adds an API for ignoring arbitrary module attributes during scripting. A class attribute named `ignored_attributes` containing names of attributes to ignore can be added to the class of the instance being scripted. Attributes ignored in this fashion cannot be used in `forward`, methods used by `forward` or by `@exported` methods. They are, however, copied to the `RecursiveScriptModule` wrapper and can be used by `@ignored` methods and regular Python code. **Test Plan** This commit adds unit tests to `TestScriptPy3` to test this new API. Differential Revision: [D23971882](https://our.internmc.facebook.com/intern/diff/D23971882) [ghstack-poisoned]

facebook-github-bot · 2020-10-07T02:14:58Z

@SplitInfinity merged this pull request in 4fdba30.

SplitInfinity requested a review from apaszke as a code owner September 24, 2020 06:06

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 24, 2020

SplitInfinity requested review from eellison and gmagogsfm September 24, 2020 18:54

eellison reviewed Sep 24, 2020

View reviewed changes

eellison requested a review from suo September 24, 2020 22:32

Meghan Lele added 5 commits September 26, 2020 11:09

eellison reviewed Sep 28, 2020

View reviewed changes

SplitInfinity requested a review from eellison September 28, 2020 21:09

suo previously requested changes Sep 29, 2020

View reviewed changes

SplitInfinity requested a review from suo September 29, 2020 19:17

eellison approved these changes Oct 5, 2020

View reviewed changes

facebook-github-bot closed this in 4fdba30 Oct 7, 2020

facebook-github-bot added the Merged label Oct 7, 2020

facebook-github-bot deleted the gh/splitinfinity/53/head branch October 10, 2020 14:17

SplitInfinity mentioned this pull request Dec 18, 2020

[jit] add ignored attributed to scriptmodule #43021

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JIT] Add API for ignoring arbitrary module attributes #45262

[JIT] Add API for ignoring arbitrary module attributes #45262

SplitInfinity commented Sep 24, 2020 •

edited

dr-ci bot commented Sep 24, 2020 •

edited

eellison left a comment

SplitInfinity commented Sep 25, 2020

eellison commented Sep 25, 2020

eellison left a comment

SplitInfinity commented Sep 28, 2020

codecov bot commented Sep 29, 2020 •

edited

suo Sep 29, 2020

SplitInfinity Sep 29, 2020

suo Sep 29, 2020

SplitInfinity Sep 29, 2020

suo Sep 29, 2020

SplitInfinity Sep 29, 2020

eellison Oct 5, 2020

SplitInfinity Oct 5, 2020 •

edited

suo Oct 6, 2020

suo Sep 29, 2020

SplitInfinity Sep 29, 2020

suo Oct 6, 2020

eellison left a comment

eellison Oct 5, 2020

facebook-github-bot commented Oct 7, 2020

[JIT] Add API for ignoring arbitrary module attributes #45262

[JIT] Add API for ignoring arbitrary module attributes #45262

Conversation

SplitInfinity commented Sep 24, 2020 • edited

dr-ci bot commented Sep 24, 2020 • edited

💊 CI failures summary and remediations

eellison left a comment

Choose a reason for hiding this comment

SplitInfinity commented Sep 25, 2020

eellison commented Sep 25, 2020

eellison left a comment

Choose a reason for hiding this comment

SplitInfinity commented Sep 28, 2020

codecov bot commented Sep 29, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SplitInfinity Oct 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eellison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 7, 2020

SplitInfinity commented Sep 24, 2020 •

edited

dr-ci bot commented Sep 24, 2020 •

edited

codecov bot commented Sep 29, 2020 •

edited

SplitInfinity Oct 5, 2020 •

edited