fix len, contains, getitem inherited from interface class derived from nn container (closes #40603) #40789

wconstab · 2020-06-30T16:56:52Z

Define static script implementation of len and contains on any subclass derived from a type such as ModuleList, Sequential, or ModuleDict. Implement getitem for classes derived from ModuleDict.

…eDict

…ke ModuleDict __iter__

…kModule is more strict

dr-ci · 2020-06-30T17:24:36Z

💊 CI failures summary and remediations

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

3/3 failures possibly* introduced in this PR
- 1/3 non-CircleCI failure(s)

🕵️ 2 new failures recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

pytorch_xla_linux_bionic_py3_6_clang9_build (1/2)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result.

CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/simple/docker_definitions.py 
Auto-merging .circleci/cimodel/data/simple/docker_definitions.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/simple/bazel_definitions.py 
Auto-merging .circleci/cimodel/data/simple/bazel_definitions.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/simple/android_definitions.py 
Auto-merging .circleci/cimodel/data/simple/android_definitions.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_definitions.py 
Auto-merging .circleci/cimodel/data/pytorch_build_definitions.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_data.py 
Auto-merging .circleci/cimodel/data/pytorch_build_data.py 
Automatic merge failed; fix conflicts and then commit the result.

pytorch_linux_xenial_py3_6_gcc5_4_build (2/2)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result.

CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/simple/docker_definitions.py 
Auto-merging .circleci/cimodel/data/simple/docker_definitions.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/simple/bazel_definitions.py 
Auto-merging .circleci/cimodel/data/simple/bazel_definitions.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/simple/android_definitions.py 
Auto-merging .circleci/cimodel/data/simple/android_definitions.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_definitions.py 
Auto-merging .circleci/cimodel/data/pytorch_build_definitions.py 
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_data.py 
Auto-merging .circleci/cimodel/data/pytorch_build_data.py 
Automatic merge failed; fix conflicts and then commit the result.

ci.pytorch.org: 1 failed

Failed: pr/caffe2-pytorch-linux-xenial-rocm3.3-py3.6-test

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 22 times.

…r-provided script methods

facebook-github-bot

@wconstab has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

wconstab · 2020-07-01T16:01:25Z

hmm, failing backward_compatibility, no idea why. Will have to take a look at this later.

Jul 01 00:27:26 The PR is introducing backward incompatible changes to the operator library. Please contact PyTorch team to confirm whether this change is wanted or not.
Jul 01 00:27:26
Jul 01 00:27:26 Broken ops: [
Jul 01 00:27:26 aten::view_as(Tensor self, Tensor other) -> (Tensor)
Jul 01 00:27:26 aten::reshape_as(Tensor self, Tensor other) -> (Tensor)
Jul 01 00:27:26 aten::pin_memory(Tensor self) -> (Tensor)
Jul 01 00:27:26 aten::reshape(Tensor self, int[] shape) -> (Tensor)
Jul 01 00:27:26 aten::detach(Tensor self) -> (Tensor)
Jul 01 00:27:26 aten::expand_as(Tensor self, Tensor other) -> (Tensor)
Jul 01 00:27:26 aten::flatten.DimnameList(Tensor self, str[] dims, str out_dim) -> (Tensor)
Jul 01 00:27:26 aten::flatten.named_out_dim(Tensor self, int start_dim, int end_dim, str out_dim) -> (Tensor)
Jul 01 00:27:26 aten::flatten.using_ints(Tensor self, int start_dim=0, int end_dim=-1) -> (Tensor)
Jul 01 00:27:26 aten::flatten.using_names(Tensor self, str start_dim, str end_dim, str out_dim) -> (Tensor)
Jul 01 00:27:26 aten::contiguous(Tensor self, *, int memory_format=0) -> (Tensor)
Jul 01 00:27:26 ]

eellison

Cool! Looks great! a few small small comments here or there (for nits feel free to not take the suggestion obviously). I think we should create a new test file for this test and some of the existing ones.

The two issues you've fixed here are in the same class of issues, but dont really have a dependence on each other. In the future it would nice to use ghstack to cover similar cases...

eellison · 2020-07-01T17:44:03Z

test/test_jit.py

+                assert self.moduledict['blah'] == "blah", "this is a keyerror"
+
+        with self.assertRaisesRegex(RuntimeError, "Key Error, blah"):
+            b = BadModule()


maybe just invoke torch.jit.script(BadModule()) here to show that it fails at compilation and not runtime

@eellison what do you mean by using ghstack? i'm only vaguely familiar with that tool, not sure how to use it

https://github.com/ezyang/ghstack

If you search internally there should be a good number of docs on how to use it.

eellison · 2020-07-01T17:45:18Z

torch/csrc/jit/python/python_sugared_value.cpp

    return getSugaredDict(loc, m)->getModules()->getitem(loc, m, idx);
+  } else if (
+      concreteType_->getIterableModuleKind() == IterableModuleKind::DICT) {
+    if (idx->type()->kind() == c10::TypeKind::StringType) {


nit: maybe more idiomatic to do type->cast<StringType>(), maybe put toIValue(idx) in conditional to reduce nesting

yup. actually, i think i can totally drop the StringType check, as toIValue does a type() == StringType inside, and will return none otherwise. see latest rev once i push.

eellison · 2020-07-01T17:47:22Z

torch/csrc/jit/python/python_sugared_value.cpp

+          auto key = keys_iter->tup_.at(i);
+          auto key_str = toIValue(key->asValue(loc, m))->toStringRef();
+          if (key_str == idx_str) {
+            std::shared_ptr<SugaredValue> module_sugared_value =


since we're returning SugaredValuePtr here, there's no need to std::dynamic_pointer_cast< ModuleValue>., can just return module_values_iter->tup_.at(i)

eellison · 2020-07-01T17:52:46Z

test/test_jit.py

        with self.assertRaisesRegex(RuntimeError, "'int' object is not iterable"):
            M()

+    def test_module_interface_special_methods(self):


With this test, and test_sequential_intermediary_types, test_moduledict, test_custom_container_forward, test_script_module_list_sequential, test_script_modulelist_index, it's probably worth splitting this off to a separate test file: test/jit/test_module_containers

eellison · 2020-07-01T18:48:08Z

torch/jit/_recursive.py

        concrete_type_store.methods_compiled.add(concrete_type)

+    # Special handling so methods like __len__ work in script methods on classes derived from containers
+    if isinstance(nn_module, (torch.nn.ModuleList, torch.nn.Sequential, torch.nn.ModuleDict)) and \


maybe worth checking that the module doesn't override the ipmlementation in the ModuleList/Sequential/ModuleDict

what do you mean? i think i actually handled this case correctly, although implicitly, by doing this block of code right after the '# Compile methods if necessary' block. IIUC, if they override len it gets compiled in that block, then my block sees len exist and bails. I did confirm this in one particular test case which overrides len.

I think of someone overrides len but doesn’t add @export to it, it won’t be present on the cpp module when we do the check on the cpp module and we’ll still do the custom compilation

wconstab · 2020-07-01T20:05:37Z

cool i'll make those changes. thanks!

facebook-github-bot

@wconstab has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@wconstab has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@wconstab has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-07-05T00:16:06Z

@wconstab merged this pull request in 8ecd4f3.

wconstab added 10 commits June 29, 2020 14:45

define a static __len__ method for ModuleList, Sequential, ModuleDict

626acc1

WIP add test for methods inherited from ModuleList, Sequential, Modul…

fe72a4d

…eDict

fix test

2b64af9

update test to reflect what currently passes

fcfcd06

fix ModuleDict string indexing

a8d8a50

WIP add invocation of compiled method, fix alias issue, seemingly bro…

8336161

…ke ModuleDict __iter__

confirm that old way of running module still works, something in chec…

77f01b9

…kModule is more strict

define statically populated __contains__ for ModuleDict

bb11e37

not sure why this one still fails

3027f44

rename test

6faea3a

wconstab requested a review from eellison June 30, 2020 16:56

wconstab requested a review from apaszke as a code owner June 30, 2020 16:56

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

wconstab added 3 commits June 30, 2020 13:20

this seems like a better place to define the special methods

7f501ab

fix

5b7448e

remove duplicate test and fix order of defining __len__ after any use…

e516940

…r-provided script methods

wconstab changed the title ~~[WIP] Wconstab/40603 subclass~~ fix __len__, __contains__, getitem inherited from interface class derived from nn container Jun 30, 2020

wconstab self-assigned this Jun 30, 2020

wconstab added 3 commits June 30, 2020 16:32

add handling and error for invalid string literal idx

baffbc2

finish test_module_interface_special_methods

5b99c0d

update test to check newest error

7fd2646

wconstab changed the title ~~fix __len__, __contains__, getitem inherited from interface class derived from nn container~~ fix __len__, __contains__, getitem inherited from interface class derived from nn container (closes #40603) Jun 30, 2020

fix flake8

5db700b

facebook-github-bot reviewed Jun 30, 2020

View reviewed changes

eellison approved these changes Jul 1, 2020

View reviewed changes

eellison reviewed Jul 1, 2020

View reviewed changes

refactor tests into separate file test_module_containers.py

2f03646

wconstab added 3 commits July 1, 2020 13:19

refactor return type

805e8d5

show that asserts happen during compilation not runtime

7315897

refactor conditionals in ModuleValue::getitem

be92683

facebook-github-bot reviewed Jul 1, 2020

View reviewed changes

Add test for __len__ override

3f70d6d

facebook-github-bot reviewed Jul 2, 2020

View reviewed changes

fix generated __contains__ for case where dict is empty

3554566

facebook-github-bot reviewed Jul 2, 2020

View reviewed changes

facebook-github-bot closed this in 8ecd4f3 Jul 4, 2020

facebook-github-bot added the merged label Jul 5, 2020

mruberry added the Merged label Oct 28, 2020

fix __len__, __contains__, getitem inherited from interface class derived from nn container (closes #40603) #40789

fix __len__, __contains__, getitem inherited from interface class derived from nn container (closes #40603) #40789

Uh oh!

Conversation

wconstab commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

🕵️ 2 new failures recognized by patterns

pytorch_xla_linux_bionic_py3_6_clang9_build (1/2)

pytorch_linux_xenial_py3_6_gcc5_4_build (2/2)

ci.pytorch.org: 1 failed

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

wconstab commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eellison 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!

wconstab commented Jul 1, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix len, contains, getitem inherited from interface class derived from nn container (closes #40603) #40789

fix len, contains, getitem inherited from interface class derived from nn container (closes #40603) #40789

wconstab commented Jun 30, 2020 •

edited

Loading

dr-ci bot commented Jun 30, 2020 •

edited

Loading

wconstab commented Jul 1, 2020 •

edited

Loading