[Inlined Callstack Fix] Fix inlined callstack for blocks of the node. #56562

kimishpatel · 2021-04-21T04:07:22Z

Stack from ghstack:

[Pytorch Delegated Backend] Save function name in debug info #57481 [Pytorch Delegated Backend] Save function name in debug info
[Pytorch Delegated Backend] Save operator name and function name in #57441 [Pytorch Delegated Backend] Save operator name and function name in
Add python binding for generate_debug_handles #57156 [Pytorch Delegated Backend] Add python binding for generate_debug_handles
[Pytorch Backend delegation] Add api for backend lowering to query debug #55462 [Pytorch Backend delegation] Add api for backend lowering to query debug
[Inlined Callstack Fix] Fix inlined callstack for blocks of the node. #56562 [Inlined Callstack Fix] Fix inlined callstack for blocks of the node.
[Pytorch Mobile DebugInfo Serialization] Save debug handles for all instructions. #55252 [Pytorch Mobile DebugInfo Serialization] Save debug handles for all instructions.
[Pytorch, Mobile] Serialize inlined callstack pointer with debug handle. #55062 [Pytorch, Mobile] Serialize inlined callstack pointer with debug handle.
[PyTorch, Mobile] Serialization format change for source range #54284 [PyTorch, Mobile] Serialization format change for source range

Summary:
Earlier inlined callstack was annotated only nodes. This left out nodes
such as If which have block of nodes. These nodes should also be updated
similarly.

Test Plan:
Added test in test_misc

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D27902516

Summary: Earlier inlined callstack was annotated only nodes. This left out nodes such as If which have block of nodes. These nodes should also be updated similarly. Test Plan: Added test in test_misc Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

facebook-github-bot · 2021-04-21T04:07:29Z

💊 CI failures summary and remediations

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

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

🕵️ 2 new failures recognized by patterns

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

pytorch_linux_xenial_py3_clang5_asan_test1 (1/2)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun)

May 04 00:54:47 SUMMARY: UndefinedBehaviorSanit.../jenkins/workspace/aten/src/ATen/Utils.cpp:20:3 in

May 04 00:54:47     #7 0x56513f82651b in PyEval_EvalCode /tmp/build/80754af9/python_1614113050744/work/Python/ceval.c:731
May 04 00:54:47     #8 0x56513f8a65e3 in run_mod /tmp/build/80754af9/python_1614113050744/work/Python/pythonrun.c:1025
May 04 00:54:47     #9 0x56513f8a667c in PyRun_StringFlags /tmp/build/80754af9/python_1614113050744/work/Python/pythonrun.c:949
May 04 00:54:47     #10 0x56513f8a66de in PyRun_SimpleStringFlags /tmp/build/80754af9/python_1614113050744/work/Python/pythonrun.c:445
May 04 00:54:47     #11 0x56513f8aa4e2 in run_command /tmp/build/80754af9/python_1614113050744/work/Modules/main.c:301
May 04 00:54:47     #12 0x56513f8aa4e2 in Py_Main /tmp/build/80754af9/python_1614113050744/work/Modules/main.c:749
May 04 00:54:47     #13 0x56513f774b0d in main /tmp/build/80754af9/python_1614113050744/work/Programs/python.c:69
May 04 00:54:47     #14 0x7f3494ceb83f in __libc_start_main /build/glibc-e6zv40/glibc-2.23/csu/../csu/libc-start.c:291
May 04 00:54:47     #15 0x56513f853d6f in _start /home/rdonnelly/mc/conda-bld/compilers_linux-64_1534865402226/work/.build/src/glibc-2.12.2/csu/../sysdeps/x86_64/elf/start.S:103
May 04 00:54:47 
May 04 00:54:47 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /var/lib/jenkins/workspace/aten/src/ATen/Utils.cpp:20:3 in 
May 04 00:54:47 + retcode=1
May 04 00:54:47 + set -e
May 04 00:54:47 + return 1
May 04 00:54:47 + [[ pytorch-linux-xenial-py3-clang5-asan-test1 == *-NO_AVX-* ]]
May 04 00:54:47 + [[ pytorch-linux-xenial-py3-clang5-asan-test1 == *-NO_AVX2-* ]]
May 04 00:54:47 + IN_PULL_REQUEST=https://github.com/pytorch/pytorch/pull/56562
May 04 00:54:47 + '[' -n https://github.com/pytorch/pytorch/pull/56562 ']'
May 04 00:54:47 + [[ pytorch-linux-xenial-py3-clang5-asan-test1 != *coverage* ]]
May 04 00:54:47 ++ mktemp
May 04 00:54:47 + DETERMINE_FROM=/tmp/tmp.YGpHoORMjT

pytorch_xla_linux_bionic_py3_6_clang9_test (2/2)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun)

