[jit] Better match behavior of loaded ScriptModules vs. freshly created ones #43298

suo · 2020-08-19T23:18:45Z

Stack from ghstack:

[jit] Better match behavior of loaded ScriptModules vs. freshly created ones #43298 [jit] Better match behavior of loaded ScriptModules vs. freshly created ones
[jit] fix segfault in attribute lookup on loaded ScriptModules #43284 [jit] fix segfault in attribute lookup on loaded ScriptModules

IR emitter uses ModuleValue to represent ScriptModules and emit IR for
attribute access, submodule access, etc.

ModuleValue relies on two pieces of information, the JIT type of the
module, and the ConcreteModuleType, which encapsulates Python-only
information about the module.

ScriptModules loaded from a package used to create a dummy
ConcreteModuleType without any info in it. This led to divergences in
behavior during compilation.

This PR makes the two ways of constructing a ConcreteModuleType equivalent,
modulo any py-only information (which, by definition, is never present in
packaged files anyway).

Differential Revision: D23228738

…ed ones IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). [ghstack-poisoned]

dr-ci · 2020-08-19T23:19:59Z

💊 CI failures summary and remediations

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

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

ci.pytorch.org: 2 failed

Failed: pr/caffe2-pytorch-linux-bionic-rocm3.7-py3.6-test
Failed: pr/pytorch-linux-bionic-rocm3.7-py3.6

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

…eshly created ones" IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). [ghstack-poisoned]

…ed ones IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). ghstack-source-id: 5fb8f8a Pull Request resolved: #43298

…eshly created ones" IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). Differential Revision: [D23228738](https://our.internmc.facebook.com/intern/diff/D23228738) [ghstack-poisoned]

…ed ones IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). ghstack-source-id: 00997e5 Pull Request resolved: #43298

SplitInfinity

One additional question: do we need to do anything for function attributes? I see that they are part of the ConcreteModuleType, but not the JIT type.

SplitInfinity · 2020-08-24T21:14:28Z

torch/csrc/jit/frontend/concrete_module_type.cpp

