Fix flaky profiler and test_callback_simple RPC tests #41287

rohan-varma · 2020-07-10T23:46:30Z

Stack from ghstack:

Fix flaky profiler and test_callback_simple RPC tests #41287 Fix flaky profiler and test_callback_simple RPC tests

Profiler tests that test profiling with builtin functions and test_callback_simple test has been broken for a while. This diff fixes that by preferring c10 ops to non-c10 ops in our operation matching logic.

The result of this is that these ops go through the c10 dispatch and thus have profiling enabled. For test_callback_simple this results in the effect that we choose aten::add.Tensor over aten::add.Int which fixes the type issue.

Differential Revision: D22489197

NOTE FOR REVIEWERS: This PR has internal Facebook specific changes or comments, please review them on Phabricator!

Profiler tests that test profiling with builtin functions and `test_callback_simple` test has been broken for a while. This diff fixes that by preferring c10 ops to non-c10 ops in our operation matching logic. The result of this is that these ops go through the c10 dispatch and thus have profiling enabled. For `test_callback_simple` this results in the effect that we choose `aten::add.Tensor` over `aten::add.Int` which fixes the type issue. Differential Revision: [D22489197](https://our.internmc.facebook.com/intern/diff/D22489197/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D22489197/)! [ghstack-poisoned]

Pull Request resolved: #41287 Profiler tests that test profiling with builtin functions and `test_callback_simple` test has been broken for a while. This diff fixes that by preferring c10 ops to non-c10 ops in our operation matching logic. The result of this is that these ops go through the c10 dispatch and thus have profiling enabled. For `test_callback_simple` this results in the effect that we choose `aten::add.Tensor` over `aten::add.Int` which fixes the type issue. Differential Revision: [D22489197](https://our.internmc.facebook.com/intern/diff/D22489197/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D22489197/)! ghstack-source-id: 107572222

lw

I'm not familiar with the code but the change is simple and looks safe as it only changes the order so to me it looks good! But maybe someone who knows the JIT dispatch better can provide more insight...

mrshenli · 2020-07-13T22:02:03Z

torch/csrc/distributed/rpc/python_functions.cpp

+      // torch.add) so a worst-case linear search should not incur significant
+      // extra overhead.
+      if (op->isC10Op()) {
+        return op;


Thanks for fixing! And thanks for digging into the bottom of this error!

This op-matching logic does need a thorough revamp.

Profiler tests that test profiling with builtin functions and `test_callback_simple` test has been broken for a while. This diff fixes that by preferring c10 ops to non-c10 ops in our operation matching logic. The result of this is that these ops go through the c10 dispatch and thus have profiling enabled. For `test_callback_simple` this results in the effect that we choose `aten::add.Tensor` over `aten::add.Int` which fixes the type issue. Differential Revision: [D22489197](https://our.internmc.facebook.com/intern/diff/D22489197/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D22489197/)! [ghstack-poisoned]

Pull Request resolved: #41287 Profiler tests that test profiling with builtin functions and `test_callback_simple` test has been broken for a while. This diff fixes that by preferring c10 ops to non-c10 ops in our operation matching logic. The result of this is that these ops go through the c10 dispatch and thus have profiling enabled. For `test_callback_simple` this results in the effect that we choose `aten::add.Tensor` over `aten::add.Int` which fixes the type issue. Differential Revision: [D22489197](https://our.internmc.facebook.com/intern/diff/D22489197/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D22489197/)! ghstack-source-id: 107680843

Profiler tests that test profiling with builtin functions and `test_callback_simple` test has been broken for a while. This diff fixes that by preferring c10 ops to non-c10 ops in our operation matching logic. The result of this is that these ops go through the c10 dispatch and thus have profiling enabled. For `test_callback_simple` this results in the effect that we choose `aten::add.Tensor` over `aten::add.Int` which fixes the type issue. Differential Revision: [D22489197](https://our.internmc.facebook.com/intern/diff/D22489197/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D22489197/)! [ghstack-poisoned]

dr-ci · 2020-07-14T02:46:56Z

💊 CI failures summary and remediations

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

💚 💚 Looks good so far! There are no failures yet. 💚 💚

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

Profiler tests that test profiling with builtin functions and `test_callback_simple` test has been broken for a while. This diff fixes that by preferring c10 ops to non-c10 ops in our operation matching logic. The result of this is that these ops go through the c10 dispatch and thus have profiling enabled. For `test_callback_simple` this results in the effect that we choose `aten::add.Tensor` over `aten::add.Int` which fixes the type issue. Differential Revision: [D22489197](https://our.internmc.facebook.com/intern/diff/D22489197/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D22489197/)! [ghstack-poisoned]

Pull Request resolved: #41287 Profiler tests that test profiling with builtin functions and `test_callback_simple` test has been broken for a while. This diff fixes that by preferring c10 ops to non-c10 ops in our operation matching logic. The result of this is that these ops go through the c10 dispatch and thus have profiling enabled. For `test_callback_simple` this results in the effect that we choose `aten::add.Tensor` over `aten::add.Int` which fixes the type issue. Differential Revision: [D22489197](https://our.internmc.facebook.com/intern/diff/D22489197/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D22489197/)! ghstack-source-id: 107697506

facebook-github-bot · 2020-07-15T04:11:44Z

This pull request has been merged in fd03290.

facebook-github-bot · 2020-07-15T04:11:52Z

This pull request has been merged in fd03290.

As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/) [ghstack-poisoned]

…wing" As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/) [ghstack-poisoned]

Pull Request resolved: #49009 As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. ghstack-source-id: 118076202 Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/)

…wing" As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/) [ghstack-poisoned]

Pull Request resolved: #49009 As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. ghstack-source-id: 118122150 Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25392905/)!

…wing" As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/) [ghstack-poisoned]

Pull Request resolved: #49009 As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. ghstack-source-id: 118457676 Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25392905/)!

…wing" As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/) [ghstack-poisoned]

Pull Request resolved: #49009 As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. ghstack-source-id: 118746790 Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25392905/)!

…wing" As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/) [ghstack-poisoned]

Pull Request resolved: #49009 As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. ghstack-source-id: 118747511 Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25392905/)!

…wing" As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/) [ghstack-poisoned]

Pull Request resolved: #49009 As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. ghstack-source-id: 118794661 Differential Revision: [D25392905](https://our.internmc.facebook.com/intern/diff/D25392905/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25392905/)!

Summary: Pull Request resolved: #49009 As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by #41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. ghstack-source-id: 118794661 Test Plan: CI Reviewed By: mrshenli Differential Revision: D25392905 fbshipit-source-id: 6f93251635740bcf902824548b2bc6f9249be5f0

…ch#49009) Summary: Pull Request resolved: pytorch#49009 As per the title, we should generally not have exception swalling and this commit makes it so that if there is a true error in JIT operator resolution, it is propagated back to the RPC callee and we don't silently swallow any other exceptions that may happen. Swallowing the exceptions previously resulted in hard to debug issues such as unexpected ops showing up in profiler, and flaky tests which were fixed by pytorch#41287 Added a unittest that validates the error that comes from `jit/pybind_utils.h`. ghstack-source-id: 118794661 Test Plan: CI Reviewed By: mrshenli Differential Revision: D25392905 fbshipit-source-id: 6f93251635740bcf902824548b2bc6f9249be5f0

rohan-varma requested review from mrshenli, pritamdamania87 and zhaojuanmao as code owners July 10, 2020 23:46

rohan-varma requested a review from lw July 13, 2020 20:18

lw approved these changes Jul 13, 2020

View reviewed changes

mrshenli approved these changes Jul 13, 2020

View reviewed changes

facebook-github-bot closed this in fd03290 Jul 15, 2020

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

facebook-github-bot deleted the gh/rohan-varma/148/head branch July 18, 2020 14:16

mruberry added the Merged label Oct 28, 2020

rohan-varma mentioned this pull request Dec 8, 2020

Refactor RPC matchBuiltInOp to get rid of exception swallowing #49009

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flaky profiler and test_callback_simple RPC tests #41287

Fix flaky profiler and test_callback_simple RPC tests #41287

rohan-varma commented Jul 10, 2020 •

edited

lw left a comment

mrshenli Jul 13, 2020

dr-ci bot commented Jul 14, 2020 •

edited

facebook-github-bot commented Jul 15, 2020

facebook-github-bot commented Jul 15, 2020

Fix flaky profiler and test_callback_simple RPC tests #41287

Fix flaky profiler and test_callback_simple RPC tests #41287

Conversation

rohan-varma commented Jul 10, 2020 • edited

lw left a comment

Choose a reason for hiding this comment

mrshenli Jul 13, 2020

Choose a reason for hiding this comment

dr-ci bot commented Jul 14, 2020 • edited

💊 CI failures summary and remediations

facebook-github-bot commented Jul 15, 2020

facebook-github-bot commented Jul 15, 2020

rohan-varma commented Jul 10, 2020 •

edited

dr-ci bot commented Jul 14, 2020 •

edited