May 04 00:22:10 ERROR [1.738s]: test_clamp_xla_int64 (__main__.TestDevicePrecisionXLA)

May 04 00:22:10 	PyRun_FileExFlags
May 04 00:22:10 	PyRun_SimpleFileExFlags
May 04 00:22:10 	Py_Main
May 04 00:22:10 	main
May 04 00:22:10 	__libc_start_main
May 04 00:22:10 	
May 04 00:22:10 *** End stack trace ***
May 04 00:22:10 
May 04 00:22:10 
May 04 00:22:10 ======================================================================
May 04 00:22:10 ERROR [1.738s]: test_clamp_xla_int64 (__main__.TestDevicePrecisionXLA)
May 04 00:22:10 ----------------------------------------------------------------------
May 04 00:22:10 Traceback (most recent call last):
May 04 00:22:10   File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 297, in instantiated_test
May 04 00:22:10     raise rte
May 04 00:22:10   File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 292, in instantiated_test
May 04 00:22:10     result = test_fn(self, *args)
May 04 00:22:10   File "/var/lib/jenkins/workspace/xla/test/../../test/test_torch.py", line 7500, in test_clamp
May 04 00:22:10     actual = x[..., :1].clamp(lb, ub)
May 04 00:22:10 RuntimeError: /var/lib/jenkins/workspace/xla/third_party/tensorflow/bazel-tensorflow/tensorflow/compiler/xla/xla_client/debug_macros.h:27 : Check failed: status.status() == ::tensorflow::Status::OK() (Invalid argument: Input dimension should be either 1 or equal to the output dimension it is broadcasting into; the 1th operand dimension is 50, the 1th output dimension is 1. vs. OK)
May 04 00:22:10 *** Begin stack trace ***

1 job timed out:

pytorch_linux_xenial_py3_clang5_asan_test1

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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

…f the node." Summary: Earlier inlined callstack was annotated only nodes. This left out nodes such as If which have block of nodes. These nodes should also be updated similarly. Test Plan: Added test in test_misc Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D27902516](https://our.internmc.facebook.com/intern/diff/D27902516) [ghstack-poisoned]

ZolotukhinM

Overall this looks reasonable, I have a couple of question inline:

ZolotukhinM · 2021-04-22T19:43:14Z

test/cpp/jit/test_misc.cpp