+    // Populate the builder metadata from the JIT type. This is to ensure
+    // ConcreteModuleTypes produced from Python and ones produced from a JIT
+    // type directly behave the same to the rest of the system.
+    for (size_t i = 0; i < classType->numAttributes(); i++) {


Suggested change

for (size_t i = 0; i < classType->numAttributes(); i++) {

for (size_t i = 0, e = classType->numAttributes(); i < e, i++) {

I think this makes the code a little less readable for no clear benefit, so I'm going to skip this one unless you feel strongly about it.

I prefer my suggestion to avoid making an extra function call every time the loop exit condition is tested: https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop

But numAttributes() is not an expensive call, so I will not lose sleep over this.

yes, I would expect a compiler to inline and then avoid recomputing numAttributes(). I think the suggestion makes more sense if we are computing the iterator of a complicated data structure instead of returning an int. Although if the data structure is that complicated, writing a raw for loop over its iterator is probably bad to begin with…

torch/csrc/jit/frontend/concrete_module_type.cpp

SplitInfinity · 2020-08-24T22:56:46Z

torch/csrc/jit/frontend/concrete_module_type.cpp

+      }
+    }
+
+    for (size_t i = 0; i < classType->numConstants(); i++) {


Suggested change

for (size_t i = 0; i < classType->numConstants(); i++) {

for (size_t i = 0, e = classType->numConstants(); i < e, i++) {

torch/csrc/jit/frontend/concrete_module_type.h

test/test_jit.py

suo · 2020-08-25T17:48:19Z

One additional question: do we need to do anything for function attributes? I see that they are part of the ConcreteModuleType, but not the JIT type.

Anything that is not representable on the JIT type can't be captured here. This makes saving/loading a "lossy", since once you save/load all you have is a JIT type, and lose all the other Python-y bits like function attributes, module iterator kind, etc. The way to reduce this lossiness is by making more things representable first-class on the JIT type.

…eshly created ones" IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). Differential Revision: [D23228738](https://our.internmc.facebook.com/intern/diff/D23228738) [ghstack-poisoned]

…ed ones IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). ghstack-source-id: 6baa83f Pull Request resolved: #43298

test/test_jit.py

SplitInfinity · 2020-08-25T19:24:12Z

torch/csrc/jit/frontend/concrete_module_type.cpp

+    // Populate the builder metadata from the JIT type. This is to ensure
+    // ConcreteModuleTypes produced from Python and ones produced from a JIT
+    // type directly behave the same to the rest of the system.
+    for (size_t i = 0; i < classType->numAttributes(); i++) {


I prefer my suggestion to avoid making an extra function call every time the loop exit condition is tested: https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop

But numAttributes() is not an expensive call, so I will not lose sleep over this.

…eshly created ones" IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). Differential Revision: [D23228738](https://our.internmc.facebook.com/intern/diff/D23228738) [ghstack-poisoned]

…ed ones IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). ghstack-source-id: 40135c8 Pull Request resolved: #43298

SplitInfinity

🥇

…eshly created ones" IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). Differential Revision: [D23228738](https://our.internmc.facebook.com/intern/diff/D23228738) [ghstack-poisoned]

…ed ones IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). ghstack-source-id: 6c20331 Pull Request resolved: #43298

…eshly created ones" IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). Differential Revision: [D23228738](https://our.internmc.facebook.com/intern/diff/D23228738) [ghstack-poisoned]

…ed ones IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). ghstack-source-id: e0c6e82 Pull Request resolved: #43298

…eshly created ones" IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). Differential Revision: [D23228738](https://our.internmc.facebook.com/intern/diff/D23228738) [ghstack-poisoned]

…ed ones IR emitter uses `ModuleValue` to represent ScriptModules and emit IR for attribute access, submodule access, etc. `ModuleValue` relies on two pieces of information, the JIT type of the module, and the `ConcreteModuleType`, which encapsulates Python-only information about the module. ScriptModules loaded from a package used to create a dummy ConcreteModuleType without any info in it. This led to divergences in behavior during compilation. This PR makes the two ways of constructing a ConcreteModuleType equivalent, modulo any py-only information (which, by definition, is never present in packaged files anyway). ghstack-source-id: 9caee09 Pull Request resolved: #43298

codecov · 2020-09-03T20:39:30Z

Codecov Report

Merging #43298 into gh/suo/345/base will increase coverage by 0.00%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           gh/suo/345/base   #43298   +/-   ##
================================================
  Coverage            69.27%   69.27%           
================================================
  Files                  381      381           
  Lines                47265    47265           
================================================
+ Hits                 32744    32745    +1     
+ Misses               14521    14520    -1

Impacted Files	Coverage Δ
torch/testing/_internal/expecttest.py	`77.55% <0.00%> (-1.03%)`	⬇️
torch/utils/_benchmark/utils/common.py	`78.99% <0.00%> (+1.68%)`	⬆️

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 653f684...42cf2bd. Read the comment docs.

facebook-github-bot · 2020-09-04T04:14:30Z

@suo merged this pull request in 9dd8670.

suo requested a review from apaszke as a code owner August 19, 2020 23:18

suo mentioned this pull request Aug 19, 2020

[jit] fix segfault in attribute lookup on loaded ScriptModules #43284

Closed

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

suo requested a review from SplitInfinity August 24, 2020 19:21

SplitInfinity reviewed Aug 24, 2020

View reviewed changes

suo requested a review from SplitInfinity August 25, 2020 18:38

SplitInfinity reviewed Aug 25, 2020

View reviewed changes

SplitInfinity approved these changes Aug 27, 2020

View reviewed changes

facebook-github-bot closed this in 9dd8670 Sep 3, 2020

facebook-github-bot added the merged label Sep 4, 2020

facebook-github-bot deleted the gh/suo/345/head branch September 7, 2020 14:17

mruberry added the Merged label Oct 28, 2020

	for (size_t i = 0; i < classType->numAttributes(); i++) {
	for (size_t i = 0, e = classType->numAttributes(); i < e, i++) {

	for (size_t i = 0; i < classType->numConstants(); i++) {
	for (size_t i = 0, e = classType->numConstants(); i < e, i++) {

[jit] Better match behavior of loaded ScriptModules vs. freshly created ones #43298

[jit] Better match behavior of loaded ScriptModules vs. freshly created ones #43298

Uh oh!

Conversation

suo commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 2 failed

Uh oh!

SplitInfinity left a comment

Choose a reason for hiding this comment

Uh oh!

SplitInfinity Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

suo Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

SplitInfinity Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

suo Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SplitInfinity Aug 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suo commented Aug 25, 2020

Uh oh!

Uh oh!

SplitInfinity Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

SplitInfinity left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

facebook-github-bot commented Sep 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

suo commented Aug 19, 2020 •

edited

Loading

dr-ci bot commented Aug 19, 2020 •

edited

Loading

codecov bot commented Sep 3, 2020 •

edited

Loading