[Graph C API] Enable plugin optimizers and configs. by Solaryee · Pull Request #47635 · tensorflow/tensorflow

Solaryee · 2021-03-08T08:40:35Z

This is the 3rd PR(and the last one) of Graph C API, following RFC Modular TensorFlow Graph C API.

This PR is mainly to add plugin optimizer and config in meta_optimizer, which includes:

Run plugin optimizer in meta_optimizer.
Change config if plugin optimizer is enabled, otherwise leave it as default. Print it when plugin config is conflicit with user config.
Call InitGraphPlugin in RegisterPluggableDevicePlugin.

Solaryee · 2021-03-08T08:41:35Z

Hi @penpornk @ezhulenev, please help to have a review. Thanks very much!

Jianhui-Li · 2021-03-11T01:01:36Z

@penpornk This PR is the 3rd Graph C API PR. Without this PR, users won't be able to use graph plugin.

ezhulenev · 2021-03-11T18:39:05Z

tensorflow/core/grappler/optimizers/meta_optimizer.cc

  return mem_opt_type != RewriterConfig::NO_MEM_OPT;
 }

+Status GetGraphDevice(const GraphDef& g_def, std::set<std::string>& devices) {


Output parameters must be taken by pointer.

ezhulenev · 2021-03-11T18:39:34Z

tensorflow/core/grappler/optimizers/meta_optimizer.cc

 }