+  for (Node* n : graph->nodes()) {
+    if (n->kind() == prim::If) {
+      for (Block* block : n->blocks()) {
+        for (Node* if_node : block->nodes()) {
+          if (if_node->kind() == aten::add) {


Nit: this could be factored out as a function findNodeOfGivenKind or something like this.

Btw, you could try to use subgraph matcher for this, but i'm not sure that with the required boilerplate code it would be beneficial.

This comment still applies.

True. But looking at this, it looks like it is just a 10ish lines of code without much resuse. If more similar code is added I suppose we can refactor. So for now I will leave it as is unless you have a strong opinion about it.

ZolotukhinM · 2021-04-22T19:43:17Z

torch/csrc/jit/ir/ir.cpp

+    InlinedCallStack* raw_callstack_ptr =
+        new_node_cs ? new_node_cs->get() : nullptr;
+
+    if (!new_cs_entires.count(raw_callstack_ptr)) {
+      if (new_node_cs) {
+        new_cs_entires[raw_callstack_ptr] =
+            c10::make_intrusive<InlinedCallStack>(
+                *new_node_cs, callee, to_replace->sourceRange(), m_info);
+      } else {
+        new_cs_entires[raw_callstack_ptr] =
+            c10::make_intrusive<InlinedCallStack>(
+                callee, to_replace->sourceRange(), m_info);
+      }
+    }


This seem to duplicate the code from inlineCallTo almost identically. Would it be possible to get rid of that duplication by calling inlineCallStackOfBlock there?

Yes. I was almost gonna do that, but wanted to move too fast. Let me refactor it.

Can you fix what I assume is a typo: new_cs_entires -> new_cs_entries

Also, why is it possible that

!new_cs_entires.count(raw_callstack_ptr) is true

but if (new_node_cs) is false?

Wouldn't if (new_node_cs) false means that raw_callstack_ptr is nullptr, and that new_cs_entires.count(raw_callstack_ptr) would be zero?

Also, why is it possible that !new_cs_entires.count(raw_callstack_ptr) is true but if (new_node_cs) is false?

So say you are inlining:

class B(..): def forward(self, x): return x + 3 class A(..): def forward(self, y): return self.b.forward(y)

Then when you are inlining A's forward method you replace
%something = prim::CallMethod(...) with %something = aten::add(..
This aten::add node is created by cloning B's forward method. At that time aten::add node's cs is null. And new_cs_entires may not have an entry for raw_callstack_ptr = nullptr so if (!new_cs_entires.count(raw_callstack_ptr)) would be true.

Although I wonder if there is a bug here: Meaning if all the node's that dont have callstack ptr get clobbered together in new_cs_entires. But that is ok because we keep replacing them? @ZolotukhinM to confirm.

raziel · 2021-04-22T21:42:36Z

torch/csrc/jit/ir/ir.cpp

    new_node->setCallStack(new_callstack_entries.at(raw_callstack_ptr));
+    // We updated the inlined callstack of new_node.
+    // Same must be done for the nodes of the blocks of new_nodes.
+    // For exampl If node's block otherwise is not annotated appropriately.


typo: exampl -> example

raziel · 2021-04-22T23:44:34Z

torch/csrc/jit/ir/ir.cpp

+    InlinedCallStack* raw_callstack_ptr =
+        new_node_cs ? new_node_cs->get() : nullptr;
+
+    if (!new_cs_entires.count(raw_callstack_ptr)) {
+      if (new_node_cs) {
+        new_cs_entires[raw_callstack_ptr] =
+            c10::make_intrusive<InlinedCallStack>(
+                *new_node_cs, callee, to_replace->sourceRange(), m_info);
+      } else {
+        new_cs_entires[raw_callstack_ptr] =
+            c10::make_intrusive<InlinedCallStack>(
+                callee, to_replace->sourceRange(), m_info);
+      }
+    }


Can you fix what I assume is a typo: new_cs_entires -> new_cs_entries

Also, why is it possible that

!new_cs_entires.count(raw_callstack_ptr) is true

but if (new_node_cs) is false?

Wouldn't if (new_node_cs) false means that raw_callstack_ptr is nullptr, and that new_cs_entires.count(raw_callstack_ptr) would be zero?

raziel · 2021-04-23T00:51:58Z

test/cpp/jit/test_misc.cpp

+  ASSERT_NE(mul_ss.str().find("line 6"), std::string::npos);
+  ASSERT_NE(
+      mul_ss.str().find("return self.A0.forward(x, y, z)"), std::string::npos);
+  ASSERT_NE(add_ss.str().find("return x * y"), std::string::npos);


why is return x * y in add_ss and not mul_ss?

Oh thats a typo. Althoug the reason this still passes is because I think source range prints a few lines (2 I think) around the actual source.

so are you changing

ASSERT_NE(add_ss.str().find("return x * y"), std::string::npos);

to

ASSERT_NE(mul_ss.str().find("return x * y"), std::string::npos);

I really thought I did.

…f the node." Summary: Earlier inlined callstack was annotated only nodes. This left out nodes such as If which have block of nodes. These nodes should also be updated similarly. Test Plan: Added test in test_misc Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D27902516](https://our.internmc.facebook.com/intern/diff/D27902516) [ghstack-poisoned]

raziel · 2021-04-27T20:22:48Z

test/cpp/jit/test_misc.cpp

+  ASSERT_NE(mul_ss.str().find("line 6"), std::string::npos);
+  ASSERT_NE(
+      mul_ss.str().find("return self.A0.forward(x, y, z)"), std::string::npos);
+  ASSERT_NE(add_ss.str().find("return x * y"), std::string::npos);


so are you changing

ASSERT_NE(add_ss.str().find("return x * y"), std::string::npos);

to

ASSERT_NE(mul_ss.str().find("return x * y"), std::string::npos);

raziel · 2021-04-27T21:43:20Z

torch/csrc/jit/ir/ir.cpp

+
+void inlineCallStackOfNode(
+    Node* new_node,
+    std::unordered_map<InlinedCallStack*, InlinedCallStackPtr>& new_cs_entires,


So the name is really new_cs_entires? not new_cs_entries

LOL. OK thanks for that catch. I missed that somehow.

…f the node." Summary: Earlier inlined callstack was annotated only nodes. This left out nodes such as If which have block of nodes. These nodes should also be updated similarly. Test Plan: Added test in test_misc Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D27902516](https://our.internmc.facebook.com/intern/diff/D27902516) [ghstack-poisoned]

ZolotukhinM

LGTM, only a couple of minor remarks from me.

ZolotukhinM · 2021-04-30T22:24:30Z

test/cpp/jit/test_misc.cpp

+  for (Node* n : graph->nodes()) {
+    if (n->kind() == prim::If) {
+      for (Block* block : n->blocks()) {
+        for (Node* if_node : block->nodes()) {
+          if (if_node->kind() == aten::add) {


This comment still applies.

ZolotukhinM · 2021-04-30T22:25:57Z

test/cpp/jit/test_misc.cpp

+  auto graph = c.get_method("forward").graph();
+  torch::jit::Inline(*graph);


If you use optimized_graph() method you won't need to call Inline directly (similarly to how it was done in existing tests).

ZolotukhinM · 2021-04-30T22:26:18Z

test/cpp/jit/test_misc.cpp

 #include <torch/csrc/jit/passes/graph_fuser.h>
 #include <torch/csrc/jit/passes/guard_elimination.h>
 #include <torch/csrc/jit/passes/inline_autodiff_subgraphs.h>
+#include <torch/csrc/jit/passes/inliner.h>


This would be unnecessary if we use optimized_graph instead of just graph.

…f the node." Summary: Earlier inlined callstack was annotated only nodes. This left out nodes such as If which have block of nodes. These nodes should also be updated similarly. Test Plan: Added test in test_misc Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D27902516](https://our.internmc.facebook.com/intern/diff/D27902516) [ghstack-poisoned]

Summary: Earlier inlined callstack was annotated only nodes. This left out nodes such as If which have block of nodes. These nodes should also be updated similarly. Test Plan: Added test in test_misc Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 715b76b Pull Request resolved: pytorch#56562

…f the node." Summary: Earlier inlined callstack was annotated only nodes. This left out nodes such as If which have block of nodes. These nodes should also be updated similarly. Test Plan: Added test in test_misc Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D27902516](https://our.internmc.facebook.com/intern/diff/D27902516) [ghstack-poisoned]

Summary: Earlier inlined callstack was annotated only nodes. This left out nodes such as If which have block of nodes. These nodes should also be updated similarly. Test Plan: Added test in test_misc Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: d67dba6 Pull Request resolved: pytorch#56562

…f the node." Summary: Earlier inlined callstack was annotated only nodes. This left out nodes such as If which have block of nodes. These nodes should also be updated similarly. Test Plan: Added test in test_misc Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D27902516](https://our.internmc.facebook.com/intern/diff/D27902516) [ghstack-poisoned]

facebook-github-bot · 2021-05-04T16:21:38Z

This pull request has been merged in 5326ec6.

…pytorch#56562) Summary: Pull Request resolved: pytorch#56562 Earlier inlined callstack was annotated only nodes. This left out nodes such as If which have block of nodes. These nodes should also be updated similarly. Test Plan: Added test in test_misc Imported from OSS Reviewed By: ZolotukhinM Differential Revision: D27902516 fbshipit-source-id: 4e65c686fa6b4977e8719db45f71f7d2599d4d8e

This was referenced Apr 21, 2021

[PyTorch, Mobile] Serialization format change for source range #54284

Closed

[Pytorch, Mobile] Serialize inlined callstack pointer with debug handle. #55062

Closed

[Pytorch Mobile DebugInfo Serialization] Save debug handles for all instructions. #55252

Closed

facebook-github-bot added oncall: jit Add this issue/PR to JIT oncall triage queue cla signed labels Apr 21, 2021

kimishpatel mentioned this pull request Apr 21, 2021

[Pytorch Backend delegation] Add api for backend lowering to query debug #55462

Closed

kimishpatel requested review from ZolotukhinM, eellison and raziel April 21, 2021 04:09

kimishpatel added 5 commits April 20, 2021 21:11

ZolotukhinM reviewed Apr 22, 2021

View reviewed changes

raziel reviewed Apr 23, 2021

View reviewed changes

kimishpatel requested review from ZolotukhinM and raziel April 26, 2021 23:48

raziel reviewed Apr 27, 2021

View reviewed changes

kimishpatel mentioned this pull request Apr 28, 2021

Add python binding for generate_debug_handles #57156

Closed

kimishpatel added 3 commits April 28, 2021 14:23

ZolotukhinM approved these changes Apr 30, 2021

View reviewed changes

kimishpatel mentioned this pull request May 2, 2021

[Pytorch Delegated Backend] Save operator name and function name in #57441

Closed

kimishpatel mentioned this pull request May 3, 2021

[Pytorch Delegated Backend] Save function name in debug info #57481

Closed

kimishpatel added 3 commits May 3, 2021 14:05

facebook-github-bot closed this in 5326ec6 May 4, 2021

facebook-github-bot added the Merged label May 4, 2021

facebook-github-bot deleted the gh/kimishpatel/51/head branch May 8, 2021 14:17

		auto graph = c.get_method("forward").graph();
		torch::jit::Inline(*graph);

[Inlined Callstack Fix] Fix inlined callstack for blocks of the node. #56562

[Inlined Callstack Fix] Fix inlined callstack for blocks of the node. #56562

Uh oh!

Conversation

kimishpatel commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Apr 21, 2021 • 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_linux_xenial_py3_clang5_asan_test1 (1/2)

pytorch_xla_linux_bionic_py3_6_clang9_test (2/2)

Uh oh!

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

kimishpatel Apr 23, 2021 • 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!

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!

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

facebook-github-bot commented May 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kimishpatel commented Apr 21, 2021 •

edited

Loading

facebook-github-bot commented Apr 21, 2021 •

edited

Loading

kimishpatel Apr 23, 2021 •

edited

Loading