fix(module_fallback): Catching recursive search if method doesnt exist #619

narendasan · 2021-09-03T22:05:50Z

Description

There was an issue where the module fallback notation pass just assumes
methods exist. This should catch that. This might be something we may want to release as a patch

Fixes #613, (potentially #609, Also probably partially solves: #608)

Type of change

Please delete options that are not relevant and/or add your own.

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code follows the style guidelines of this project (You can use the linters)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas and hacks
I have made corresponding changes to the documentation
I have added tests to verify my fix or my feature
New and existing unit tests pass locally with my changes

github-actions

There are some changes that do not conform to C++ style guidelines:

diff --git a/workspace/core/lowering/lowering.cpp b/tmp/changes.txt
index 42be486..dae163a 100644
--- a/workspace/core/lowering/lowering.cpp
+++ b/tmp/changes.txt
@@ -60,10 +60,7 @@ void LowerGraph(std::shared_ptr<torch::jit::Graph>& g, LowerInfo lower_info) {
  LOG_GRAPH(*g);
}

-torch::jit::Module LowerModule(
-    const torch::jit::Module& mod,
-    std::string method_name,
-    const LowerInfo& lower_info) {
+torch::jit::Module LowerModule(const torch::jit::Module& mod, std::string method_name, const LowerInfo& lower_info) {
  std::unordered_set<std::string> forced_fallback_modules(
      lower_info.forced_fallback_modules.begin(), lower_info.forced_fallback_modules.end());
  passes::NotateModuleForFallback(mod, "", method_name, forced_fallback_modules);
ERROR: Some files do not conform to style guidelines

github-actions

There are some changes that do not conform to C++ style guidelines:

diff --git a/workspace/core/lowering/lowering.cpp b/tmp/changes.txt
index 42be486..dae163a 100644
--- a/workspace/core/lowering/lowering.cpp
+++ b/tmp/changes.txt
@@ -60,10 +60,7 @@ void LowerGraph(std::shared_ptr<torch::jit::Graph>& g, LowerInfo lower_info) {
  LOG_GRAPH(*g);
}

-torch::jit::Module LowerModule(
-    const torch::jit::Module& mod,
-    std::string method_name,
-    const LowerInfo& lower_info) {
+torch::jit::Module LowerModule(const torch::jit::Module& mod, std::string method_name, const LowerInfo& lower_info) {
  std::unordered_set<std::string> forced_fallback_modules(
      lower_info.forced_fallback_modules.begin(), lower_info.forced_fallback_modules.end());
  passes::NotateModuleForFallback(mod, "", method_name, forced_fallback_modules);
ERROR: Some files do not conform to style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

There are some changes that do not conform to C++ style guidelines:

diff --git a/workspace/core/lowering/lowering.cpp b/tmp/changes.txt
index 42be486..dae163a 100644
--- a/workspace/core/lowering/lowering.cpp
+++ b/tmp/changes.txt
@@ -60,10 +60,7 @@ void LowerGraph(std::shared_ptr<torch::jit::Graph>& g, LowerInfo lower_info) {
  LOG_GRAPH(*g);
}

-torch::jit::Module LowerModule(
-    const torch::jit::Module& mod,
-    std::string method_name,
-    const LowerInfo& lower_info) {
+torch::jit::Module LowerModule(const torch::jit::Module& mod, std::string method_name, const LowerInfo& lower_info) {
  std::unordered_set<std::string> forced_fallback_modules(
      lower_info.forced_fallback_modules.begin(), lower_info.forced_fallback_modules.end());
  passes::NotateModuleForFallback(mod, "", method_name, forced_fallback_modules);
ERROR: Some files do not conform to style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

ArvindSridhar · 2021-09-04T04:04:53Z

core/lowering/passes/module_fallback.cpp


  for (const auto sub_mod : mod.named_children()) {
-    NotateModuleForFallback(sub_mod.value, sub_mod.name, method_name, forced_fallback_modules);
+    if (sub_mod.value.find_method(method_name)) {


Is there ever a case where we might pass in a name other than "forward" as the method name here? In that case, would the desired behavior be to only recurse through submodules with that specific method (and ignore others, and their children)?

What we really should be doing is finding the Method calls and using that to recurse vs all children probably

peri044

One minor change maybe when forced_fallback_modules is empty ?
PR runs fine and BERT 1.9 graph passes

peri044 · 2021-09-09T16:56:32Z

core/lowering/lowering.cpp

+torch::jit::Module LowerModule(const torch::jit::Module& mod, std::string method_name, const LowerInfo& lower_info) {
+  std::unordered_set<std::string> forced_fallback_modules(
+      lower_info.forced_fallback_modules.begin(), lower_info.forced_fallback_modules.end());
  passes::NotateModuleForFallback(mod, "", method_name, forced_fallback_modules);


When the forced_fallback_modules is empty, we don't need to have this pass so we can disable it. This can prevent iterating through all the nodes in graph. Disabling this also fixed BERT issue but your other fix is necessary.

peri044 · 2021-09-22T19:03:03Z

Can we merge this ? Seems like we are getting duplicate "Method forward is not defined" bugs.

isgursoy · 2021-09-26T16:54:36Z

Yeah when can we expect to get this patch as a new version?

narendasan · 2021-09-28T15:14:56Z

We are trying to get out a patch release in the next couple weeks. The issue is I think this WAR is probably not the correct solution. I am trying to determine if a formal solution could be implemented in a reasonable amount of time

This commit fixes module level fallback by using method calls to determine modules to recurse down too. This should be robust to names other than forward used for methods as well as ignoring functional modules. Signed-off-by: Naren Dasan <[email protected]> Signed-off-by: Naren Dasan <[email protected]>

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to Python style guidelines

peri044

LGTM. Tests pass

narendasan · 2021-09-29T20:08:29Z

@peri044 can you also test with a network you know failed previously? Also try with module level fallback specifying a module that is not part of the module to be compiled

peri044 · 2021-09-29T22:39:59Z

@narendasan Tested a network (DETR) which failed previously. Tried a module which is not a part of the graph. Works fine.

narendasan requested review from ArvindSridhar and peri044 September 3, 2021 22:05

github-actions bot added component: core Issues re: The core compiler component: lowering Issues re: The lowering / preprocessing passes labels Sep 3, 2021

github-actions bot requested changes Sep 3, 2021

View reviewed changes

github-actions bot approved these changes Sep 3, 2021

View reviewed changes

github-actions bot requested changes Sep 3, 2021

View reviewed changes

narendasan force-pushed the fix_mod_fallback_methods branch from c049894 to 29ded84 Compare September 3, 2021 22:14

github-actions bot approved these changes Sep 3, 2021

View reviewed changes

github-actions bot approved these changes Sep 4, 2021

View reviewed changes

ArvindSridhar reviewed Sep 4, 2021

View reviewed changes

peri044 requested changes Sep 9, 2021

View reviewed changes

peri044 mentioned this pull request Sep 20, 2021

🐛 [Bug] Can't use trt calibrator to calibrate SSD300 #630

Closed

narendasan added the release: patch This change needs to go out as a patch to the current version label Sep 23, 2021

narendasan force-pushed the fix_mod_fallback_methods branch from f7240e3 to f94ae8f Compare September 29, 2021 05:07

narendasan requested a review from peri044 September 29, 2021 05:07

github-actions bot approved these changes Sep 29, 2021

View reviewed changes

peri044 approved these changes Sep 29, 2021

View reviewed changes

narendasan mentioned this pull request Oct 1, 2021

fix(//core/lowering): Fixes module level fallback recursion #648

Merged

narendasan merged commit 2918675 into master Oct 1, 2021

narendasan deleted the fix_mod_fallback_methods branch October 1, 2021 23:46

fix(module_fallback): Catching recursive search if method doesnt exist #619

fix(module_fallback): Catching recursive search if method doesnt exist #619

Uh oh!

Conversation

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

Uh oh!

Description

Type of change

Checklist:

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

ArvindSridhar Sep 4, 2021

Choose a reason for hiding this comment

Uh oh!

narendasan Sep 4, 2021

Choose a reason for hiding this comment

Uh oh!

peri044 left a comment

Choose a reason for hiding this comment

Uh oh!

peri044 Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

peri044 commented Sep 22, 2021

Uh oh!

isgursoy commented Sep 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

narendasan commented Sep 28, 2021

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

peri044 left a comment

Choose a reason for hiding this comment

Uh oh!

narendasan commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peri044 commented Sep 29, 2021

Uh oh!

Reviewers

narendasan commented Sep 3, 2021 •

edited

Loading

isgursoy commented Sep 26, 2021 •

edited

Loading

narendasan commented Sep 29, 2021 •

edited

Loading