+Status GetGraphDevice(const GraphDef& g_def, std::set<std::string>& devices) {
+  for (auto node : g_def.node()) {


I think auto will do a copy here, you'll need auto&.

ezhulenev · 2021-03-11T18:43:01Z

tensorflow/core/grappler/optimizers/meta_optimizer.cc

    optimizers->push_back(MakeUnique<ModelPruner>());
  }
-  if (cfg_.implementation_selector() != RewriterConfig::OFF) {
+  if (cfg_.implementation_selector() != RewriterConfig::OFF &&


Maybe move this repeated pattern to macro?

cfg_.implementation_selector() != RewriterConfig::OFF && plugin_configs.toggle_config["implementation_selector"]

The pattern is not exactly the same for different optimizers, so I use 6 macros here.

#define USER_IS_ON(CFG) cfg_.CFG() == RewriterConfig::ON #define USER_NOT_OFF(CFG) cfg_.CFG() != RewriterConfig::OFF #define PLUGIN_IS_ON(CFG) plugin_configs.toggle_config[#CFG] == RewriterConfig::ON #define PLUGIN_NOT_OFF(CFG) plugin_configs.toggle_config[#CFG] != RewriterConfig::OFF #define BOTH_IS_ON(CFG) USER_IS_ON(CFG) && PLUGIN_IS_ON(CFG) #define BOTH_NOT_OFF(CFG) USER_NOT_OFF(CFG) && PLUGIN_NOT_OFF(CFG)

ezhulenev · 2021-03-11T18:44:04Z

tensorflow/core/grappler/optimizers/meta_optimizer.cc

+}
+
+Status MetaOptimizer::InitializePluginGraphOptimizers(
+    std::vector<std::unique_ptr<GraphOptimizer>>* optimizers,


Output parameters should be at the end.

OK. Done for all functions.

tensorflow/core/grappler/optimizers/meta_optimizer.cc

Solaryee · 2021-03-12T08:13:12Z

@ezhulenev Thanks for your review! I have addressed all your comments.

gbaned · 2021-03-17T14:11:59Z

@ShengYang1 Can you please resolve conflicts? Thanks!

Solaryee · 2021-03-18T03:06:18Z

@ShengYang1 Can you please resolve conflicts? Thanks!

I have already rebased, please review. Thanks!

penpornk · 2021-03-18T05:16:11Z

#45784 just got merged. Could you please resolve conflicts again? Thank you!

Solaryee · 2021-03-18T10:05:06Z

@penpornk Already rebased.

Meanwhile I have slightly changed the logic in RegisterPluggableDevicePlugin. In the previous design, it will directly return error if SE_InitPlugin is not found. However, plugin with graph only should also be allowed, so I changed it to "return error if neither device nor graph is found". Suggestions are welcome. Thanks.

penpornk

Meanwhile I have slightly changed the logic in RegisterPluggableDevicePlugin. In the previous design, it will directly return error if SE_InitPlugin is not found. However, plugin with graph only should also be allowed, so I changed it to "return error if neither device nor graph is found". Suggestions are welcome. Thanks.

Thank you very much! I was about to ask for that too, because the use case with just the graph plugin is also valid in the original RFC (2nd scenario).

I marked some trivial typos. No need to fix them. I will fix them internally.

The main PluggableDevice implementation PR (#45784) missed last night's nightly test, so we'll know tomorrow whether it will get reverted. I will merge this PR after I'm sure #45784 won't get reverted.

tensorflow/core/grappler/optimizers/meta_optimizer.cc

…tensorflow#47635.) Context: [1] Grappler C API RFC https://github.com/tensorflow/community/blob/master/rfcs/20201027-modular-tensorflow-graph-c-api.md [2] Pluggable Grappler pass registration PR tensorflow#47635. PiperOrigin-RevId: 363788388 Change-Id: I8463382748c2d7c211ee80e0f9907276f6a20ad2

…#47635.) Context: [1] Grappler C API RFC https://github.com/tensorflow/community/blob/master/rfcs/20201027-modular-tensorflow-graph-c-api.md [2] Pluggable Grappler pass registration PR #47635. PiperOrigin-RevId: 363803765 Change-Id: I14b46889fc87bf7d64215dd54788398cae5a846f

penpornk · 2021-03-19T21:57:07Z

So I have good news and bad news.

Good news: This PR is merged, and the main PluggableDevice PR is now confirmed safe (based on last night's tests)! 🎉

Bad news: There has been some issues adding the new RewriterConfig option (use_plugin_optimizers). I added it separately here but the change got reverted. So I made this PR use kUsePluginOptimizers = RewriterConfig::ON; in metaoptimizer.cc and removed the changes in eager/context.py for now. I will change them back once I can get the RewriterConfig change back in.

Solaryee · 2021-03-20T05:36:13Z

@penpornk Thanks for your effort to get this PR merged. It's really good news🎉.

…d to complete pluggable Grappler registration PR #47635. Context: [1] Grappler C API RFC https://github.com/tensorflow/community/blob/master/rfcs/20201027-modular-tensorflow-graph-c-api.md [2] Pluggable Grappler pass registration PR #47635. PiperOrigin-RevId: 364412108 Change-Id: I0152d83de1b7ccc9da751080551f992d1ee46349

…Config and Python interface. This is the rest of the changes from PR #47635 that was hold off because there was a problem adding a new parameter (`use_plugin_optimizers`) to `RewriterConfig`. `use_plugin_optimizers` is now in `RewriteConfig` (197817f) PiperOrigin-RevId: 364617915 Change-Id: I7a1b1bb7167b4628a16ebcc551d4839aa559fda0

Enable plugin optimizer

a6870d9

Solaryee requested review from kkimdev and rohan100jain as code owners March 8, 2021 08:40

google-ml-butler bot added the size:L CL Change Size: Large label Mar 8, 2021

google-cla bot added the cla: yes label Mar 8, 2021

gbaned self-assigned this Mar 8, 2021

penpornk requested review from ezhulenev and penpornk March 11, 2021 01:06

ezhulenev suggested changes Mar 11, 2021

View reviewed changes

Refine with macro, change param position

b76178d

gbaned requested a review from ezhulenev March 12, 2021 14:38

ezhulenev approved these changes Mar 12, 2021

View reviewed changes

google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 12, 2021

kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 12, 2021

penpornk added the kokoro:force-run Tests on submitted change label Mar 12, 2021

kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 12, 2021

gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Mar 16, 2021

gbaned added stat:awaiting response Status - Awaiting response from author and removed ready to pull PR ready for merge process labels Mar 17, 2021

Merge remote-tracking branch 'github/master' into yang/graph-api-meta

92b25fa

Merge remote-tracking branch 'github/master' into yang/graph-api-meta

05dccdb

Raise error if neither device nor graph is found

1fbe9a2

penpornk approved these changes Mar 18, 2021

View reviewed changes

tensorflow/core/grappler/optimizers/meta_optimizer.cc Show resolved Hide resolved

tensorflow/core/grappler/optimizers/meta_optimizer.cc Show resolved Hide resolved

tensorflow/core/grappler/optimizers/meta_optimizer.cc Show resolved Hide resolved

google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 18, 2021

kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 18, 2021

copybara-service bot merged commit 1342121 into tensorflow:master Mar 19, 2021

gbaned removed the stat:awaiting response Status - Awaiting response from author label Mar 21, 2021

Solaryee mentioned this pull request Mar 22, 2021

[Graph C API] Updating warning messages #47969

Merged

Comments

Conversation

Solaryee commented Mar 8, 2021

Uh oh!

Solaryee commented Mar 8, 2021

Uh oh!

Jianhui-Li commented Mar 11, 2021

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!

Uh oh!

Solaryee commented Mar 12, 2021

Uh oh!

gbaned commented Mar 17, 2021

Uh oh!

Solaryee commented Mar 18, 2021

Uh oh!

penpornk commented Mar 18, 2021

Uh oh!

Solaryee commented Mar 18, 2021

Uh oh!

penpornk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

penpornk commented Mar 19, 2021

Uh oh!

Solaryee commented Mar 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants